netdev - Re: [PATCH net] bnxt_en: update xdp_rxq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMArcTXm=4sfJ3oLf7-ZxobNyoeHWrg+-_Yirm4QszoaVnSbfA@mail.gmail.com>
Date: Sun, 21 Jul 2024 00:06:27 +0900
From: Taehee Yoo <ap420073@...il.com>
To: David Wei <dw@...idwei.uk>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, 
	edumazet@...gle.com, michael.chan@...adcom.com, netdev@...r.kernel.org, 
	somnath.kotur@...adcom.com, horms@...nel.org
Subject: Re: [PATCH net] bnxt_en: update xdp_rxq_info in queue restart logic

On Sat, Jul 20, 2024 at 2:13 PM David Wei <dw@...idwei.uk> wrote:
>

Hi David,
Thanks a lot for the review!

> On 2024-07-18 21:19, Taehee Yoo wrote:
> > When the netdev_rx_queue_restart() restarts queues, the bnxt_en driver
> > updates(creates and deletes) a page_pool.
> > But it doesn't update xdp_rxq_info, so the xdp_rxq_info is still
> > connected to an old page_pool.
> > So, bnxt_rx_ring_info->page_pool indicates a new page_pool, but
> > bnxt_rx_ring_info->xdp_rxq is still connected to an old page_pool.
> >
> > An old page_pool is no longer used so it is supposed to be
> > deleted by page_pool_destroy() but it isn't.
> > Because the xdp_rxq_info is holding the reference count for it and the
> > xdp_rxq_info is not updated, an old page_pool will not be deleted in
> > the queue restart logic.
> >
> > Before restarting 1 queue:
> > ./tools/net/ynl/samples/page-pool
> > enp10s0f1np1[6] page pools: 4 (zombies: 0)
> >       refs: 8192 bytes: 33554432 (refs: 0 bytes: 0)
> >       recycling: 0.0% (alloc: 128:8048 recycle: 0:0)
> >
> > After restarting 1 queue:
> > ./tools/net/ynl/samples/page-pool
> > enp10s0f1np1[6] page pools: 5 (zombies: 0)
> >       refs: 10240 bytes: 41943040 (refs: 0 bytes: 0)
> >       recycling: 20.0% (alloc: 160:10080 recycle: 1920:128)
>
> Thanks, didn't know this existed! As a follow up once Mina lands his
> devmem TCP series with netdev_rx_queue_restart(), a netdev netlink
> selftest using would be great.
>

Yes, that would be great!

> >
> > Before restarting queues, an interface has 4 page_pools.
> > After restarting one queue, an interface has 5 page_pools, but it
> > should be 4, not 5.
> > The reason is that queue restarting logic creates a new page_pool and
> > an old page_pool is not deleted due to the absence of an update of
> > xdp_rxq_info logic.
> >
> > Fixes: 2d694c27d32e ("bnxt_en: implement netdev_queue_mgmt_ops")
> > Signed-off-by: Taehee Yoo <ap420073@...il.com>
> > ---
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index bb3be33c1bbd..11d8459376a9 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> [...]
> > @@ -15065,6 +15079,8 @@ static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
> >       page_pool_destroy(rxr->page_pool);
> >       rxr->page_pool = NULL;
> >
> > +     xdp_rxq_info_unreg(&rxr->xdp_rxq);
> > +
>
> IMO this should go before page_pool_destroy() for symmetry with
> bnxt_free_rx_rings(). I know there's already a call deep inside of
> xdp_rxq_info_unreg().
>

I agree about symmetry. So I will change it.

> >       ring = &rxr->rx_ring_struct;
> >       bnxt_free_ring(bp, &ring->ring_mem);
> >
> > @@ -15145,6 +15161,7 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
> >       rxr->rx_sw_agg_prod = clone->rx_sw_agg_prod;
> >       rxr->rx_next_cons = clone->rx_next_cons;
> >       rxr->page_pool = clone->page_pool;
> > +     memcpy(&rxr->xdp_rxq, &clone->xdp_rxq, sizeof(struct xdp_rxq_info));
>
> Assignment is fine here.

Okay, I will use the assignment instead of the memcpy.

I will send a v2 patch after some tests.

Thanks a lot!
Taehee Yoo