Re: [PATCH net-next v3 7/7] net: lan966x: Add support for XDP_REDIRECT

[Alexander Lobakin]

From: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
Date: Tue, 22 Nov 2022 22:37:24 +0100

> The 11/22/2022 13:04, Alexander Lobakin wrote:
> > 
> > From: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> > Date: Mon, 21 Nov 2022 22:28:50 +0100
> > 
> > > Extend lan966x XDP support with the action XDP_REDIRECT. This is similar
> > > with the XDP_TX, so a lot of functionality can be reused.
> > >
> > > Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> > > ---
> > >  .../ethernet/microchip/lan966x/lan966x_fdma.c | 83 +++++++++++++++----
> > >  .../ethernet/microchip/lan966x/lan966x_main.c |  1 +
> > >  .../ethernet/microchip/lan966x/lan966x_main.h | 10 ++-
> > >  .../ethernet/microchip/lan966x/lan966x_xdp.c  | 31 ++++++-
> > >  4 files changed, 109 insertions(+), 16 deletions(-)

[...]

> > I suggest carefully inspecting this struct with pahole (or by just
> > printkaying its layout/sizes/offsets at runtime) and see if there's
> > any holes and how it could be optimized.
> > Also, it's just my personal preference, but it's not that unpopular:
> > I don't trust bools inside structures as they may surprise with
> > their sizes or alignment depending on the architercture. Considering
> > all the blah I wrote, I'd define it as:
> > 
> > struct lan966x_tx_dcb_buf {
> >         dma_addr_t dma_addr;            // can be 8 bytes on 32-bit plat
> >         struct net_device *dev;         // ensure natural alignment
> >         struct sk_buff *skb;
> >         struct xdp_frame *xdpf;
> >         u32 len;
> >         u32 xdp_ndo:1;                  // put all your booleans here in
> >         u32 used:1;                     // one u32
> >         ...
> > };
> 
> Thanks for the suggestion. I make sure not that this struct will not
> have any holes.
> Can it be a rule of thumb, that every time when a new member is added to
> a struct, to make sure that it doesn't introduce any holes?

Yass, it's always good to do a quick check each time you're making
changes in a structure. This can prevent not only from excessive
memory usage, but most important from performance hits when some
hot field gets pushed out of the cacheline the field was in
previously.
Minimizing holes and using `u32 :1` vs `bool` for flags is more of
my personal preference, but it's kinda backed by experience, so I
treat it as something worth sharing :D

> 
> > 
> > BTW, we usually do union { skb, xdpf } since they're mutually
> > exclusive. And to distinguish between XDP and regular Tx you can use
> > one more bit/bool. This can also come handy later when you add XSk
> > support (you will be adding it, right? Please :P).
> 
> I think I will take this battle at later point when I will add XSK :)
> After I finish with this patch series, I will need to focus on some VCAP
> support for lan966x.

Sure!

> And maybe after that I will be able to add XSK. Because I need to look
> more at this XSK topic as I have looked too much on it before but I heard
> a lot of great things about it :)

Depends on the real usecases of the hardware. But always good to see
more drivers supporting it :>

> 
> > 
> > >       int len;
> > >       dma_addr_t dma_addr;
> > >       bool used;
> > 
> > [...]
> > 
> > > --
> > > 2.38.0
> > 
> > Thanks,
> > Olek
> 
> -- 
> /Horatiu

Thanks,
Olek