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] [day] [month] [year] [list]

Message-ID: <89c29a49-0f32-4222-8c71-5317eb8b0d1a@davidwei.uk>
Date: Fri, 19 Apr 2024 09:08:13 -0700
From: David Wei <dw@...idwei.uk>
To: Jakub Kicinski <kuba@...nel.org>
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 2024-04-16 5:27 pm, Jakub Kicinski wrote:
> 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.

Didn't know about sk_buff_head! Thanks, it's much better.

> 
>> +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.

Would veth also want this dropped, or does it serve a different purpose
there?

> 
>> +	}
>> +
>> +	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.

Symmetry, not sure if it was needed or not. Will remove.

> 
>> -	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?

Yeah, sorry that slipped through the rebase.

> 
>> +};
>> +
>>  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;
>