netdev - Re: [PATCH net-next v2 6/8] netdevsim: add queue management API support

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHS8izO3FWZ6Wgnf0jwHLo8xDczz1zmCq_ypXRAWijYuxUY0MA@mail.gmail.com>
Date: Tue, 7 Jan 2025 14:53:09 -0800
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org, 
	willemdebruijn.kernel@...il.com, sdf@...ichev.me, 
	Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next v2 6/8] netdevsim: add queue management API support

On Tue, Jan 7, 2025 at 8:11 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Add queue management API support. We need a way to reset queues
> to test NAPI reordering, the queue management API provides a
> handy scaffolding for that.
>
> Reviewed-by: Willem de Bruijn <willemb@...gle.com>
> Acked-by: Stanislav Fomichev <sdf@...ichev.me>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Reviewed-by: Mina Almasry <almasrymina@...gle.com>

> ---
> v2:
>  - don't null-check page pool before page_pool_destroy()
>  - controled -> controlled
> ---
>  drivers/net/netdevsim/netdev.c    | 134 +++++++++++++++++++++++++++---
>  drivers/net/netdevsim/netdevsim.h |   2 +
>  2 files changed, 124 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 7b80796dbe26..cfb079a34532 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -359,25 +359,24 @@ static int nsim_poll(struct napi_struct *napi, int budget)
>         return done;
>  }
>
> -static int nsim_create_page_pool(struct nsim_rq *rq)
> +static int nsim_create_page_pool(struct page_pool **p, struct napi_struct *napi)
>  {
> -       struct page_pool_params p = {
> +       struct page_pool_params params = {
>                 .order = 0,
>                 .pool_size = NSIM_RING_SIZE,
>                 .nid = NUMA_NO_NODE,
> -               .dev = &rq->napi.dev->dev,
> -               .napi = &rq->napi,
> +               .dev = &napi->dev->dev,
> +               .napi = napi,
>                 .dma_dir = DMA_BIDIRECTIONAL,
> -               .netdev = rq->napi.dev,
> +               .netdev = napi->dev,
>         };
> +       struct page_pool *pool;
>
> -       rq->page_pool = page_pool_create(&p);
> -       if (IS_ERR(rq->page_pool)) {
> -               int err = PTR_ERR(rq->page_pool);
> +       pool = page_pool_create(&params);
> +       if (IS_ERR(pool))
> +               return PTR_ERR(pool);
>
> -               rq->page_pool = NULL;
> -               return err;
> -       }
> +       *p = pool;
>         return 0;
>  }
>
> @@ -396,7 +395,7 @@ static int nsim_init_napi(struct netdevsim *ns)
>         for (i = 0; i < dev->num_rx_queues; i++) {
>                 rq = ns->rq[i];
>
> -               err = nsim_create_page_pool(rq);
> +               err = nsim_create_page_pool(&rq->page_pool, &rq->napi);
>                 if (err)
>                         goto err_pp_destroy;
>         }
> @@ -613,6 +612,116 @@ static void nsim_queue_free(struct nsim_rq *rq)
>         kfree(rq);
>  }
>
> +/* Queue reset mode is controlled by ns->rq_reset_mode.
> + * - normal - new NAPI new pool (old NAPI enabled when new added)

Nit, probably not worth a respin: Normal seems to me to delete old
napi after the new one is added and enabled.

queue stop -> napi_disable(old)
queue alloc -> netif_napi_add_config(new)
queue start -> napi_enable(new)
queue free -> netif_napi_del(old)

> + * - mode 1 - allocate new pool (NAPI is only disabled / enabled)
> + * - mode 2 - new NAPI new pool (old NAPI removed before new added)
> + * - mode 3 - new NAPI new pool (old NAPI disabled when new added)
> + */

Which modes are 'correct' for a driver to implement? 2/3 is for
testing only, as you note in the code, the add/del functions should
really be called from alloc/free and not from queue_start. I assume
modes normal and 1 are both correct implementations of the queue API
and the driver gets to pick whether to reuse the napi instance or not?
Asking because IIRC GVE implements mode 1, not what you consider
'normal'.


--
Thanks,
Mina