On Sat, 17 Aug 2024 19:33:23 +0800 Menglong Dong wrote:

On Sat, Aug 17, 2024 at 10:22 AM Jakub Kicinski [off-list ref] wrote:

quoted

On Thu, 15 Aug 2024 20:42:58 +0800 Menglong Dong wrote:  

quoted

 #define VXLAN_DROP_REASONS(R)                        \
+     R(VXLAN_DROP_FLAGS)                     \
+     R(VXLAN_DROP_VNI)                       \
+     R(VXLAN_DROP_MAC)                       \  

Drop reasons should be documented.  

Yeah, I wrote the code here just like what we did in
net/openvswitch/drop.h, which makes the definition of
enum ovs_drop_reason a call of VXLAN_DROP_REASONS().

I think that we can define the enum ovs_drop_reason just like
what we do in include/net/dropreason-core.h, which can make
it easier to document the reasons.

quoted

I don't think name of a header field is a great fit for a reason.
 

Enn...Do you mean the "VXLAN_DROP_" prefix?

No, I mean the thing after VXLAN_DROP_, it's FLAGS, VNI, MAC,
those are names of header fields.

quoted

quoted

@@ -1815,8 +1831,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
      return 0;

 drop:
+     SKB_DR_RESET(reason);  

the name of this macro is very confusing, I don't think it should exist
in the first place. nothing should goto drop without initialing reason
 

It's for the case that we call a function which returns drop reasons.
For example, the reason now is assigned from:

  reason = pskb_may_pull_reason(skb, VXLAN_HLEN);
  if (reason) goto drop;

  xxxxxx
  if (xx) goto drop;

The reason now is SKB_NOT_DROPPED_YET when we "goto drop",
as we don't set a drop reason here, which is unnecessary in some cases.
And, we can't set the drop reason for every "drop" code path, can we?

Why? It's like saying "we can't set return code before jumping to
an error label". In my mind drop reasons and function return codes
are very similar. So IDK why we need all the SK_DR_ macros when
we are just fine without them for function return codes.