Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan_rcv()

[Jakub Kicinski <kuba@kernel.org>]

On Sat, 17 Aug 2024 19:33:23 +0800 Menglong Dong wrote:
> On Sat, Aug 17, 2024 at 10:22 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu, 15 Aug 2024 20:42:58 +0800 Menglong Dong wrote:  
> > >  #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.
> 
> > 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.

> > > @@ -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.