[PATCH net-next] net: run netdev work under the ops-compat lock

[PATCH net-next] net: run netdev work under the ops-compat lock

[Jakub Kicinski]

netdev_work_proc() took rtnl_lock() for the whole batch, forcing
even ops-locked drivers onto rtnl just to run their rx_mode / ndo_work
callbacks. Take the per-device ops-compat lock instead: the instance
lock for ops-locked drivers, rtnl_lock for the rest. That's what the
callbacks already assert (e.g. netif_rx_mode_run() asserts
netdev_assert_locked_ops_compat()), and non-ops-locked work
such as vlan ndo_work keeps rtnl, since its compat lock resolves
to rtnl anyway.

Without this adding more uses of ndo_work, for cases which don't
need rtnl_lock feels like a step back.

We can drop the comment about the extra ref now, with the relocking
it's a necessity.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: corbet@lwn.net
CC: skhan@linuxfoundation.org
CC: rdunlap@infradead.org
CC: sdf@fomichev.me
CC: kuniyu@google.com
CC: linux-doc@vger.kernel.org
---
 Documentation/networking/netdevices.rst | 14 ++++++++++----
 include/linux/netdevice.h               |  5 ++++-
 net/core/netdev_work.c                  | 15 ++-------------
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
index db71d4283032..aac33497813f 100644
--- a/Documentation/networking/netdevices.rst
+++ b/Documentation/networking/netdevices.rst
@@ -292,15 +292,21 @@ struct net_device synchronization rules
 	in process context.
 
 ndo_set_rx_mode_async:
-	Synchronization: rtnl_lock() semaphore. In addition, netdev instance
-	lock if the driver implements queue management or shaper API.
+	Synchronization: netdev instance lock for "ops locked" drivers,
+	rtnl_lock() semaphore for all other drivers.
 	Context: process (from a work queue)
 	Notes: Async version of ndo_set_rx_mode which runs in process
 	context. Receives snapshots of the unicast and multicast address lists.
 
 ndo_change_rx_flags:
-	Synchronization: rtnl_lock() semaphore. In addition, netdev instance
-	lock if the driver implements queue management or shaper API.
+	Synchronization: netdev instance lock for "ops locked" drivers,
+	rtnl_lock() semaphore for all other drivers.
+
+ndo_work:
+	Synchronization: netdev instance lock for "ops locked" drivers,
+	rtnl_lock() semaphore for all other drivers.
+	Context: process (from a work queue)
+	Notes: Runs deferred work scheduled with netdev_work_sched().
 
 ndo_setup_tc:
 	Locking depends on ``tc_setup_type``. For most types the callback
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8454646d6a45..17b8fb8e94fe 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1135,13 +1135,16 @@ struct netdev_net_notifier {
  *				struct netdev_hw_addr_list *uc,
  *				struct netdev_hw_addr_list *mc);
  *	Async version of ndo_set_rx_mode which runs in process context
- *	with rtnl_lock and netdev_lock_ops(dev) held. The uc/mc parameters
+ *	under the netdev instance lock for "ops locked" drivers, or
+ *	rtnl_lock for all other drivers. The uc/mc parameters
  *	are snapshots of the address lists - iterate with
  *	netdev_hw_addr_list_for_each(ha, uc). Return 0 on success or a
  *	negative errno to request a retry via the core backoff.
  *
  * void (*ndo_work)(struct net_device *dev, unsigned long events);
  *	Run deferred work scheduled with netdev_work_sched(@events).
+ *	Runs in process context under the netdev instance lock for "ops
+ *	locked" drivers, or rtnl_lock for all other drivers.
  *
  * int (*ndo_set_mac_address)(struct net_device *dev, void *addr);
  *	This function  is called when the Media Access Control address
diff --git a/net/core/netdev_work.c b/net/core/netdev_work.c
index e721a06d58df..c99967907e18 100644
--- a/net/core/netdev_work.c
+++ b/net/core/netdev_work.c
@@ -3,7 +3,6 @@
 #include <linux/export.h>
 #include <linux/list.h>
 #include <linux/netdevice.h>
-#include <linux/rtnetlink.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 #include <net/netdev_lock.h>
@@ -129,8 +128,6 @@ static void netdev_work_run(struct net_device *dev, unsigned long events,
 
 static void netdev_work_proc(struct work_struct *work)
 {
-	rtnl_lock();
-
 	while (true) {
 		unsigned long events = 0, core = 0;
 		netdevice_tracker tracker;
@@ -143,16 +140,10 @@ static void netdev_work_proc(struct work_struct *work)
 		}
 		dev = list_first_entry(&netdev_work_list, struct net_device,
 				       work_node);
-		/* Take a temporary reference so @dev can't be freed while we
-		 * drop the lock to grab its ops lock; the work reference is
-		 * only released once we claim the work below.
-		 * The re-locking dance is to ensure that ops lock is enough
-		 * to ensure canceling work is not racy with dequeue.
-		 */
 		netdev_hold(dev, &tracker, GFP_ATOMIC);
 		spin_unlock_bh(&netdev_work_lock);
 
-		netdev_lock_ops(dev);
+		netdev_lock_ops_compat(dev);
 		spin_lock_bh(&netdev_work_lock);
 		if (!list_empty(&dev->work_node)) {
 			list_del_init(&dev->work_node);
@@ -169,10 +160,8 @@ static void netdev_work_proc(struct work_struct *work)
 		spin_unlock_bh(&netdev_work_lock);
 
 		netdev_work_run(dev, events, core);
-		netdev_unlock_ops(dev);
+		netdev_unlock_ops_compat(dev);
 
 		netdev_put(dev, &tracker);
 	}
-
-	rtnl_unlock();
 }
-- 
2.55.0