Message-ID: <68d2b08c-27ae-498e-9ce9-09e88796cd35@intel.com>
Date: Tue, 21 Nov 2023 13:26:27 -0800
From: "Nambiar, Amritha" <amritha.nambiar@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<sridhar.samudrala@...el.com>
Subject: Re: [net-next PATCH v8 02/10] net: Add queue and napi association

On 11/20/2023 3:54 PM, Jakub Kicinski wrote:
> On Thu, 16 Nov 2023 17:16:48 -0800 Amritha Nambiar wrote:
>> Add the napi pointer in netdev queue for tracking the napi
>> instance for each queue. This achieves the queue<->napi mapping.
> 
> I took the series for a spin. I'll send a bnxt patch in a separate
> reply, please add it to the series.
> 

Sure, will add the bnxt patch to v9.

> Three high level comments:
> 
>   - the netif_queue_set_napi() vs __netif_queue_set_napi() gave me pause;
>     developers may be used to calling netif_*() functions from open/stop
>     handlers, without worrying about locking.
>     I think that netif_queue_set_napi() should assume rtnl_lock was
>     already taken, as it will be in 90% of cases. A rare driver which
>     does not hold it should take it locally for now.
> 

Okay, will make these changes in v9.

>   - drivers don't set real_num_*_queues to 0 when they go down,
>     currently. So even tho we don't list any disabled queues when
>     device is UP, we list queues when device is down.
>     I mean:
> 
>     $ ifup eth0
>     $ ethtool -L eth0 combined 4
>     $ ./get-queues my-device
>     ... will list 4 rx and 4 rx queues ...
>     $ ethtool -L eth0 combined 2
>     $ ./get-queues my-device
>     ... will list 2 rx and 2 rx queues ...
>     $ ifdown eth0
>     $ ./get-queues my-device
>     ... will list 2 rx and 2 rx queues ...
>     ... even tho practically speaking there are no active queues ...
> 
>     I think we should skip listing queue and NAPI info of devices which
>     are DOWN. Do you have any use case which would need looking at those?
> 

So, currently, 'ethtool --show-channels' and 'ps -aef | grep napi' would 
list all the queues and NAPIs if the device is DOWN. I think what you 
are pointing at is:
<ifdown and ./get-queues> should show something similar to <ethtool -L 
eth0 combined 0 (0 is not valid... but almost to that effect) and 
./get-queues>.

But, 'ethtool -L' actually deletes the queues vs 'device DOWN' which 
only disables or makes the queues inactive.

Maybe as a follow-up patch, would it be better to have an additional 
parameter called 'state' for 'queues-get' and 'napi-get' that indicates 
the queues or NAPIs as active/inactive. The queue/NAPI state would be 
inherited from the device state. This way we can still list the 
queues/NAPIs when the device is down, set/update parameter 
configurations and then bring UP the device (in case where we stop 
traffic and tune parameters).

Also, if in future, we have the interface to tune parameters per-queue 
without full reset (of all queues or the device itself, as the hardware 
supports this), the 'state' would report this for specific queue as 
active/inactive. Maybe:
'queue-set' can set 'state = active' for a single queue '{"ifindex": 12, 
"id": 0, "type": 0}' and start a queue.

>   - We need a way to detach queues form NAPI. This is sort-of related to
>     the above, maybe its not as crucial once we skip DOWN devices but
>     nonetheless for symmetry we should let the driver clear the NAPI
>     pointer. NAPIs may be allocated dynamically, the queue::napi pointer
>     may get stale.

Okay, will handle this in v9.

> 
> I hacked together the following for my local testing, feel free to fold
> appropriate parts into your patches:
> 

Sure, thank you!

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 1a0603b3529d..2ed7a3aeec40 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -2948,10 +2948,11 @@ static void
>   ice_queue_set_napi(unsigned int queue_index, enum netdev_queue_type type,
>   		   struct napi_struct *napi, bool locked)
>   {
> -	if (locked)
> -		__netif_queue_set_napi(queue_index, type, napi);
> -	else
> -		netif_queue_set_napi(queue_index, type, napi);
> +	if (!locked)
> +		rtnl_lock();
> +	netif_queue_set_napi(napi->dev, queue_index, type, napi);
> +	if (!locked)
> +		rtnl_unlock();
>   }
>   
>   /**
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index dbc4ea74b8d6..e09a039a092a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2644,13 +2644,10 @@ static inline void *netdev_priv(const struct net_device *dev)
>    */
>   #define SET_NETDEV_DEVTYPE(net, devtype)	((net)->dev.type = (devtype))
>   
> -void netif_queue_set_napi(unsigned int queue_index, enum netdev_queue_type type,
> +void netif_queue_set_napi(struct net_device *dev, unsigned int queue_index,
> +			  enum netdev_queue_type type,
>   			  struct napi_struct *napi);
>   
> -void __netif_queue_set_napi(unsigned int queue_index,
> -			    enum netdev_queue_type type,
> -			    struct napi_struct *napi);
> -
>   static inline void netif_napi_set_irq(struct napi_struct *napi, int irq)
>   {
>   	napi->irq = irq;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 99ca59e18abf..bb93240c69b9 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6400,25 +6400,27 @@ int dev_set_threaded(struct net_device *dev, bool threaded)
>   EXPORT_SYMBOL(dev_set_threaded);
>   
>   /**
> - * __netif_queue_set_napi - Associate queue with the napi
> + * netif_queue_set_napi - Associate queue with the napi
> + * @dev: device to which NAPI and queue belong
>    * @queue_index: Index of queue
>    * @type: queue type as RX or TX
> - * @napi: NAPI context
> + * @napi: NAPI context, pass NULL to clear previously set NAPI
>    *
>    * Set queue with its corresponding napi context. This should be done after
>    * registering the NAPI handler for the queue-vector and the queues have been
>    * mapped to the corresponding interrupt vector.
>    */
> -void __netif_queue_set_napi(unsigned int queue_index,
> -			    enum netdev_queue_type type,
> -			    struct napi_struct *napi)
> +void netif_queue_set_napi(struct net_device *dev, unsigned int queue_index,
> +			  enum netdev_queue_type type,
> +			  struct napi_struct *napi)
>   {
> -	struct net_device *dev = napi->dev;
>   	struct netdev_rx_queue *rxq;
>   	struct netdev_queue *txq;
>   
> -	if (WARN_ON_ONCE(!dev))
> +	if (WARN_ON_ONCE(napi && !napi->dev))
>   		return;
> +	if (dev->reg_state >= NETREG_REGISTERED)
> +		ASSERT_RTNL();
>   
>   	switch (type) {
>   	case NETDEV_QUEUE_TYPE_RX:
> @@ -6433,15 +6435,6 @@ void __netif_queue_set_napi(unsigned int queue_index,
>   		return;
>   	}
>   }
> -EXPORT_SYMBOL(__netif_queue_set_napi);
> -
> -void netif_queue_set_napi(unsigned int queue_index, enum netdev_queue_type type,
> -			  struct napi_struct *napi)
> -{
> -	rtnl_lock();
> -	__netif_queue_set_napi(queue_index, type, napi);
> -	rtnl_unlock();
> -}
>   EXPORT_SYMBOL(netif_queue_set_napi);
>   
>   void netif_napi_add_weight(struct net_device *dev, struct napi_struct *napi,

Powered by blists - more mailing lists