netdev - Re: [PATCH v1 1/2] netdevsim: add NAPI 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: <20240416172730.1b588eef@kernel.org>
Date: Tue, 16 Apr 2024 17:27:30 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: David Wei <dw@...idwei.uk>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH v1 1/2] netdevsim: add NAPI support

On Mon, 15 Apr 2024 22:15:26 -0700 David Wei wrote:
> Add NAPI support to netdevim, similar to veth.
> 
> * Add a nsim_rq rx queue structure to hold a NAPI instance and a skb
>   queue.
> * During xmit, store the skb in the peer skb queue and schedule NAPI.
> * During napi_poll(), drain the skb queue and pass up the stack.
> * Add assoc between rxq and NAPI instance using netif_queue_set_napi().

> +#define NSIM_RING_SIZE		256
> +
> +static int nsim_napi_rx(struct nsim_rq *rq, struct sk_buff *skb)
> +{
> +	if (list_count_nodes(&rq->skb_queue) > NSIM_RING_SIZE) {
> +		dev_kfree_skb_any(skb);
> +		return NET_RX_DROP;
> +	}
> +
> +	list_add_tail(&skb->list, &rq->skb_queue);

Why not use struct sk_buff_head ?
It has a purge helper for freeing, and remembers its own length.

> +static int nsim_poll(struct napi_struct *napi, int budget)
> +{
> +	struct nsim_rq *rq = container_of(napi, struct nsim_rq, napi);
> +	int done;
> +
> +	done = nsim_rcv(rq, budget);
> +
> +	if (done < budget && napi_complete_done(napi, done)) {
> +		if (unlikely(!list_empty(&rq->skb_queue)))
> +			napi_schedule(&rq->napi);

I think you can drop the re-check, NAPI has a built in protection 
for this kind of race.

> +	}
> +
> +	return done;
> +}

>  static int nsim_open(struct net_device *dev)
>  {
>  	struct netdevsim *ns = netdev_priv(dev);
> -	struct page_pool_params pp = { 0 };
> +	int err;
> +
> +	err = nsim_init_napi(ns);
> +	if (err)
> +		return err;
> +
> +	nsim_enable_napi(ns);
>  
> -	pp.pool_size = 128;
> -	pp.dev = &dev->dev;
> -	pp.dma_dir = DMA_BIDIRECTIONAL;
> -	pp.netdev = dev;
> +	netif_carrier_on(dev);

Why the carrier? It's on by default.
Should be a separate patch if needed.

> -	ns->pp = page_pool_create(&pp);
> -	return PTR_ERR_OR_ZERO(ns->pp);
> +	return 0;
> +}

> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
> index 7664ab823e29..87bf45ec4dd2 100644
> --- a/drivers/net/netdevsim/netdevsim.h
> +++ b/drivers/net/netdevsim/netdevsim.h
> @@ -90,11 +90,18 @@ struct nsim_ethtool {
>  	struct ethtool_fecparam fec;
>  };
>  
> +struct nsim_rq {
> +	struct napi_struct napi;
> +	struct list_head skb_queue;
> +	struct page_pool *page_pool;

You added the new page_pool pointer but didn't delete the one
I added recently to the device?

> +};
> +
>  struct netdevsim {
>  	struct net_device *netdev;
>  	struct nsim_dev *nsim_dev;
>  	struct nsim_dev_port *nsim_dev_port;
>  	struct mock_phc *phc;
> +	struct nsim_rq *rq;
>  
>  	u64 tx_packets;
>  	u64 tx_bytes;