netdev - Re: [net-next PATCH v8 02/10] net: Add queue and napi association

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]
Date: Mon, 20 Nov 2023 15:54:36 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Amritha Nambiar <amritha.nambiar@...el.com>
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 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.

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.

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

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

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

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,
-- 
2.42.0