[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20230421080339.x2fllg65qmcrk6vk@soft-dev3-1>
Date: Fri, 21 Apr 2023 10:03:39 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<bpf@...r.kernel.org>, <davem@...emloft.net>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<daniel@...earbox.net>, <hawk@...nel.org>,
<john.fastabend@...il.com>, <richardcochran@...il.com>,
<UNGLinuxDriver@...rochip.com>, <alexandr.lobakin@...el.com>
Subject: Re: [PATCH net-next] net: lan966x: Don't use xdp_frame when action
is XDP_TX
The 04/20/2023 22:52, Maciej Fijalkowski wrote:
> [Some people who received this message don't often get email from maciej.fijalkowski@...el.com. 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@...rochip.com>
> > ---
> > .../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
Powered by blists - more mailing lists