netdev - Re: [PATCH net-next 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <677d344a30383_25382b29446@willemb.c.googlers.com.notmuch>
Date: Tue, 07 Jan 2025 09:03:54 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, 
 davem@...emloft.net
Cc: netdev@...r.kernel.org, 
 edumazet@...gle.com, 
 pabeni@...hat.com, 
 dw@...idwei.uk, 
 almasrymina@...gle.com, 
 jdamato@...tly.com, 
 Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next 6/8] netdevsim: add queue management API support

Jakub Kicinski 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.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  drivers/net/netdevsim/netdev.c    | 135 +++++++++++++++++++++++++++---
>  drivers/net/netdevsim/netdevsim.h |   2 +
>  2 files changed, 125 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index e1bd3c1563b7..86614292314a 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,117 @@ static void nsim_queue_free(struct nsim_rq *rq)
>  	kfree(rq);
>  }
>  
> +/* Queue reset mode is controled by ns->rq_reset_mode.

controlled

also perhaps an enum for the modes?

> + * - normal - new NAPI new pool (old NAPI enabled when new added)
> + * - 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)
> + */
> +struct nsim_queue_mem {
> +	struct nsim_rq *rq;
> +	struct page_pool *pp;
> +};
> +
> +static int
> +nsim_queue_mem_alloc(struct net_device *dev, void *per_queue_mem, int idx)
> +{
> +	struct nsim_queue_mem *qmem = per_queue_mem;
> +	struct netdevsim *ns = netdev_priv(dev);
> +	int err;
> +
> +	if (ns->rq_reset_mode > 3)
> +		return -EINVAL;
> +
> +	if (ns->rq_reset_mode == 1)
> +		return nsim_create_page_pool(&qmem->pp, &ns->rq[idx]->napi);
> +
> +	qmem->rq = nsim_queue_alloc();
> +	if (!qmem->rq)
> +		return -ENOMEM;
> +
> +	err = nsim_create_page_pool(&qmem->rq->page_pool, &qmem->rq->napi);
> +	if (err)
> +		goto err_free;
> +
> +	if (!ns->rq_reset_mode)
> +		netif_napi_add_config(dev, &qmem->rq->napi, nsim_poll, idx);
> +
> +	return 0;
> +
> +err_free:
> +	nsim_queue_free(qmem->rq);
> +	return err;
> +}
> +
> +static void nsim_queue_mem_free(struct net_device *dev, void *per_queue_mem)
> +{
> +	struct nsim_queue_mem *qmem = per_queue_mem;
> +	struct netdevsim *ns = netdev_priv(dev);
> +
> +	if (qmem->pp)
> +		page_pool_destroy(qmem->pp);
> +	if (qmem->rq) {
> +		if (!ns->rq_reset_mode)
> +			netif_napi_del(&qmem->rq->napi);
> +		page_pool_destroy(qmem->rq->page_pool);
> +		nsim_queue_free(qmem->rq);
> +	}
> +}
> +
> +static int
> +nsim_queue_start(struct net_device *dev, void *per_queue_mem, int idx)
> +{
> +	struct nsim_queue_mem *qmem = per_queue_mem;
> +	struct netdevsim *ns = netdev_priv(dev);
> +
> +	if (ns->rq_reset_mode == 1) {
> +		ns->rq[idx]->page_pool = qmem->pp;
> +		napi_enable(&ns->rq[idx]->napi);
> +		return 0;
> +	}
> +
> +	/* netif_napi_add()/_del() should normally be called from alloc/free,
> +	 * here we want to test various call orders.
> +	 */
> +	if (ns->rq_reset_mode == 2) {
> +		netif_napi_del(&ns->rq[idx]->napi);
> +		netif_napi_add_config(dev, &qmem->rq->napi, nsim_poll, idx);
> +	} else if (ns->rq_reset_mode == 3) {
> +		netif_napi_add_config(dev, &qmem->rq->napi, nsim_poll, idx);
> +		netif_napi_del(&ns->rq[idx]->napi);

Just to make sure my understanding: this is expected to not change
anything, due to test_and_(set|clear)_bit(NAPI_STATE_LISTED, ..),
right?