Re: [PATCH net-next] net: lan966x: Don't use xdp_frame when action is XDP_TX

[Horatiu Vultur]

The 04/20/2023 22:52, Maciej Fijalkowski wrote:
> [Some people who received this message don't often get email from maciej.fijalkowski@xxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 

Hi Maciej,

> 
> On Thu, Apr 20, 2023 at 02:11:52PM +0200, Horatiu Vultur wrote:
> 
> 'net: ' in patch subject is excessive to me

I usually have set this in the subject. I can remove this in the next
version and I will try to keep in mind for other patches for lan966x.

> 
> > When the action of an xdp program was XDP_TX, lan966x was creating
> > a xdp_frame and use this one to send the frame back. But it is also
> > possible to send back the frame without needing a xdp_frame, because
> > it possible to send it back using the page.
> 
> s/it/it is
> 
> > And then once the frame is transmitted is possible to use directly
> > page_pool_recycle_direct as lan966x is using page pools.
> > This would save some CPU usage on this path.
> 
> i remember this optimization gave me noticeable perf improvement, would
> you mind sharing it in % on your side?

The way I have done the measurements, is to measure actually how much
more traffic can be send back. I tried with different frame sizes,
frame size     improvement
64                ~8%
256              ~11%
512               ~8%
1000              ~0%
1500              ~0%

I will make sure do add this to the comments in the next version.

> 
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> > ---
> >  .../ethernet/microchip/lan966x/lan966x_fdma.c | 35 +++++++++++--------
> >  .../ethernet/microchip/lan966x/lan966x_main.h |  2 ++
> >  .../ethernet/microchip/lan966x/lan966x_xdp.c  | 11 +++---
> >  3 files changed, 27 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > index 2ed76bb61a731..7947259e67e4e 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > @@ -390,6 +390,7 @@ static void lan966x_fdma_stop_netdev(struct lan966x *lan966x)
> >  static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
> >  {
> >       struct lan966x_tx *tx = &lan966x->tx;
> > +     struct lan966x_rx *rx = &lan966x->rx;
> >       struct lan966x_tx_dcb_buf *dcb_buf;
> >       struct xdp_frame_bulk bq;
> >       struct lan966x_db *db;
> > @@ -432,7 +433,8 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
> >                       if (dcb_buf->xdp_ndo)
> >                               xdp_return_frame_bulk(dcb_buf->data.xdpf, &bq);
> >                       else
> > -                             xdp_return_frame_rx_napi(dcb_buf->data.xdpf);
> > +                             page_pool_recycle_direct(rx->page_pool,
> > +                                                      dcb_buf->data.page);
> >               }
> >
> >               clear = true;
> > @@ -702,6 +704,7 @@ static void lan966x_fdma_tx_start(struct lan966x_tx *tx, int next_to_use)
> >  int lan966x_fdma_xmit_xdpf(struct lan966x_port *port,
> >                          struct xdp_frame *xdpf,
> >                          struct page *page,
> > +                        u32 len,
> 
> agreed with Olek regarding arguments reduction here

Yes, I will change this in the next version.

> 
> >                          bool dma_map)
> >  {
> >       struct lan966x *lan966x = port->lan966x;
> > @@ -722,6 +725,15 @@ int lan966x_fdma_xmit_xdpf(struct lan966x_port *port,
> >               goto out;
> >       }
> >
> > +     /* Fill up the buffer */
> > +     next_dcb_buf = &tx->dcbs_buf[next_to_use];
> > +     next_dcb_buf->use_skb = false;
> > +     next_dcb_buf->xdp_ndo = dma_map;
> 
> a bit misleading that xdp_ndo is a bool :P

There are few other variables that are misleading :), I need to get to
this and clean it a little bit.

> 
> > +     next_dcb_buf->len = len + IFH_LEN_BYTES;
> > +     next_dcb_buf->used = true;
> > +     next_dcb_buf->ptp = false;
> > +     next_dcb_buf->dev = port->dev;
> > +
> >       /* Generate new IFH */
> >       if (dma_map) {
> >               if (xdpf->headroom < IFH_LEN_BYTES) {
-- 
/Horatiu