[PATCH v3 net-next 00/12] Make .ndo_get_stats64 sleepable

[PATCH v3 net-next 00/12] Make .ndo_get_stats64 sleepable

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Changes in v3:
- Resolved some memory leak issues in the bonding patch 10/12.

Changes in v2:
- Addressed the recursion issues in .ndo_get_stats64 from bonding and
  net_failover.
- Renamed netdev_lists_lock to netif_lists_lock
- Stopped taking netif_lists_lock from drivers as much as possible.

This series converts all callers of dev_get_stats() to be in sleepable
context, so that we can do more work in the .ndo_get_stats64 method.

The situation today is that if we have hardware that needs to be
accessed through a slow bus like SPI, or through a firmware, we cannot
do that directly in .ndo_get_stats64, so we have to poll counters
periodically and return a cached (not up to date) copy in the atomic NDO
callback. This is undesirable on both ends: more work than strictly
needed is being done, and the end result is also worse (not guaranteed
to be up to date). So converting the code paths to be compatible with
sleeping seems to make more sense.

Vladimir Oltean (12):
  net: mark dev_base_lock for deprecation
  net: introduce a mutex for the netns interface lists
  net: procfs: hold netif_lists_lock when retrieving device statistics
  net: sysfs: don't hold dev_base_lock while retrieving device
    statistics
  s390/appldata_net_sum: hold the netdev lists lock when retrieving
    device statistics
  parisc/led: reindent the code that gathers device statistics
  parisc/led: hold the netdev lists lock when retrieving device
    statistics
  net: make dev_get_stats return void
  net: net_failover: ensure .ndo_get_stats64 can sleep
  net: bonding: ensure .ndo_get_stats64 can sleep
  net: mark ndo_get_stats64 as being able to sleep
  net: remove obsolete comments about ndo_get_stats64 context from eth
    drivers

 Documentation/networking/netdevices.rst       |   8 +-
 Documentation/networking/statistics.rst       |   9 +-
 arch/s390/appldata/appldata_net_sum.c         |  33 ++---
 drivers/leds/trigger/ledtrig-netdev.c         |   9 +-
 drivers/net/bonding/bond_main.c               | 119 +++++++++---------
 drivers/net/ethernet/cisco/enic/enic_main.c   |   1 -
 .../net/ethernet/hisilicon/hns/hns_ethtool.c  |  51 ++++----
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |   7 +-
 drivers/net/ethernet/intel/ixgbevf/ethtool.c  |   7 +-
 drivers/net/ethernet/nvidia/forcedeth.c       |   2 -
 drivers/net/ethernet/sfc/efx_common.c         |   1 -
 drivers/net/ethernet/sfc/falcon/efx.c         |   1 -
 drivers/net/net_failover.c                    |  63 +++++++---
 drivers/parisc/led.c                          |  37 +++---
 drivers/scsi/fcoe/fcoe_transport.c            |   6 +-
 drivers/usb/gadget/function/rndis.c           |  45 +++----
 include/linux/netdevice.h                     |  13 +-
 include/net/bonding.h                         |  49 +++++++-
 include/net/net_failover.h                    |   9 +-
 include/net/net_namespace.h                   |   6 +
 net/8021q/vlanproc.c                          |  15 ++-
 net/core/dev.c                                |  69 ++++++----
 net/core/net-procfs.c                         |  48 +++----
 net/core/net-sysfs.c                          |  10 +-
 net/openvswitch/vport.c                       |  25 ++--
 25 files changed, 367 insertions(+), 276 deletions(-)

-- 
2.25.1

[PATCH v3 net-next 01/12] net: mark dev_base_lock for deprecation

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

There is a movement to eliminate the usage of dev_base_lock, which
exists since as far as I could track the kernel history down (the
"7a2deb329241 Import changeset" commit from the bitkeeper branch).

The dev_base_lock approach has multiple issues:
- It is global and not per netns.
- Its meaning has mutated over the years and the vast majority of
  current users is using it to protect against changes of network device
  state, as opposed to changes of the interface list.
- It is overlapping with other protection mechanisms introduced in the
  meantime, which have higher performance for readers, like RCU
  introduced in 2009 by Eric Dumazet for kernel 2.6.

The old comment that I just deleted made this distinction: writers
protect only against readers by holding dev_base_lock, whereas they need
to protect against other writers by holding the RTNL mutex. This is
slightly confusing/incorrect, since a rwlock_t cannot have more than one
concurrently running writer, so that explanation does not justify why
the RTNL mutex would be necessary.

Instead, Stephen Hemminger makes this clarification here:
https://lore.kernel.org/netdev/20201129211230.4d704931@hermes.local/T/#u

| There are really two different domains being covered by locks here.
|
| The first area is change of state of network devices. This has traditionally
| been covered by RTNL because there are places that depend on coordinating
| state between multiple devices. RTNL is too big and held too long but getting
| rid of it is hard because there are corner cases (like state changes from userspace
| for VPN devices).
|
| The other area is code that wants to do read access to look at list of devices.
| These pure readers can/should be converted to RCU by now. Writers should hold RTNL.

This patch edits the comment for dev_base_lock, minimizing its role in
the protection of network interface lists, and clarifies that it is has
other purposes as well.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
None.

 net/core/dev.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 8fa739259041..67d912745e5c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -169,23 +169,22 @@ static int call_netdevice_notifiers_extack(unsigned long val,
 static struct napi_struct *napi_by_id(unsigned int napi_id);
 
 /*
- * The @dev_base_head list is protected by @dev_base_lock and the rtnl
- * semaphore.
- *
- * Pure readers hold dev_base_lock for reading, or rcu_read_lock()
- *
- * Writers must hold the rtnl semaphore while they loop through the
- * dev_base_head list, and hold dev_base_lock for writing when they do the
- * actual updates.  This allows pure readers to access the list even
- * while a writer is preparing to update it.
- *
- * To put it another way, dev_base_lock is held for writing only to
- * protect against pure readers; the rtnl semaphore provides the
- * protection against other writers.
- *
- * See, for example usages, register_netdevice() and
- * unregister_netdevice(), which must be called with the rtnl
- * semaphore held.
+ * The network interface list of a netns (@net->dev_base_head) and the hash
+ * tables per ifindex (@net->dev_index_head) and per name (@net->dev_name_head)
+ * are protected using the following rules:
+ *
+ * Pure readers should hold rcu_read_lock() which should protect them against
+ * concurrent changes to the interface lists made by the writers. Pure writers
+ * must serialize by holding the RTNL mutex while they loop through the list
+ * and make changes to it.
+ *
+ * It is also possible to hold the global rwlock_t @dev_base_lock for
+ * protection (holding its read side as an alternative to rcu_read_lock, and
+ * its write side as an alternative to the RTNL mutex), however this should not
+ * be done in new code, since it is deprecated and pending removal.
+ *
+ * One other role of @dev_base_lock is to protect against changes in the
+ * operational state of a network interface.
  */
 DEFINE_RWLOCK(dev_base_lock);
 EXPORT_SYMBOL(dev_base_lock);
-- 
2.25.1

[PATCH v3 net-next 02/12] net: introduce a mutex for the netns interface lists

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Currently, any writer that wants to alter the lists of network
interfaces (either the plain list net->dev_base_head, or the hash tables
net->dev_index_head and net->dev_name_head) can keep other writers at
bay using the RTNL mutex.

However, the RTNL mutex has become a very contended resource over the
years, so there is a movement to do finer grained locking.

This patch adds one more way for writers to the network interface lists
to serialize themselves. We assume that all writers to the network
interface lists are easily identifiable because the write side of
dev_base_lock also needs to be held (note that some instances of that
were deliberately skipped, since they only dealt with protecting the
operational state of the netdev).

Holding the RTNL mutex is now optional for new code that alters the
lists, since all relevant writers were made to also hold the new lock.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
- Renamed from netdev_lists_lock to netif_lists_lock.
- Created some helper functions netif_lists_lock() and
  netif_lists_unlock().

 include/linux/netdevice.h   | 10 +++++++++
 include/net/net_namespace.h |  6 +++++
 net/core/dev.c              | 44 +++++++++++++++++++++++++------------
 3 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 259be67644e3..2cc4800238bd 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4393,6 +4393,16 @@ static inline void netif_addr_unlock_bh(struct net_device *dev)
 	spin_unlock_bh(&dev->addr_list_lock);
 }
 
+static inline void netif_lists_lock(struct net *net)
+{
+	mutex_lock(&net->netif_lists_lock);
+}
+
+static inline void netif_lists_unlock(struct net *net)
+{
+	mutex_unlock(&net->netif_lists_lock);
+}
+
 /*
  * dev_addrs walker. Should be used only for read access. Call with
  * rcu_read_lock held.
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 29567875f428..cac64c3c7ce0 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -183,6 +183,12 @@ struct net {
 	struct sock		*crypto_nlsk;
 #endif
 	struct sock		*diag_nlsk;
+
+	/* Serializes writers to @dev_base_head, @dev_name_head and
+	 * @dev_index_head. It does _not_ protect the netif adjacency lists
+	 * (struct net_device::adj_list).
+	 */
+	struct mutex		netif_lists_lock;
 } __randomize_layout;
 
 #include <linux/seq_file_net.h>
diff --git a/net/core/dev.c b/net/core/dev.c
index 67d912745e5c..49963aaf12b5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -175,13 +175,16 @@ static struct napi_struct *napi_by_id(unsigned int napi_id);
  *
  * Pure readers should hold rcu_read_lock() which should protect them against
  * concurrent changes to the interface lists made by the writers. Pure writers
- * must serialize by holding the RTNL mutex while they loop through the list
- * and make changes to it.
+ * must serialize by holding the @net->netif_lists_lock mutex while they loop
+ * through the list and make changes to it.
+ *
+ * It is possible to hold the RTNL mutex for serializing the writers too, but
+ * this should be avoided in new code due to lock contention.
  *
  * It is also possible to hold the global rwlock_t @dev_base_lock for
  * protection (holding its read side as an alternative to rcu_read_lock, and
- * its write side as an alternative to the RTNL mutex), however this should not
- * be done in new code, since it is deprecated and pending removal.
+ * its write side as an alternative to @net->netif_lists_lock), however this
+ * should not be done in new code, since it is deprecated and pending removal.
  *
  * One other role of @dev_base_lock is to protect against changes in the
  * operational state of a network interface.
@@ -360,12 +363,14 @@ static void list_netdevice(struct net_device *dev)
 
 	ASSERT_RTNL();
 
+	netif_lists_lock(net);
 	write_lock_bh(&dev_base_lock);
 	list_add_tail_rcu(&dev->dev_list, &net->dev_base_head);
 	netdev_name_node_add(net, dev->name_node);
 	hlist_add_head_rcu(&dev->index_hlist,
 			   dev_index_hash(net, dev->ifindex));
 	write_unlock_bh(&dev_base_lock);
+	netif_lists_unlock(net);
 
 	dev_base_seq_inc(net);
 }
@@ -375,16 +380,20 @@ static void list_netdevice(struct net_device *dev)
  */
 static void unlist_netdevice(struct net_device *dev)
 {
+	struct net *net = dev_net(dev);
+
 	ASSERT_RTNL();
 
 	/* Unlink dev from the device chain */
+	netif_lists_lock(net);
 	write_lock_bh(&dev_base_lock);
 	list_del_rcu(&dev->dev_list);
 	netdev_name_node_del(dev->name_node);
 	hlist_del_rcu(&dev->index_hlist);
 	write_unlock_bh(&dev_base_lock);
+	netif_lists_unlock(net);
 
-	dev_base_seq_inc(dev_net(dev));
+	dev_base_seq_inc(net);
 }
 
 /*
@@ -850,11 +859,11 @@ EXPORT_SYMBOL_GPL(dev_fill_metadata_dst);
  *	@net: the applicable net namespace
  *	@name: name to find
  *
- *	Find an interface by name. Must be called under RTNL semaphore
- *	or @dev_base_lock. If the name is found a pointer to the device
- *	is returned. If the name is not found then %NULL is returned. The
- *	reference counters are not incremented so the caller must be
- *	careful with locks.
+ *	Find an interface by name. Must be called under RTNL semaphore,
+ *	@net->netif_lists_lock or @dev_base_lock. If the name is found,
+ *	a pointer to the device is returned. If the name is not found then
+ *	%NULL is returned. The reference counters are not incremented so the
+ *	caller must be careful with locks.
  */
 
 struct net_device *__dev_get_by_name(struct net *net, const char *name)
@@ -920,8 +929,8 @@ EXPORT_SYMBOL(dev_get_by_name);
  *	Search for an interface by index. Returns %NULL if the device
  *	is not found or a pointer to the device. The device has not
  *	had its reference counter increased so the caller must be careful
- *	about locking. The caller must hold either the RTNL semaphore
- *	or @dev_base_lock.
+ *	about locking. The caller must hold either the RTNL semaphore,
+ *	@net->netif_lists_lock or @dev_base_lock.
  */
 
 struct net_device *__dev_get_by_index(struct net *net, int ifindex)
@@ -1330,15 +1339,19 @@ int dev_change_name(struct net_device *dev, const char *newname)
 
 	netdev_adjacent_rename_links(dev, oldname);
 
+	netif_lists_lock(net);
 	write_lock_bh(&dev_base_lock);
 	netdev_name_node_del(dev->name_node);
 	write_unlock_bh(&dev_base_lock);
+	netif_lists_unlock(net);
 
 	synchronize_rcu();
 
+	netif_lists_lock(net);
 	write_lock_bh(&dev_base_lock);
 	netdev_name_node_add(net, dev->name_node);
 	write_unlock_bh(&dev_base_lock);
+	netif_lists_unlock(net);
 
 	ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
 	ret = notifier_to_errno(ret);
@@ -9415,8 +9428,9 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
  *	@net: the applicable net namespace
  *
  *	Returns a suitable unique value for a new device interface
- *	number.  The caller must hold the rtnl semaphore or the
- *	dev_base_lock to be sure it remains unique.
+ *	number.
+ *	The caller must hold the rtnl semaphore, @net->netif_lists_lock or the
+ *	@dev_base_lock to be sure it remains unique.
  */
 static int dev_new_index(struct net *net)
 {
@@ -10999,6 +11013,8 @@ static int __net_init netdev_init(struct net *net)
 	if (net->dev_index_head == NULL)
 		goto err_idx;
 
+	mutex_init(&net->netif_lists_lock);
+
 	RAW_INIT_NOTIFIER_HEAD(&net->netdev_chain);
 
 	return 0;
-- 
2.25.1

[PATCH v3 net-next 03/12] net: procfs: hold netif_lists_lock when retrieving device statistics

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

In the effort of making .ndo_get_stats64 be able to sleep, we need to
ensure the callers of dev_get_stats do not use atomic context.

The /proc/net/dev file uses an RCU read-side critical section to ensure
the integrity of the list of network interfaces, because it iterates
through all net devices in the netns to show their statistics.

To offer the equivalent protection against an interface registering or
deregistering, while also remaining in sleepable context, we can use the
netns mutex for the interface lists.

Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
None.

 net/core/net-procfs.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
index c714e6a9dad4..4784703c1e39 100644
--- a/net/core/net-procfs.c
+++ b/net/core/net-procfs.c
@@ -21,7 +21,7 @@ static inline struct net_device *dev_from_same_bucket(struct seq_file *seq, loff
 	unsigned int count = 0, offset = get_offset(*pos);
 
 	h = &net->dev_index_head[get_bucket(*pos)];
-	hlist_for_each_entry_rcu(dev, h, index_hlist) {
+	hlist_for_each_entry(dev, h, index_hlist) {
 		if (++count == offset)
 			return dev;
 	}
@@ -51,9 +51,11 @@ static inline struct net_device *dev_from_bucket(struct seq_file *seq, loff_t *p
  *	in detail.
  */
 static void *dev_seq_start(struct seq_file *seq, loff_t *pos)
-	__acquires(RCU)
 {
-	rcu_read_lock();
+	struct net *net = seq_file_net(seq);
+
+	netif_lists_lock(net);
+
 	if (!*pos)
 		return SEQ_START_TOKEN;
 
@@ -70,9 +72,10 @@ static void *dev_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 }
 
 static void dev_seq_stop(struct seq_file *seq, void *v)
-	__releases(RCU)
 {
-	rcu_read_unlock();
+	struct net *net = seq_file_net(seq);
+
+	netif_lists_unlock(net);
 }
 
 static void dev_seq_printf_stats(struct seq_file *seq, struct net_device *dev)
-- 
2.25.1

[PATCH v3 net-next 04/12] net: sysfs: don't hold dev_base_lock while retrieving device statistics

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

In the effort of making .ndo_get_stats64 be able to sleep, we need to
ensure the callers of dev_get_stats do not use atomic context.

I need to preface this by saying that I have no idea why netstat_show
takes the dev_base_lock rwlock. Two things can be observed:
(a) it does not appear to be due to dev_isalive requiring it for some
    reason, because broadcast_show() also calls dev_isalive() and has
    had no problem existing since the beginning of git.
(b) the dev_get_stats function definitely does not need dev_base_lock
    protection either. In fact, holding the dev_base_lock is the entire
    problem here, because we want to make dev_get_stats sleepable, and
    holding a rwlock gives us atomic context.

So since no protection seems to be necessary, just run unlocked while
retrieving the /sys/class/net/eth0/statistics/* values.

Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
None.

 net/core/net-sysfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index daf502c13d6d..8604183678fc 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -585,14 +585,13 @@ static ssize_t netstat_show(const struct device *d,
 	WARN_ON(offset > sizeof(struct rtnl_link_stats64) ||
 		offset % sizeof(u64) != 0);
 
-	read_lock(&dev_base_lock);
 	if (dev_isalive(dev)) {
 		struct rtnl_link_stats64 temp;
 		const struct rtnl_link_stats64 *stats = dev_get_stats(dev, &temp);
 
 		ret = sprintf(buf, fmt_u64, *(u64 *)(((u8 *)stats) + offset));
 	}
-	read_unlock(&dev_base_lock);
+
 	return ret;
 }
 
-- 
2.25.1

[PATCH v3 net-next 05/12] s390/appldata_net_sum: hold the netdev lists lock when retrieving device statistics

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

In the effort of making .ndo_get_stats64 be able to sleep, we need to
ensure the callers of dev_get_stats do not use atomic context.

In the case of the appldata driver, an RCU read-side critical section is
used to ensure the integrity of the list of network interfaces, because
the driver iterates through all net devices in the netns to aggregate
statistics. We still need some protection against an interface
registering or deregistering, and the writer-side lock, the netns's
mutex, is fine for that, because it offers sleepable context.

The ops->callback function is called from under appldata_ops_mutex
protection, so this is proof that the context is sleepable and holding
a mutex is therefore fine.

Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: linux-s390@vger.kernel.org
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
None.

 arch/s390/appldata/appldata_net_sum.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/s390/appldata/appldata_net_sum.c b/arch/s390/appldata/appldata_net_sum.c
index 59c282ca002f..4db886980cba 100644
--- a/arch/s390/appldata/appldata_net_sum.c
+++ b/arch/s390/appldata/appldata_net_sum.c
@@ -78,8 +78,9 @@ static void appldata_get_net_sum_data(void *data)
 	tx_dropped = 0;
 	collisions = 0;
 
-	rcu_read_lock();
-	for_each_netdev_rcu(&init_net, dev) {
+	netif_lists_lock(&init_net);
+
+	for_each_netdev(&init_net, dev) {
 		const struct rtnl_link_stats64 *stats;
 		struct rtnl_link_stats64 temp;
 
@@ -95,7 +96,8 @@ static void appldata_get_net_sum_data(void *data)
 		collisions += stats->collisions;
 		i++;
 	}
-	rcu_read_unlock();
+
+	netif_lists_unlock(&init_net);
 
 	net_data->nr_interfaces = i;
 	net_data->rx_packets = rx_packets;
-- 
2.25.1

[PATCH v3 net-next 06/12] parisc/led: reindent the code that gathers device statistics

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The standard in the Linux kernel is to use one tab character per
indentation level.

Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-parisc@vger.kernel.org
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
None.

 drivers/parisc/led.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
index 36c6613f7a36..3cada632a4be 100644
--- a/drivers/parisc/led.c
+++ b/drivers/parisc/led.c
@@ -359,16 +359,19 @@ static __inline__ int led_get_net_activity(void)
 	/* we are running as a workqueue task, so we can use an RCU lookup */
 	rcu_read_lock();
 	for_each_netdev_rcu(&init_net, dev) {
-	    const struct rtnl_link_stats64 *stats;
-	    struct rtnl_link_stats64 temp;
-	    struct in_device *in_dev = __in_dev_get_rcu(dev);
-	    if (!in_dev || !in_dev->ifa_list)
-		continue;
-	    if (ipv4_is_loopback(in_dev->ifa_list->ifa_local))
-		continue;
-	    stats = dev_get_stats(dev, &temp);
-	    rx_total += stats->rx_packets;
-	    tx_total += stats->tx_packets;
+		const struct rtnl_link_stats64 *stats;
+		struct rtnl_link_stats64 temp;
+		struct in_device *in_dev = __in_dev_get_rcu(dev);
+
+		if (!in_dev || !in_dev->ifa_list)
+			continue;
+
+		if (ipv4_is_loopback(in_dev->ifa_list->ifa_local))
+			continue;
+
+		stats = dev_get_stats(dev, &temp);
+		rx_total += stats->rx_packets;
+		tx_total += stats->tx_packets;
 	}
 	rcu_read_unlock();
 
-- 
2.25.1

[PATCH v3 net-next 07/12] parisc/led: hold the netdev lists lock when retrieving device statistics

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

In the effort of making .ndo_get_stats64 be able to sleep, we need to
ensure the callers of dev_get_stats do not use atomic context.

The LED driver for HP-PARISC workstations uses a workqueue to
periodically check for updates in network interface statistics, and
flicker when those have changed (i.e. there has been activity on the
line). Ignoring the fact that this driver is completely duplicating
drivers/leds/trigger/ledtrig-netdev.c, there is an even bigger problem.
Now, the dev_get_stats call can sleep, and iterating through the list of
network interfaces still needs to ensure the integrity of list of
network interfaces. So that leaves us only one locking option given the
current design of the network stack, and that is the netns mutex.

Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-parisc@vger.kernel.org
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
None.

 drivers/parisc/led.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
index 3cada632a4be..c8c6b2301dc9 100644
--- a/drivers/parisc/led.c
+++ b/drivers/parisc/led.c
@@ -38,7 +38,6 @@
 #include <linux/ctype.h>
 #include <linux/blkdev.h>
 #include <linux/workqueue.h>
-#include <linux/rcupdate.h>
 #include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/hardware.h>
@@ -355,25 +354,29 @@ static __inline__ int led_get_net_activity(void)
 	int retval;
 
 	rx_total = tx_total = 0;
-	
-	/* we are running as a workqueue task, so we can use an RCU lookup */
-	rcu_read_lock();
-	for_each_netdev_rcu(&init_net, dev) {
+
+	/* we are running as a workqueue task, so we can sleep */
+	netif_lists_lock(&init_net);
+
+	for_each_netdev(&init_net, dev) {
+		struct in_device *in_dev = in_dev_get(dev);
 		const struct rtnl_link_stats64 *stats;
 		struct rtnl_link_stats64 temp;
-		struct in_device *in_dev = __in_dev_get_rcu(dev);
 
-		if (!in_dev || !in_dev->ifa_list)
+		if (!in_dev || !in_dev->ifa_list ||
+		    ipv4_is_loopback(in_dev->ifa_list->ifa_local)) {
+			in_dev_put(in_dev);
 			continue;
+		}
 
-		if (ipv4_is_loopback(in_dev->ifa_list->ifa_local))
-			continue;
+		in_dev_put(in_dev);
 
 		stats = dev_get_stats(dev, &temp);
 		rx_total += stats->rx_packets;
 		tx_total += stats->tx_packets;
 	}
-	rcu_read_unlock();
+
+	netif_lists_unlock(&init_net);
 
 	retval = 0;
 
-- 
2.25.1

[PATCH v3 net-next 08/12] net: make dev_get_stats return void

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

After commit 28172739f0a2 ("net: fix 64 bit counters on 32 bit arches"),
dev_get_stats got an additional argument for storage of statistics. At
this point, dev_get_stats could return either the passed "storage"
argument, or the output of .ndo_get_stats64.

Then commit caf586e5f23c ("net: add a core netdev->rx_dropped counter")
came, and the output of .ndo_get_stats64 (still returning a pointer to
struct rtnl_link_stats64) started being ignored.

Then came commit bc1f44709cf2 ("net: make ndo_get_stats64 a void
function") which made .ndo_get_stats64 stop returning anything.

So now, dev_get_stats always reports the "storage" pointer received as
argument. This is useless. Some drivers are dealing with unnecessary
complexity due to this, so refactor them to ignore the return value
completely.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
Patch is new.

 arch/s390/appldata/appldata_net_sum.c         | 25 +++++----
 drivers/leds/trigger/ledtrig-netdev.c         |  9 ++--
 drivers/net/bonding/bond_main.c               |  7 ++-
 .../net/ethernet/hisilicon/hns/hns_ethtool.c  | 51 +++++++++----------
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |  7 ++-
 drivers/net/ethernet/intel/ixgbevf/ethtool.c  |  7 ++-
 drivers/net/net_failover.c                    | 13 +++--
 drivers/parisc/led.c                          |  9 ++--
 drivers/scsi/fcoe/fcoe_transport.c            |  6 +--
 drivers/usb/gadget/function/rndis.c           | 45 ++++++----------
 include/linux/netdevice.h                     |  3 +-
 net/8021q/vlanproc.c                          | 15 +++---
 net/core/dev.c                                |  6 +--
 net/core/net-procfs.c                         | 35 ++++++-------
 net/core/net-sysfs.c                          |  7 +--
 net/openvswitch/vport.c                       | 25 +++++----
 16 files changed, 123 insertions(+), 147 deletions(-)

diff --git a/arch/s390/appldata/appldata_net_sum.c b/arch/s390/appldata/appldata_net_sum.c
index 4db886980cba..6146606ac9a3 100644
--- a/arch/s390/appldata/appldata_net_sum.c
+++ b/arch/s390/appldata/appldata_net_sum.c
@@ -81,19 +81,18 @@ static void appldata_get_net_sum_data(void *data)
 	netif_lists_lock(&init_net);
 
 	for_each_netdev(&init_net, dev) {
-		const struct rtnl_link_stats64 *stats;
-		struct rtnl_link_stats64 temp;
-
-		stats = dev_get_stats(dev, &temp);
-		rx_packets += stats->rx_packets;
-		tx_packets += stats->tx_packets;
-		rx_bytes   += stats->rx_bytes;
-		tx_bytes   += stats->tx_bytes;
-		rx_errors  += stats->rx_errors;
-		tx_errors  += stats->tx_errors;
-		rx_dropped += stats->rx_dropped;
-		tx_dropped += stats->tx_dropped;
-		collisions += stats->collisions;
+		struct rtnl_link_stats64 stats;
+
+		dev_get_stats(dev, &stats);
+		rx_packets += stats.rx_packets;
+		tx_packets += stats.tx_packets;
+		rx_bytes   += stats.rx_bytes;
+		tx_bytes   += stats.tx_bytes;
+		rx_errors  += stats.rx_errors;
+		tx_errors  += stats.tx_errors;
+		rx_dropped += stats.rx_dropped;
+		tx_dropped += stats.tx_dropped;
+		collisions += stats.collisions;
 		i++;
 	}
 
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d83021..4382ee278309 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -347,9 +347,8 @@ static void netdev_trig_work(struct work_struct *work)
 {
 	struct led_netdev_data *trigger_data =
 		container_of(work, struct led_netdev_data, work.work);
-	struct rtnl_link_stats64 *dev_stats;
+	struct rtnl_link_stats64 dev_stats;
 	unsigned int new_activity;
-	struct rtnl_link_stats64 temp;
 	unsigned long interval;
 	int invert;
 
@@ -364,12 +363,12 @@ static void netdev_trig_work(struct work_struct *work)
 	    !test_bit(NETDEV_LED_RX, &trigger_data->mode))
 		return;
 
-	dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
+	dev_get_stats(trigger_data->net_dev, &dev_stats);
 	new_activity =
 	    (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
-		dev_stats->tx_packets : 0) +
+		dev_stats.tx_packets : 0) +
 	    (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
-		dev_stats->rx_packets : 0);
+		dev_stats.rx_packets : 0);
 
 	if (trigger_data->last_activity != new_activity) {
 		led_stop_software_blink(trigger_data->led_cdev);
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5fe5232cc3f3..714aa0e5d041 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3753,13 +3753,12 @@ static void bond_get_stats(struct net_device *bond_dev,
 	memcpy(stats, &bond->bond_stats, sizeof(*stats));
 
 	bond_for_each_slave_rcu(bond, slave, iter) {
-		const struct rtnl_link_stats64 *new =
-			dev_get_stats(slave->dev, &temp);
+		dev_get_stats(slave->dev, &temp);
 
-		bond_fold_stats(stats, new, &slave->slave_stats);
+		bond_fold_stats(stats, &temp, &slave->slave_stats);
 
 		/* save off the slave stats for the next run */
-		memcpy(&slave->slave_stats, new, sizeof(*new));
+		memcpy(&slave->slave_stats, &temp, sizeof(temp));
 	}
 
 	memcpy(&bond->bond_stats, stats, sizeof(*stats));
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index a6e3f07caf99..ee2172011051 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
@@ -839,8 +839,7 @@ static void hns_get_ethtool_stats(struct net_device *netdev,
 	u64 *p = data;
 	struct hns_nic_priv *priv = netdev_priv(netdev);
 	struct hnae_handle *h = priv->ae_handle;
-	const struct rtnl_link_stats64 *net_stats;
-	struct rtnl_link_stats64 temp;
+	struct rtnl_link_stats64 net_stats;
 
 	if (!h->dev->ops->get_stats || !h->dev->ops->update_stats) {
 		netdev_err(netdev, "get_stats or update_stats is null!\n");
@@ -849,32 +848,32 @@ static void hns_get_ethtool_stats(struct net_device *netdev,
 
 	h->dev->ops->update_stats(h, &netdev->stats);
 
-	net_stats = dev_get_stats(netdev, &temp);
+	dev_get_stats(netdev, &net_stats);
 
 	/* get netdev statistics */
-	p[0] = net_stats->rx_packets;
-	p[1] = net_stats->tx_packets;
-	p[2] = net_stats->rx_bytes;
-	p[3] = net_stats->tx_bytes;
-	p[4] = net_stats->rx_errors;
-	p[5] = net_stats->tx_errors;
-	p[6] = net_stats->rx_dropped;
-	p[7] = net_stats->tx_dropped;
-	p[8] = net_stats->multicast;
-	p[9] = net_stats->collisions;
-	p[10] = net_stats->rx_over_errors;
-	p[11] = net_stats->rx_crc_errors;
-	p[12] = net_stats->rx_frame_errors;
-	p[13] = net_stats->rx_fifo_errors;
-	p[14] = net_stats->rx_missed_errors;
-	p[15] = net_stats->tx_aborted_errors;
-	p[16] = net_stats->tx_carrier_errors;
-	p[17] = net_stats->tx_fifo_errors;
-	p[18] = net_stats->tx_heartbeat_errors;
-	p[19] = net_stats->rx_length_errors;
-	p[20] = net_stats->tx_window_errors;
-	p[21] = net_stats->rx_compressed;
-	p[22] = net_stats->tx_compressed;
+	p[0] = net_stats.rx_packets;
+	p[1] = net_stats.tx_packets;
+	p[2] = net_stats.rx_bytes;
+	p[3] = net_stats.tx_bytes;
+	p[4] = net_stats.rx_errors;
+	p[5] = net_stats.tx_errors;
+	p[6] = net_stats.rx_dropped;
+	p[7] = net_stats.tx_dropped;
+	p[8] = net_stats.multicast;
+	p[9] = net_stats.collisions;
+	p[10] = net_stats.rx_over_errors;
+	p[11] = net_stats.rx_crc_errors;
+	p[12] = net_stats.rx_frame_errors;
+	p[13] = net_stats.rx_fifo_errors;
+	p[14] = net_stats.rx_missed_errors;
+	p[15] = net_stats.tx_aborted_errors;
+	p[16] = net_stats.tx_carrier_errors;
+	p[17] = net_stats.tx_fifo_errors;
+	p[18] = net_stats.tx_heartbeat_errors;
+	p[19] = net_stats.rx_length_errors;
+	p[20] = net_stats.tx_window_errors;
+	p[21] = net_stats.rx_compressed;
+	p[22] = net_stats.tx_compressed;
 
 	p[23] = netdev->rx_dropped.counter;
 	p[24] = netdev->tx_dropped.counter;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index a280aa34ca1d..2b8084664403 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1295,19 +1295,18 @@ static void ixgbe_get_ethtool_stats(struct net_device *netdev,
 				    struct ethtool_stats *stats, u64 *data)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	struct rtnl_link_stats64 temp;
-	const struct rtnl_link_stats64 *net_stats;
+	struct rtnl_link_stats64 net_stats;
 	unsigned int start;
 	struct ixgbe_ring *ring;
 	int i, j;
 	char *p = NULL;
 
 	ixgbe_update_stats(adapter);
-	net_stats = dev_get_stats(netdev, &temp);
+	dev_get_stats(netdev, &net_stats);
 	for (i = 0; i < IXGBE_GLOBAL_STATS_LEN; i++) {
 		switch (ixgbe_gstrings_stats[i].type) {
 		case NETDEV_STATS:
-			p = (char *) net_stats +
+			p = (char *) &net_stats +
 					ixgbe_gstrings_stats[i].stat_offset;
 			break;
 		case IXGBE_STATS:
diff --git a/drivers/net/ethernet/intel/ixgbevf/ethtool.c b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
index e49fb1cd9a99..3b9b7e5c2998 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
@@ -420,19 +420,18 @@ static void ixgbevf_get_ethtool_stats(struct net_device *netdev,
 				      struct ethtool_stats *stats, u64 *data)
 {
 	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
-	struct rtnl_link_stats64 temp;
-	const struct rtnl_link_stats64 *net_stats;
+	struct rtnl_link_stats64 net_stats;
 	unsigned int start;
 	struct ixgbevf_ring *ring;
 	int i, j;
 	char *p;
 
 	ixgbevf_update_stats(adapter);
-	net_stats = dev_get_stats(netdev, &temp);
+	dev_get_stats(netdev, &net_stats);
 	for (i = 0; i < IXGBEVF_GLOBAL_STATS_LEN; i++) {
 		switch (ixgbevf_gstrings_stats[i].type) {
 		case NETDEV_STATS:
-			p = (char *)net_stats +
+			p = (char *)&net_stats +
 					ixgbevf_gstrings_stats[i].stat_offset;
 			break;
 		case IXGBEVF_STATS:
diff --git a/drivers/net/net_failover.c b/drivers/net/net_failover.c
index 2a4892402ed8..4f83165412bd 100644
--- a/drivers/net/net_failover.c
+++ b/drivers/net/net_failover.c
@@ -183,7 +183,6 @@ static void net_failover_get_stats(struct net_device *dev,
 				   struct rtnl_link_stats64 *stats)
 {
 	struct net_failover_info *nfo_info = netdev_priv(dev);
-	const struct rtnl_link_stats64 *new;
 	struct rtnl_link_stats64 temp;
 	struct net_device *slave_dev;
 
@@ -194,16 +193,16 @@ static void net_failover_get_stats(struct net_device *dev,
 
 	slave_dev = rcu_dereference(nfo_info->primary_dev);
 	if (slave_dev) {
-		new = dev_get_stats(slave_dev, &temp);
-		net_failover_fold_stats(stats, new, &nfo_info->primary_stats);
-		memcpy(&nfo_info->primary_stats, new, sizeof(*new));
+		dev_get_stats(slave_dev, &temp);
+		net_failover_fold_stats(stats, &temp, &nfo_info->primary_stats);
+		memcpy(&nfo_info->primary_stats, &temp, sizeof(temp));
 	}
 
 	slave_dev = rcu_dereference(nfo_info->standby_dev);
 	if (slave_dev) {
-		new = dev_get_stats(slave_dev, &temp);
-		net_failover_fold_stats(stats, new, &nfo_info->standby_stats);
-		memcpy(&nfo_info->standby_stats, new, sizeof(*new));
+		dev_get_stats(slave_dev, &temp);
+		net_failover_fold_stats(stats, &temp, &nfo_info->standby_stats);
+		memcpy(&nfo_info->standby_stats, &temp, sizeof(temp));
 	}
 
 	rcu_read_unlock();
diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
index c8c6b2301dc9..cc6108785323 100644
--- a/drivers/parisc/led.c
+++ b/drivers/parisc/led.c
@@ -360,8 +360,7 @@ static __inline__ int led_get_net_activity(void)
 
 	for_each_netdev(&init_net, dev) {
 		struct in_device *in_dev = in_dev_get(dev);
-		const struct rtnl_link_stats64 *stats;
-		struct rtnl_link_stats64 temp;
+		struct rtnl_link_stats64 stats;
 
 		if (!in_dev || !in_dev->ifa_list ||
 		    ipv4_is_loopback(in_dev->ifa_list->ifa_local)) {
@@ -371,9 +370,9 @@ static __inline__ int led_get_net_activity(void)
 
 		in_dev_put(in_dev);
 
-		stats = dev_get_stats(dev, &temp);
-		rx_total += stats->rx_packets;
-		tx_total += stats->tx_packets;
+		dev_get_stats(dev, &stats);
+		rx_total += stats.rx_packets;
+		tx_total += stats.tx_packets;
 	}
 
 	netif_lists_unlock(&init_net);
diff --git a/drivers/scsi/fcoe/fcoe_transport.c b/drivers/scsi/fcoe/fcoe_transport.c
index b927b3d84523..f8ba6495e745 100644
--- a/drivers/scsi/fcoe/fcoe_transport.c
+++ b/drivers/scsi/fcoe/fcoe_transport.c
@@ -170,11 +170,11 @@ void __fcoe_get_lesb(struct fc_lport *lport,
 		     struct fc_els_lesb *fc_lesb,
 		     struct net_device *netdev)
 {
+	struct rtnl_link_stats64 stats;
 	unsigned int cpu;
 	u32 lfc, vlfc, mdac;
 	struct fc_stats *stats;
 	struct fcoe_fc_els_lesb *lesb;
-	struct rtnl_link_stats64 temp;
 
 	lfc = 0;
 	vlfc = 0;
@@ -190,8 +190,8 @@ void __fcoe_get_lesb(struct fc_lport *lport,
 	lesb->lesb_link_fail = htonl(lfc);
 	lesb->lesb_vlink_fail = htonl(vlfc);
 	lesb->lesb_miss_fka = htonl(mdac);
-	lesb->lesb_fcs_error =
-			htonl(dev_get_stats(netdev, &temp)->rx_crc_errors);
+	dev_get_stats(netdev, &stats);
+	lesb->lesb_fcs_error = htonl(stats.rx_crc_errors);
 }
 EXPORT_SYMBOL_GPL(__fcoe_get_lesb);
 
diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
index 64de9f1b874c..7ec29e007ae9 100644
--- a/drivers/usb/gadget/function/rndis.c
+++ b/drivers/usb/gadget/function/rndis.c
@@ -169,14 +169,13 @@ static const u32 oid_supported_list[] = {
 static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
 			       unsigned buf_len, rndis_resp_t *r)
 {
+	struct rtnl_link_stats64 stats;
 	int retval = -ENOTSUPP;
 	u32 length = 4;	/* usually */
 	__le32 *outbuf;
 	int i, count;
 	rndis_query_cmplt_type *resp;
 	struct net_device *net;
-	struct rtnl_link_stats64 temp;
-	const struct rtnl_link_stats64 *stats;
 
 	if (!r) return -ENOMEM;
 	resp = (rndis_query_cmplt_type *)r->buf;
@@ -199,7 +198,7 @@ static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
 	resp->InformationBufferOffset = cpu_to_le32(16);
 
 	net = params->dev;
-	stats = dev_get_stats(net, &temp);
+	dev_get_stats(net, &stats);
 
 	switch (OID) {
 
@@ -353,51 +352,41 @@ static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
 	case RNDIS_OID_GEN_XMIT_OK:
 		if (rndis_debug > 1)
 			pr_debug("%s: RNDIS_OID_GEN_XMIT_OK\n", __func__);
-		if (stats) {
-			*outbuf = cpu_to_le32(stats->tx_packets
-				- stats->tx_errors - stats->tx_dropped);
-			retval = 0;
-		}
+		*outbuf = cpu_to_le32(stats.tx_packets - stats.tx_errors -
+				      stats.tx_dropped);
+		retval = 0;
 		break;
 
 	/* mandatory */
 	case RNDIS_OID_GEN_RCV_OK:
 		if (rndis_debug > 1)
 			pr_debug("%s: RNDIS_OID_GEN_RCV_OK\n", __func__);
-		if (stats) {
-			*outbuf = cpu_to_le32(stats->rx_packets
-				- stats->rx_errors - stats->rx_dropped);
-			retval = 0;
-		}
+		*outbuf = cpu_to_le32(stats.rx_packets - stats.rx_errors -
+				      stats.rx_dropped);
+		retval = 0;
 		break;
 
 	/* mandatory */
 	case RNDIS_OID_GEN_XMIT_ERROR:
 		if (rndis_debug > 1)
 			pr_debug("%s: RNDIS_OID_GEN_XMIT_ERROR\n", __func__);
-		if (stats) {
-			*outbuf = cpu_to_le32(stats->tx_errors);
-			retval = 0;
-		}
+		*outbuf = cpu_to_le32(stats.tx_errors);
+		retval = 0;
 		break;
 
 	/* mandatory */
 	case RNDIS_OID_GEN_RCV_ERROR:
 		if (rndis_debug > 1)
 			pr_debug("%s: RNDIS_OID_GEN_RCV_ERROR\n", __func__);
-		if (stats) {
-			*outbuf = cpu_to_le32(stats->rx_errors);
-			retval = 0;
-		}
+		*outbuf = cpu_to_le32(stats.rx_errors);
+		retval = 0;
 		break;
 
 	/* mandatory */
 	case RNDIS_OID_GEN_RCV_NO_BUFFER:
 		pr_debug("%s: RNDIS_OID_GEN_RCV_NO_BUFFER\n", __func__);
-		if (stats) {
-			*outbuf = cpu_to_le32(stats->rx_dropped);
-			retval = 0;
-		}
+		*outbuf = cpu_to_le32(stats.rx_dropped);
+		retval = 0;
 		break;
 
 	/* ieee802.3 OIDs (table 4-3) */
@@ -449,10 +438,8 @@ static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
 	/* mandatory */
 	case RNDIS_OID_802_3_RCV_ERROR_ALIGNMENT:
 		pr_debug("%s: RNDIS_OID_802_3_RCV_ERROR_ALIGNMENT\n", __func__);
-		if (stats) {
-			*outbuf = cpu_to_le32(stats->rx_frame_errors);
-			retval = 0;
-		}
+		*outbuf = cpu_to_le32(stats.rx_frame_errors);
+		retval = 0;
 		break;
 
 	/* mandatory */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2cc4800238bd..7075a6e94486 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4562,8 +4562,7 @@ void netdev_notify_peers(struct net_device *dev);
 void netdev_features_change(struct net_device *dev);
 /* Load a device via the kmod */
 void dev_load(struct net *net, const char *name);
-struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
-					struct rtnl_link_stats64 *storage);
+void dev_get_stats(struct net_device *dev, struct rtnl_link_stats64 *storage);
 void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
 			     const struct net_device_stats *netdev_stats);
 void dev_fetch_sw_netstats(struct rtnl_link_stats64 *s,
diff --git a/net/8021q/vlanproc.c b/net/8021q/vlanproc.c
index ec87dea23719..3a6682d79630 100644
--- a/net/8021q/vlanproc.c
+++ b/net/8021q/vlanproc.c
@@ -242,26 +242,25 @@ static int vlandev_seq_show(struct seq_file *seq, void *offset)
 {
 	struct net_device *vlandev = (struct net_device *) seq->private;
 	const struct vlan_dev_priv *vlan = vlan_dev_priv(vlandev);
-	struct rtnl_link_stats64 temp;
-	const struct rtnl_link_stats64 *stats;
 	static const char fmt64[] = "%30s %12llu\n";
+	struct rtnl_link_stats64 stats;
 	int i;
 
 	if (!is_vlan_dev(vlandev))
 		return 0;
 
-	stats = dev_get_stats(vlandev, &temp);
+	dev_get_stats(vlandev, &stats);
 	seq_printf(seq,
 		   "%s  VID: %d	 REORDER_HDR: %i  dev->priv_flags: %hx\n",
 		   vlandev->name, vlan->vlan_id,
 		   (int)(vlan->flags & 1), vlandev->priv_flags);
 
-	seq_printf(seq, fmt64, "total frames received", stats->rx_packets);
-	seq_printf(seq, fmt64, "total bytes received", stats->rx_bytes);
-	seq_printf(seq, fmt64, "Broadcast/Multicast Rcvd", stats->multicast);
+	seq_printf(seq, fmt64, "total frames received", stats.rx_packets);
+	seq_printf(seq, fmt64, "total bytes received", stats.rx_bytes);
+	seq_printf(seq, fmt64, "Broadcast/Multicast Rcvd", stats.multicast);
 	seq_puts(seq, "\n");
-	seq_printf(seq, fmt64, "total frames transmitted", stats->tx_packets);
-	seq_printf(seq, fmt64, "total bytes transmitted", stats->tx_bytes);
+	seq_printf(seq, fmt64, "total frames transmitted", stats.tx_packets);
+	seq_printf(seq, fmt64, "total bytes transmitted", stats.tx_bytes);
 	seq_printf(seq, "Device: %s", vlan->real_dev->name);
 	/* now show all PRIORITY mappings relating to this VLAN */
 	seq_printf(seq, "\nINGRESS priority mappings: "
diff --git a/net/core/dev.c b/net/core/dev.c
index 49963aaf12b5..93618300ac90 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10398,13 +10398,12 @@ EXPORT_SYMBOL(netdev_stats_to_stats64);
  *	@dev: device to get statistics from
  *	@storage: place to store stats
  *
- *	Get network statistics from device. Return @storage.
+ *	Get network statistics from device.
  *	The device driver may provide its own method by setting
  *	dev->netdev_ops->get_stats64 or dev->netdev_ops->get_stats;
  *	otherwise the internal statistics structure is used.
  */
-struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
-					struct rtnl_link_stats64 *storage)
+void dev_get_stats(struct net_device *dev, struct rtnl_link_stats64 *storage)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 
@@ -10419,7 +10418,6 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 	storage->rx_dropped += (unsigned long)atomic_long_read(&dev->rx_dropped);
 	storage->tx_dropped += (unsigned long)atomic_long_read(&dev->tx_dropped);
 	storage->rx_nohandler += (unsigned long)atomic_long_read(&dev->rx_nohandler);
-	return storage;
 }
 EXPORT_SYMBOL(dev_get_stats);
 
diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
index 4784703c1e39..64666ba7ccab 100644
--- a/net/core/net-procfs.c
+++ b/net/core/net-procfs.c
@@ -80,26 +80,27 @@ static void dev_seq_stop(struct seq_file *seq, void *v)
 
 static void dev_seq_printf_stats(struct seq_file *seq, struct net_device *dev)
 {
-	struct rtnl_link_stats64 temp;
-	const struct rtnl_link_stats64 *stats = dev_get_stats(dev, &temp);
+	struct rtnl_link_stats64 stats;
+
+	dev_get_stats(dev, &stats);
 
 	seq_printf(seq, "%6s: %7llu %7llu %4llu %4llu %4llu %5llu %10llu %9llu "
 		   "%8llu %7llu %4llu %4llu %4llu %5llu %7llu %10llu\n",
-		   dev->name, stats->rx_bytes, stats->rx_packets,
-		   stats->rx_errors,
-		   stats->rx_dropped + stats->rx_missed_errors,
-		   stats->rx_fifo_errors,
-		   stats->rx_length_errors + stats->rx_over_errors +
-		    stats->rx_crc_errors + stats->rx_frame_errors,
-		   stats->rx_compressed, stats->multicast,
-		   stats->tx_bytes, stats->tx_packets,
-		   stats->tx_errors, stats->tx_dropped,
-		   stats->tx_fifo_errors, stats->collisions,
-		   stats->tx_carrier_errors +
-		    stats->tx_aborted_errors +
-		    stats->tx_window_errors +
-		    stats->tx_heartbeat_errors,
-		   stats->tx_compressed);
+		   dev->name, stats.rx_bytes, stats.rx_packets,
+		   stats.rx_errors,
+		   stats.rx_dropped + stats.rx_missed_errors,
+		   stats.rx_fifo_errors,
+		   stats.rx_length_errors + stats.rx_over_errors +
+		    stats.rx_crc_errors + stats.rx_frame_errors,
+		   stats.rx_compressed, stats.multicast,
+		   stats.tx_bytes, stats.tx_packets,
+		   stats.tx_errors, stats.tx_dropped,
+		   stats.tx_fifo_errors, stats.collisions,
+		   stats.tx_carrier_errors +
+		    stats.tx_aborted_errors +
+		    stats.tx_window_errors +
+		    stats.tx_heartbeat_errors,
+		   stats.tx_compressed);
 }
 
 /*
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 8604183678fc..5d89c85b42d4 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -586,10 +586,11 @@ static ssize_t netstat_show(const struct device *d,
 		offset % sizeof(u64) != 0);
 
 	if (dev_isalive(dev)) {
-		struct rtnl_link_stats64 temp;
-		const struct rtnl_link_stats64 *stats = dev_get_stats(dev, &temp);
+		struct rtnl_link_stats64 stats;
 
-		ret = sprintf(buf, fmt_u64, *(u64 *)(((u8 *)stats) + offset));
+		dev_get_stats(dev, &stats);
+
+		ret = sprintf(buf, fmt_u64, *(u64 *)(((u8 *)&stats) + offset));
 	}
 
 	return ret;
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index 4ed7e52c7012..215a818bf9ce 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -269,19 +269,18 @@ void ovs_vport_del(struct vport *vport)
  */
 void ovs_vport_get_stats(struct vport *vport, struct ovs_vport_stats *stats)
 {
-	const struct rtnl_link_stats64 *dev_stats;
-	struct rtnl_link_stats64 temp;
-
-	dev_stats = dev_get_stats(vport->dev, &temp);
-	stats->rx_errors  = dev_stats->rx_errors;
-	stats->tx_errors  = dev_stats->tx_errors;
-	stats->tx_dropped = dev_stats->tx_dropped;
-	stats->rx_dropped = dev_stats->rx_dropped;
-
-	stats->rx_bytes	  = dev_stats->rx_bytes;
-	stats->rx_packets = dev_stats->rx_packets;
-	stats->tx_bytes	  = dev_stats->tx_bytes;
-	stats->tx_packets = dev_stats->tx_packets;
+	struct rtnl_link_stats64 dev_stats;
+
+	dev_get_stats(vport->dev, &dev_stats);
+	stats->rx_errors  = dev_stats.rx_errors;
+	stats->tx_errors  = dev_stats.tx_errors;
+	stats->tx_dropped = dev_stats.tx_dropped;
+	stats->rx_dropped = dev_stats.rx_dropped;
+
+	stats->rx_bytes	  = dev_stats.rx_bytes;
+	stats->rx_packets = dev_stats.rx_packets;
+	stats->tx_bytes	  = dev_stats.tx_bytes;
+	stats->tx_packets = dev_stats.tx_packets;
 }
 
 /**
-- 
2.25.1

[PATCH v3 net-next 09/12] net: net_failover: ensure .ndo_get_stats64 can sleep

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The failover framework sets up a virtio_net interface [ when it has the
VIRTIO_NET_F_STANDBY feature ] and a VF interface, having the same MAC
address, in a standby/active relationship. When the active VF is
unplugged, the standby virtio_net temporarily kicks in.

The failover framework registers a common upper for the active and the
standby interface, which is what the application layer uses. This is
similar to bonding/team. The statistics of the upper interface are the
sum of the statistics of the active and of the standby interface.

There is an effort to convert .ndo_get_stats64 to sleepable context, and
for that to work, we need to prevent callers of dev_get_stats from using
atomic locking. The failover driver needs protection via an RCU
read-side critical section to access the standby and the active
interface. This has two features:
- It is atomic: this needs to change.
- It is reentrant: this is ok, because generally speaking, dev_get_stats
  is recursive, and taking global locks is a bad thing from a recursive
  context.

A better locking architecture would be to do what the team driver does.
Instead of using something as broad as the rtnl_mutex to ensure
serialization of updates, it should use something more specific, like a
private mutex. This patch adds that and names it slaves_lock. The
slaves_lock now protects the only updater, the rcu_assign_pointer
sections from net_failover_slave_register.

In the team driver, a separate lockdep class is created for each team
lock, to account for possible nesting (team over team over ...). For the
net_failover driver, we can do something simpler, which is to just not
hold any lock while we call dev_get_stats recursively. We can "cheat"
and use dev_hold to take a reference on the active and backup interfaces,
and netdev_wait_allrefs() will just have to wait until we finish.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
Switched to the new scheme of holding just a refcnt to the slave
interfaces while recursing with dev_get_stats.

 drivers/net/net_failover.c | 62 +++++++++++++++++++++++++++-----------
 include/net/net_failover.h |  9 ++++--
 2 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/drivers/net/net_failover.c b/drivers/net/net_failover.c
index 4f83165412bd..c83066b0ef70 100644
--- a/drivers/net/net_failover.c
+++ b/drivers/net/net_failover.c
@@ -27,6 +27,9 @@
 #include <uapi/linux/if_arp.h>
 #include <net/net_failover.h>
 
+#define nfo_dereference(nfo_info, p)				\
+	rcu_dereference_protected(p, lockdep_is_held(&nfo_info->slaves_lock))
+
 static bool net_failover_xmit_ready(struct net_device *dev)
 {
 	return netif_running(dev) && netif_carrier_ok(dev);
@@ -183,32 +186,48 @@ static void net_failover_get_stats(struct net_device *dev,
 				   struct rtnl_link_stats64 *stats)
 {
 	struct net_failover_info *nfo_info = netdev_priv(dev);
-	struct rtnl_link_stats64 temp;
-	struct net_device *slave_dev;
+	struct rtnl_link_stats64 primary_stats;
+	struct rtnl_link_stats64 standby_stats;
+	struct net_device *primary_dev;
+	struct net_device *standby_dev;
 
-	spin_lock(&nfo_info->stats_lock);
-	memcpy(stats, &nfo_info->failover_stats, sizeof(*stats));
+	mutex_lock(&nfo_info->slaves_lock);
 
-	rcu_read_lock();
+	primary_dev = nfo_dereference(nfo_info, nfo_info->primary_dev);
+	if (primary_dev)
+		dev_hold(primary_dev);
 
-	slave_dev = rcu_dereference(nfo_info->primary_dev);
-	if (slave_dev) {
-		dev_get_stats(slave_dev, &temp);
-		net_failover_fold_stats(stats, &temp, &nfo_info->primary_stats);
-		memcpy(&nfo_info->primary_stats, &temp, sizeof(temp));
+	standby_dev = nfo_dereference(nfo_info, nfo_info->standby_dev);
+	if (standby_dev)
+		dev_hold(standby_dev);
+
+	mutex_unlock(&nfo_info->slaves_lock);
+
+	/* Don't hold slaves_lock while calling dev_get_stats, just a
+	 * reference to ensure they won't get unregistered.
+	 */
+	if (primary_dev) {
+		dev_get_stats(primary_dev, &primary_stats);
+		dev_put(primary_dev);
 	}
 
-	slave_dev = rcu_dereference(nfo_info->standby_dev);
-	if (slave_dev) {
-		dev_get_stats(slave_dev, &temp);
-		net_failover_fold_stats(stats, &temp, &nfo_info->standby_stats);
-		memcpy(&nfo_info->standby_stats, &temp, sizeof(temp));
+	if (standby_dev) {
+		dev_get_stats(standby_dev, &standby_stats);
+		dev_put(standby_dev);
 	}
 
-	rcu_read_unlock();
+	mutex_lock(&nfo_info->stats_lock);
+
+	memcpy(stats, &nfo_info->failover_stats, sizeof(*stats));
+
+	net_failover_fold_stats(stats, &primary_stats, &nfo_info->primary_stats);
+	memcpy(&nfo_info->primary_stats, &primary_stats, sizeof(primary_stats));
+	net_failover_fold_stats(stats, &standby_stats, &nfo_info->standby_stats);
+	memcpy(&nfo_info->standby_stats, &standby_stats, sizeof(standby_stats));
 
 	memcpy(&nfo_info->failover_stats, stats, sizeof(*stats));
-	spin_unlock(&nfo_info->stats_lock);
+
+	mutex_unlock(&nfo_info->stats_lock);
 }
 
 static int net_failover_change_mtu(struct net_device *dev, int new_mtu)
@@ -540,6 +559,8 @@ static int net_failover_slave_register(struct net_device *slave_dev,
 	primary_dev = rtnl_dereference(nfo_info->primary_dev);
 	slave_is_standby = slave_dev->dev.parent == failover_dev->dev.parent;
 
+	mutex_lock(&nfo_info->slaves_lock);
+
 	if (slave_is_standby) {
 		rcu_assign_pointer(nfo_info->standby_dev, slave_dev);
 		standby_dev = slave_dev;
@@ -552,6 +573,8 @@ static int net_failover_slave_register(struct net_device *slave_dev,
 		failover_dev->max_mtu = slave_dev->max_mtu;
 	}
 
+	mutex_unlock(&nfo_info->slaves_lock);
+
 	net_failover_lower_state_changed(slave_dev, primary_dev, standby_dev);
 	net_failover_compute_features(failover_dev);
 
@@ -709,6 +732,7 @@ static struct failover_ops net_failover_ops = {
 struct failover *net_failover_create(struct net_device *standby_dev)
 {
 	struct device *dev = standby_dev->dev.parent;
+	struct net_failover_info *nfo_info;
 	struct net_device *failover_dev;
 	struct failover *failover;
 	int err;
@@ -753,6 +777,10 @@ struct failover *net_failover_create(struct net_device *standby_dev)
 	failover_dev->min_mtu = standby_dev->min_mtu;
 	failover_dev->max_mtu = standby_dev->max_mtu;
 
+	nfo_info = netdev_priv(failover_dev);
+	mutex_init(&nfo_info->slaves_lock);
+	mutex_init(&nfo_info->stats_lock);
+
 	err = register_netdev(failover_dev);
 	if (err) {
 		dev_err(dev, "Unable to register failover_dev!\n");
diff --git a/include/net/net_failover.h b/include/net/net_failover.h
index b12a1c469d1c..988cdfaf14ca 100644
--- a/include/net/net_failover.h
+++ b/include/net/net_failover.h
@@ -23,8 +23,13 @@ struct net_failover_info {
 	/* aggregated stats */
 	struct rtnl_link_stats64 failover_stats;
 
-	/* spinlock while updating stats */
-	spinlock_t stats_lock;
+	/* lock for updating stats */
+	struct mutex stats_lock;
+
+	/* lock for protecting lower interfaces.
+	 * TODO: convert all rtnl_dereference instances to nfo_dereference
+	 */
+	struct mutex slaves_lock;
 };
 
 struct failover *net_failover_create(struct net_device *standby_dev);
-- 
2.25.1

[PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

There is an effort to convert .ndo_get_stats64 to sleepable context, and
for that to work, we need to prevent callers of dev_get_stats from using
atomic locking.

The bonding driver retrieves its statistics recursively from its lower
interfaces, with additional care to only count packets sent/received
while those lowers were actually enslaved to the bond - see commit
5f0c5f73e5ef ("bonding: make global bonding stats more reliable").

Since commit 87163ef9cda7 ("bonding: remove last users of bond->lock and
bond->lock itself"), the bonding driver uses the following protection
for its array of slaves: RCU for readers and rtnl_mutex for updaters.
This is not great because there is another movement [ somehow
simultaneous with the one to make .ndo_get_stats64 sleepable ] to reduce
driver usage of rtnl_mutex. This makes sense, because the rtnl_mutex has
become a very contended resource.

The aforementioned commit removed an interesting comment:

	/* [...] we can't hold bond->lock [...] because we'll
	 * deadlock. The only solution is to rely on the fact
	 * that we're under rtnl_lock here, and the slaves
	 * list won't change. This doesn't solve the problem
	 * of setting the slave's MTU while it is
	 * transmitting, but the assumption is that the base
	 * driver can handle that.
	 *
	 * TODO: figure out a way to safely iterate the slaves
	 * list, but without holding a lock around the actual
	 * call to the base driver.
	 */

The above summarizes pretty well the challenges we have with nested
bonding interfaces (bond over bond over bond over...), which need to be
addressed by a better locking scheme that also not relies on the bloated
rtnl_mutex.

Instead of using something as broad as the rtnl_mutex to ensure
serialization of updates to the slave array, we can reintroduce a
private mutex in the bonding driver, called slaves_lock.
This mutex circles the only updater, bond_update_slave_arr, and ensures
that whatever other readers want to see a consistent slave array, they
don't need to hold the rtnl_mutex for that.

Now _of_course_ I did not convert the entire driver to use
bond_for_each_slave protected by the bond->slaves_lock, and
rtnl_dereference to bond_dereference. I just started that process by
converting the one reader I needed: ndo_get_stats64. Not only is it nice
to not hold rtnl_mutex in .ndo_get_stats64, but it is also in fact
forbidden to do so (since top-level callers may hold netif_lists_lock,
which is a sub-lock of the rtnl_mutex, and therefore this would cause a
lock inversion and a deadlock).

To solve the nesting problem, the simple way is to not hold any locks
when recursing into the slave netdev operation, which is exactly the
approach that we take. We can "cheat" and use dev_hold to take a
reference on the slave net_device, which is enough to ensure that
netdev_wait_allrefs() waits until we finish, and the kernel won't fault.
However, the slave structure might no longer be valid, just its
associated net_device. That isn't a biggie. We just need to do some more
work to ensure that the slave exists after we took the statistics, and
if it still does, reapply the logic from Andy's commit 5f0c5f73e5ef.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
- Added kfree(dev_stats);
- Removed the now useless stats_lock (bond->bond_stats and
  slave->slave_stats are now protected by bond->slaves_lock)

Changes in v2:
Switched to the new scheme of holding just a refcnt to the slave
interfaces while recursing with dev_get_stats.

 drivers/net/bonding/bond_main.c | 118 +++++++++++++++-----------------
 include/net/bonding.h           |  49 ++++++++++++-
 2 files changed, 104 insertions(+), 63 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 714aa0e5d041..6e38079ea66d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3693,77 +3693,64 @@ static void bond_fold_stats(struct rtnl_link_stats64 *_res,
 	}
 }
 
-#ifdef CONFIG_LOCKDEP
-static int bond_get_lowest_level_rcu(struct net_device *dev)
-{
-	struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1];
-	struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1];
-	int cur = 0, max = 0;
-
-	now = dev;
-	iter = &dev->adj_list.lower;
-
-	while (1) {
-		next = NULL;
-		while (1) {
-			ldev = netdev_next_lower_dev_rcu(now, &iter);
-			if (!ldev)
-				break;
-
-			next = ldev;
-			niter = &ldev->adj_list.lower;
-			dev_stack[cur] = now;
-			iter_stack[cur++] = iter;
-			if (max <= cur)
-				max = cur;
-			break;
-		}
-
-		if (!next) {
-			if (!cur)
-				return max;
-			next = dev_stack[--cur];
-			niter = iter_stack[cur];
-		}
-
-		now = next;
-		iter = niter;
-	}
-
-	return max;
-}
-#endif
-
 static void bond_get_stats(struct net_device *bond_dev,
 			   struct rtnl_link_stats64 *stats)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
-	struct rtnl_link_stats64 temp;
-	struct list_head *iter;
-	struct slave *slave;
-	int nest_level = 0;
+	struct rtnl_link_stats64 *dev_stats;
+	struct net_device **slaves;
+	int i, res, num_slaves;
 
+	res = bond_get_slave_arr(bond, &slaves, &num_slaves);
+	if (res) {
+		netdev_err(bond->dev,
+			   "failed to allocate memory for slave array\n");
+		return;
+	}
 
-	rcu_read_lock();
-#ifdef CONFIG_LOCKDEP
-	nest_level = bond_get_lowest_level_rcu(bond_dev);
-#endif
+	dev_stats = kcalloc(num_slaves, sizeof(*dev_stats), GFP_KERNEL);
+	if (!dev_stats) {
+		netdev_err(bond->dev,
+			   "failed to allocate memory for slave stats\n");
+		bond_put_slave_arr(slaves, num_slaves);
+		return;
+	}
+
+	/* Recurse with no locks taken */
+	for (i = 0; i < num_slaves; i++)
+		dev_get_stats(slaves[i], &dev_stats[i]);
+
+	/* When taking the slaves lock again, the new slave array might be
+	 * different from the original one.
+	 */
+	mutex_lock(&bond->slaves_lock);
 
-	spin_lock_nested(&bond->stats_lock, nest_level);
 	memcpy(stats, &bond->bond_stats, sizeof(*stats));
 
-	bond_for_each_slave_rcu(bond, slave, iter) {
-		dev_get_stats(slave->dev, &temp);
+	for (i = 0; i < num_slaves; i++) {
+		struct list_head *iter;
+		struct slave *slave;
 
-		bond_fold_stats(stats, &temp, &slave->slave_stats);
+		bond_for_each_slave(bond, slave, iter) {
+			if (slave->dev != slaves[i])
+				continue;
 
-		/* save off the slave stats for the next run */
-		memcpy(&slave->slave_stats, &temp, sizeof(temp));
+			bond_fold_stats(stats, &dev_stats[i],
+					&slave->slave_stats);
+
+			/* save off the slave stats for the next run */
+			memcpy(&slave->slave_stats, &dev_stats[i],
+			       sizeof(dev_stats[i]));
+			break;
+		}
 	}
 
 	memcpy(&bond->bond_stats, stats, sizeof(*stats));
-	spin_unlock(&bond->stats_lock);
-	rcu_read_unlock();
+
+	mutex_unlock(&bond->slaves_lock);
+
+	kfree(dev_stats);
+	bond_put_slave_arr(slaves, num_slaves);
 }
 
 static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd)
@@ -4287,11 +4274,11 @@ static void bond_set_slave_arr(struct bonding *bond,
 {
 	struct bond_up_slave *usable, *all;
 
-	usable = rtnl_dereference(bond->usable_slaves);
+	usable = bond_dereference(bond, bond->usable_slaves);
 	rcu_assign_pointer(bond->usable_slaves, usable_slaves);
 	kfree_rcu(usable, rcu);
 
-	all = rtnl_dereference(bond->all_slaves);
+	all = bond_dereference(bond, bond->all_slaves);
 	rcu_assign_pointer(bond->all_slaves, all_slaves);
 	kfree_rcu(all, rcu);
 }
@@ -4333,6 +4320,8 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 	WARN_ON(lockdep_is_held(&bond->mode_lock));
 #endif
 
+	mutex_lock(&bond->slaves_lock);
+
 	usable_slaves = kzalloc(struct_size(usable_slaves, arr,
 					    bond->slave_cnt), GFP_KERNEL);
 	all_slaves = kzalloc(struct_size(all_slaves, arr,
@@ -4376,17 +4365,22 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 	}
 
 	bond_set_slave_arr(bond, usable_slaves, all_slaves);
+
+	mutex_unlock(&bond->slaves_lock);
+
 	return ret;
 out:
 	if (ret != 0 && skipslave) {
-		bond_skip_slave(rtnl_dereference(bond->all_slaves),
+		bond_skip_slave(bond_dereference(bond, bond->all_slaves),
 				skipslave);
-		bond_skip_slave(rtnl_dereference(bond->usable_slaves),
+		bond_skip_slave(bond_dereference(bond, bond->usable_slaves),
 				skipslave);
 	}
 	kfree_rcu(all_slaves, rcu);
 	kfree_rcu(usable_slaves, rcu);
 
+	mutex_unlock(&bond->slaves_lock);
+
 	return ret;
 }
 
@@ -4699,6 +4693,7 @@ void bond_setup(struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 
+	mutex_init(&bond->slaves_lock);
 	spin_lock_init(&bond->mode_lock);
 	bond->params = bonding_defaults;
 
@@ -5189,7 +5184,6 @@ static int bond_init(struct net_device *bond_dev)
 	if (!bond->wq)
 		return -ENOMEM;
 
-	spin_lock_init(&bond->stats_lock);
 	netdev_lockdep_set_classes(bond_dev);
 
 	list_add_tail(&bond->bond_list, &bn->dev_list);
diff --git a/include/net/bonding.h b/include/net/bonding.h
index adc3da776970..146b46d276c0 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -222,7 +222,6 @@ struct bonding {
 	 * ALB mode (6) - to sync the use and modifications of its hash table
 	 */
 	spinlock_t mode_lock;
-	spinlock_t stats_lock;
 	u8	 send_peer_notif;
 	u8       igmp_retrans;
 #ifdef CONFIG_PROC_FS
@@ -249,6 +248,11 @@ struct bonding {
 #ifdef CONFIG_XFRM_OFFLOAD
 	struct xfrm_state *xs;
 #endif /* CONFIG_XFRM_OFFLOAD */
+
+	/* Protects the slave array. TODO: convert all instances of
+	 * rtnl_dereference to bond_dereference
+	 */
+	struct mutex slaves_lock;
 };
 
 #define bond_slave_get_rcu(dev) \
@@ -257,6 +261,9 @@ struct bonding {
 #define bond_slave_get_rtnl(dev) \
 	((struct slave *) rtnl_dereference(dev->rx_handler_data))
 
+#define bond_dereference(bond, p) \
+	rcu_dereference_protected(p, lockdep_is_held(&(bond)->slaves_lock))
+
 void bond_queue_slave_event(struct slave *slave);
 void bond_lower_state_changed(struct slave *slave);
 
@@ -449,6 +456,46 @@ static inline void bond_hw_addr_copy(u8 *dst, const u8 *src, unsigned int len)
 	memcpy(dst, src, len);
 }
 
+static inline int bond_get_slave_arr(struct bonding *bond,
+				     struct net_device ***slaves,
+				     int *num_slaves)
+{
+	struct net *net = dev_net(bond->dev);
+	struct list_head *iter;
+	struct slave *slave;
+	int i = 0;
+
+	mutex_lock(&bond->slaves_lock);
+
+	*slaves = kcalloc(bond->slave_cnt, sizeof(*slaves), GFP_KERNEL);
+	if (!(*slaves)) {
+		netif_lists_unlock(net);
+		return -ENOMEM;
+	}
+
+	bond_for_each_slave(bond, slave, iter) {
+		dev_hold(slave->dev);
+		*slaves[i++] = slave->dev;
+	}
+
+	*num_slaves = bond->slave_cnt;
+
+	mutex_unlock(&bond->slaves_lock);
+
+	return 0;
+}
+
+static inline void bond_put_slave_arr(struct net_device **slaves,
+				      int num_slaves)
+{
+	int i;
+
+	for (i = 0; i < num_slaves; i++)
+		dev_put(slaves[i]);
+
+	kfree(slaves);
+}
+
 #define BOND_PRI_RESELECT_ALWAYS	0
 #define BOND_PRI_RESELECT_BETTER	1
 #define BOND_PRI_RESELECT_FAILURE	2
-- 
2.25.1

[PATCH v3 net-next 11/12] net: mark ndo_get_stats64 as being able to sleep

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Now that all callers have been converted to not use atomic context when
calling dev_get_stats, it is time to update the documentation and put a
notice in the function that it expects process context.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
Updated the documentation.

 Documentation/networking/netdevices.rst | 8 ++++++--
 Documentation/networking/statistics.rst | 9 ++++-----
 net/core/dev.c                          | 2 ++
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
index 5a85fcc80c76..944599722c76 100644
--- a/Documentation/networking/netdevices.rst
+++ b/Documentation/networking/netdevices.rst
@@ -64,8 +64,12 @@ ndo_do_ioctl:
 	Context: process
 
 ndo_get_stats:
-	Synchronization: dev_base_lock rwlock.
-	Context: nominally process, but don't sleep inside an rwlock
+	Synchronization:
+		none. netif_lists_lock(net) might be held, but not guaranteed.
+		It is illegal to hold rtnl_lock() in this method, since it will
+		cause a lock inversion with netif_lists_lock and a deadlock.
+	Context:
+		process
 
 ndo_start_xmit:
 	Synchronization: __netif_tx_lock spinlock.
diff --git a/Documentation/networking/statistics.rst b/Documentation/networking/statistics.rst
index 234abedc29b2..ad3e353df0dd 100644
--- a/Documentation/networking/statistics.rst
+++ b/Documentation/networking/statistics.rst
@@ -155,11 +155,10 @@ Drivers must ensure best possible compliance with
 Please note for example that detailed error statistics must be
 added into the general `rx_error` / `tx_error` counters.
 
-The `.ndo_get_stats64` callback can not sleep because of accesses
-via `/proc/net/dev`. If driver may sleep when retrieving the statistics
-from the device it should do so periodically asynchronously and only return
-a recent copy from `.ndo_get_stats64`. Ethtool interrupt coalescing interface
-allows setting the frequency of refreshing statistics, if needed.
+Drivers may sleep when retrieving the statistics from the device, or they might
+read the counters periodically and only return in `.ndo_get_stats64` a recent
+copy collected asynchronously. In the latter case, the ethtool interrupt
+coalescing interface allows setting the frequency of refreshing statistics.
 
 Retrieving ethtool statistics is a multi-syscall process, drivers are advised
 to keep the number of statistics constant to avoid race conditions with
diff --git a/net/core/dev.c b/net/core/dev.c
index 93618300ac90..ccb31a52e514 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10407,6 +10407,8 @@ void dev_get_stats(struct net_device *dev, struct rtnl_link_stats64 *storage)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 
+	might_sleep();
+
 	if (ops->ndo_get_stats64) {
 		memset(storage, 0, sizeof(*storage));
 		ops->ndo_get_stats64(dev, storage);
-- 
2.25.1

[PATCH v3 net-next 12/12] net: remove obsolete comments about ndo_get_stats64 context from eth drivers

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Now that we have a good summary in Documentation/networking/netdevices.rst,
these comments serve no purpose and are actually distracting/confusing.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
None.

 drivers/net/ethernet/cisco/enic/enic_main.c | 1 -
 drivers/net/ethernet/nvidia/forcedeth.c     | 2 --
 drivers/net/ethernet/sfc/efx_common.c       | 1 -
 drivers/net/ethernet/sfc/falcon/efx.c       | 1 -
 4 files changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index fb269d587b74..7425f94f9091 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -870,7 +870,6 @@ static netdev_tx_t enic_hard_start_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
-/* dev_base_lock rwlock held, nominally process context */
 static void enic_get_stats(struct net_device *netdev,
 			   struct rtnl_link_stats64 *net_stats)
 {
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 8724d6a9ed02..8fa254dc64e9 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -1761,8 +1761,6 @@ static void nv_get_stats(int cpu, struct fe_priv *np,
 /*
  * nv_get_stats64: dev->ndo_get_stats64 function
  * Get latest stats value from the nic.
- * Called with read_lock(&dev_base_lock) held for read -
- * only synchronized against unregister_netdevice.
  */
 static void
 nv_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *storage)
diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
index de797e1ac5a9..4d8047b35fb2 100644
--- a/drivers/net/ethernet/sfc/efx_common.c
+++ b/drivers/net/ethernet/sfc/efx_common.c
@@ -596,7 +596,6 @@ void efx_stop_all(struct efx_nic *efx)
 	efx_stop_datapath(efx);
 }
 
-/* Context: process, dev_base_lock or RTNL held, non-blocking. */
 void efx_net_stats(struct net_device *net_dev, struct rtnl_link_stats64 *stats)
 {
 	struct efx_nic *efx = netdev_priv(net_dev);
diff --git a/drivers/net/ethernet/sfc/falcon/efx.c b/drivers/net/ethernet/sfc/falcon/efx.c
index f8979991970e..6db2b6583dec 100644
--- a/drivers/net/ethernet/sfc/falcon/efx.c
+++ b/drivers/net/ethernet/sfc/falcon/efx.c
@@ -2096,7 +2096,6 @@ int ef4_net_stop(struct net_device *net_dev)
 	return 0;
 }
 
-/* Context: process, dev_base_lock or RTNL held, non-blocking. */
 static void ef4_net_stats(struct net_device *net_dev,
 			  struct rtnl_link_stats64 *stats)
 {
-- 
2.25.1

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Eric Dumazet]

On Thu, Jan 7, 2021 at 10:51 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
>

-
>  static void bond_get_stats(struct net_device *bond_dev,
>                            struct rtnl_link_stats64 *stats)
>  {
>         struct bonding *bond = netdev_priv(bond_dev);
> -       struct rtnl_link_stats64 temp;
> -       struct list_head *iter;
> -       struct slave *slave;
> -       int nest_level = 0;
> +       struct rtnl_link_stats64 *dev_stats;
> +       struct net_device **slaves;
> +       int i, res, num_slaves;
>
> +       res = bond_get_slave_arr(bond, &slaves, &num_slaves);
> +       if (res) {
> +               netdev_err(bond->dev,
> +                          "failed to allocate memory for slave array\n");
> +               return;
> +       }
>

What a mess really.

You chose to keep the assumption that ndo_get_stats() would not fail,
since we were providing the needed storage from callers.

If ndo_get_stats() are now allowed to sleep, and presumably allocate
memory, we need to make sure
we report potential errors back to the user.

I think your patch series is mostly good, but I would prefer not
hiding errors and always report them to user space.
And no, netdev_err() is not appropriate, we do not want tools to look
at syslog to guess something went wrong.

Last point about drivers having to go to slow path, talking to
firmware : Make sure that malicious/innocent users
reading /proc/net/dev from many threads in parallel wont brick these devices.

Maybe they implicitly _relied_ on the fact that firmware was gently
read every second and results were cached from a work queue or
something.

Thanks.

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Vladimir Oltean]

On Thu, Jan 07, 2021 at 12:18:28PM +0100, Eric Dumazet wrote:
> What a mess really.

Thanks, that's at least _some_ feedback :)

> You chose to keep the assumption that ndo_get_stats() would not fail,
> since we were providing the needed storage from callers.
>
> If ndo_get_stats() are now allowed to sleep, and presumably allocate
> memory, we need to make sure
> we report potential errors back to the user.
>
> I think your patch series is mostly good, but I would prefer not
> hiding errors and always report them to user space.
> And no, netdev_err() is not appropriate, we do not want tools to look
> at syslog to guess something went wrong.

Well, there are only 22 dev_get_stats callers in the kernel, so I assume
that after the conversion to return void, I can do another conversion to
return int, and then I can convert the ndo_get_stats64 method to return
int too. I will keep the plain ndo_get_stats still void (no reason not
to).

> Last point about drivers having to go to slow path, talking to
> firmware : Make sure that malicious/innocent users
> reading /proc/net/dev from many threads in parallel wont brick these devices.
>
> Maybe they implicitly _relied_ on the fact that firmware was gently
> read every second and results were cached from a work queue or
> something.

How? I don't understand how I can make sure of that.

There is an effort initiated by Jakub to standardize the ethtool
statistics. My objection was that you can't expect that to happen unless
dev_get_stats is sleepable just like ethtool -S is. So I think the same
reasoning should apply to ethtool -S too, really.

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Eric Dumazet]

On Thu, Jan 7, 2021 at 12:33 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Thu, Jan 07, 2021 at 12:18:28PM +0100, Eric Dumazet wrote:
> > What a mess really.
>
> Thanks, that's at least _some_ feedback :)

Yeah, I was on PTO for the last two weeks.

>
> > You chose to keep the assumption that ndo_get_stats() would not fail,
> > since we were providing the needed storage from callers.
> >
> > If ndo_get_stats() are now allowed to sleep, and presumably allocate
> > memory, we need to make sure
> > we report potential errors back to the user.
> >
> > I think your patch series is mostly good, but I would prefer not
> > hiding errors and always report them to user space.
> > And no, netdev_err() is not appropriate, we do not want tools to look
> > at syslog to guess something went wrong.
>
> Well, there are only 22 dev_get_stats callers in the kernel, so I assume
> that after the conversion to return void, I can do another conversion to
> return int, and then I can convert the ndo_get_stats64 method to return
> int too. I will keep the plain ndo_get_stats still void (no reason not
> to).
>
> > Last point about drivers having to go to slow path, talking to
> > firmware : Make sure that malicious/innocent users
> > reading /proc/net/dev from many threads in parallel wont brick these devices.
> >
> > Maybe they implicitly _relied_ on the fact that firmware was gently
> > read every second and results were cached from a work queue or
> > something.
>
> How? I don't understand how I can make sure of that.

Your patches do not attempt to change these drivers, but I guess your
cover letter might send to driver maintainers incentive to get rid of their
logic, that is all.

We might simply warn maintainers and ask them to test their future changes
with tests using 1000 concurrent theads reading /proc/net/dev

>
> There is an effort initiated by Jakub to standardize the ethtool
> statistics. My objection was that you can't expect that to happen unless
> dev_get_stats is sleepable just like ethtool -S is. So I think the same
> reasoning should apply to ethtool -S too, really.

I think we all agree on the principles, once we make sure to not
add more pressure on RTNL. It seems you addressed our feedback, all is fine.

Thanks.

Re: [PATCH v3 net-next 08/12] net: make dev_get_stats return void

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7321 bytes --]

Hi Vladimir,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Make-ndo_get_stats64-sleepable/20210107-175746
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3db1a3fa98808aa90f95ec3e0fa2fc7abf28f5c9
config: x86_64-randconfig-a005-20210107 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5c951623bc8965fa1e89660f2f5f4a2944e4981a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/5d1dbcbfc55bf64381ca2bf9833e95f2256a7b3f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vladimir-Oltean/Make-ndo_get_stats64-sleepable/20210107-175746
        git checkout 5d1dbcbfc55bf64381ca2bf9833e95f2256a7b3f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/scsi/fcoe/fcoe_transport.c:176:19: error: redefinition of 'stats' with a different type: 'struct fc_stats *' vs 'struct rtnl_link_stats64'
           struct fc_stats *stats;
                            ^
   drivers/scsi/fcoe/fcoe_transport.c:173:27: note: previous definition is here
           struct rtnl_link_stats64 stats;
                                    ^
>> drivers/scsi/fcoe/fcoe_transport.c:185:9: error: assigning to 'struct rtnl_link_stats64' from incompatible type 'typeof ((typeof (*((lport->stats))) *)((lport->stats)))' (aka 'struct fc_stats *')
                   stats = per_cpu_ptr(lport->stats, cpu);
                         ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/fcoe/fcoe_transport.c:186:15: error: member reference type 'struct rtnl_link_stats64' is not a pointer; did you mean to use '.'?
                   lfc += stats->LinkFailureCount;
                          ~~~~~^~
                               .
   drivers/scsi/fcoe/fcoe_transport.c:186:17: error: no member named 'LinkFailureCount' in 'struct rtnl_link_stats64'
                   lfc += stats->LinkFailureCount;
                          ~~~~~  ^
   drivers/scsi/fcoe/fcoe_transport.c:187:16: error: member reference type 'struct rtnl_link_stats64' is not a pointer; did you mean to use '.'?
                   vlfc += stats->VLinkFailureCount;
                           ~~~~~^~
                                .
   drivers/scsi/fcoe/fcoe_transport.c:187:18: error: no member named 'VLinkFailureCount' in 'struct rtnl_link_stats64'
                   vlfc += stats->VLinkFailureCount;
                           ~~~~~  ^
   drivers/scsi/fcoe/fcoe_transport.c:188:16: error: member reference type 'struct rtnl_link_stats64' is not a pointer; did you mean to use '.'?
                   mdac += stats->MissDiscAdvCount;
                           ~~~~~^~
                                .
   drivers/scsi/fcoe/fcoe_transport.c:188:18: error: no member named 'MissDiscAdvCount' in 'struct rtnl_link_stats64'
                   mdac += stats->MissDiscAdvCount;
                           ~~~~~  ^
   drivers/scsi/fcoe/fcoe_transport.c:866:27: warning: cast to smaller integer type 'enum fip_mode' from 'void *' [-Wvoid-pointer-to-enum-cast]
           enum fip_mode fip_mode = (enum fip_mode)kp->arg;
                                    ^~~~~~~~~~~~~~~~~~~~~~
   1 warning and 8 errors generated.


vim +185 drivers/scsi/fcoe/fcoe_transport.c

03702689fcc985e Yi Zou                  2012-12-06  159  
57c2728fa806aff Yi Zou                  2012-12-06  160  /**
57c2728fa806aff Yi Zou                  2012-12-06  161   * __fcoe_get_lesb() - Get the Link Error Status Block (LESB) for a given lport
57c2728fa806aff Yi Zou                  2012-12-06  162   * @lport: The local port to update speeds for
57c2728fa806aff Yi Zou                  2012-12-06  163   * @fc_lesb: Pointer to the LESB to be filled up
57c2728fa806aff Yi Zou                  2012-12-06  164   * @netdev: Pointer to the netdev that is associated with the lport
57c2728fa806aff Yi Zou                  2012-12-06  165   *
57c2728fa806aff Yi Zou                  2012-12-06  166   * Note, the Link Error Status Block (LESB) for FCoE is defined in FC-BB-6
57c2728fa806aff Yi Zou                  2012-12-06  167   * Clause 7.11 in v1.04.
57c2728fa806aff Yi Zou                  2012-12-06  168   */
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  169  void __fcoe_get_lesb(struct fc_lport *lport,
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  170  		     struct fc_els_lesb *fc_lesb,
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  171  		     struct net_device *netdev)
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  172  {
5d1dbcbfc55bf64 Vladimir Oltean         2021-01-07  173  	struct rtnl_link_stats64 stats;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  174  	unsigned int cpu;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  175  	u32 lfc, vlfc, mdac;
1bd49b482077e23 Vasu Dev                2012-05-25  176  	struct fc_stats *stats;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  177  	struct fcoe_fc_els_lesb *lesb;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  178  
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  179  	lfc = 0;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  180  	vlfc = 0;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  181  	mdac = 0;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  182  	lesb = (struct fcoe_fc_els_lesb *)fc_lesb;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  183  	memset(lesb, 0, sizeof(*lesb));
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  184  	for_each_possible_cpu(cpu) {
1bd49b482077e23 Vasu Dev                2012-05-25 @185  		stats = per_cpu_ptr(lport->stats, cpu);
1bd49b482077e23 Vasu Dev                2012-05-25  186  		lfc += stats->LinkFailureCount;
1bd49b482077e23 Vasu Dev                2012-05-25  187  		vlfc += stats->VLinkFailureCount;
1bd49b482077e23 Vasu Dev                2012-05-25  188  		mdac += stats->MissDiscAdvCount;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  189  	}
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  190  	lesb->lesb_link_fail = htonl(lfc);
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  191  	lesb->lesb_vlink_fail = htonl(vlfc);
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  192  	lesb->lesb_miss_fka = htonl(mdac);
5d1dbcbfc55bf64 Vladimir Oltean         2021-01-07  193  	dev_get_stats(netdev, &stats);
5d1dbcbfc55bf64 Vladimir Oltean         2021-01-07  194  	lesb->lesb_fcs_error = htonl(stats.rx_crc_errors);
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  195  }
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  196  EXPORT_SYMBOL_GPL(__fcoe_get_lesb);
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  197  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 47100 bytes --]

Re: [PATCH v3 net-next 08/12] net: make dev_get_stats return void

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7431 bytes --]

Hi Vladimir,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Make-ndo_get_stats64-sleepable/20210107-175746
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3db1a3fa98808aa90f95ec3e0fa2fc7abf28f5c9
config: x86_64-randconfig-a005-20210107 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5c951623bc8965fa1e89660f2f5f4a2944e4981a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/5d1dbcbfc55bf64381ca2bf9833e95f2256a7b3f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vladimir-Oltean/Make-ndo_get_stats64-sleepable/20210107-175746
        git checkout 5d1dbcbfc55bf64381ca2bf9833e95f2256a7b3f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/scsi/fcoe/fcoe_transport.c:176:19: error: redefinition of 'stats' with a different type: 'struct fc_stats *' vs 'struct rtnl_link_stats64'
           struct fc_stats *stats;
                            ^
   drivers/scsi/fcoe/fcoe_transport.c:173:27: note: previous definition is here
           struct rtnl_link_stats64 stats;
                                    ^
>> drivers/scsi/fcoe/fcoe_transport.c:185:9: error: assigning to 'struct rtnl_link_stats64' from incompatible type 'typeof ((typeof (*((lport->stats))) *)((lport->stats)))' (aka 'struct fc_stats *')
                   stats = per_cpu_ptr(lport->stats, cpu);
                         ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/fcoe/fcoe_transport.c:186:15: error: member reference type 'struct rtnl_link_stats64' is not a pointer; did you mean to use '.'?
                   lfc += stats->LinkFailureCount;
                          ~~~~~^~
                               .
   drivers/scsi/fcoe/fcoe_transport.c:186:17: error: no member named 'LinkFailureCount' in 'struct rtnl_link_stats64'
                   lfc += stats->LinkFailureCount;
                          ~~~~~  ^
   drivers/scsi/fcoe/fcoe_transport.c:187:16: error: member reference type 'struct rtnl_link_stats64' is not a pointer; did you mean to use '.'?
                   vlfc += stats->VLinkFailureCount;
                           ~~~~~^~
                                .
   drivers/scsi/fcoe/fcoe_transport.c:187:18: error: no member named 'VLinkFailureCount' in 'struct rtnl_link_stats64'
                   vlfc += stats->VLinkFailureCount;
                           ~~~~~  ^
   drivers/scsi/fcoe/fcoe_transport.c:188:16: error: member reference type 'struct rtnl_link_stats64' is not a pointer; did you mean to use '.'?
                   mdac += stats->MissDiscAdvCount;
                           ~~~~~^~
                                .
   drivers/scsi/fcoe/fcoe_transport.c:188:18: error: no member named 'MissDiscAdvCount' in 'struct rtnl_link_stats64'
                   mdac += stats->MissDiscAdvCount;
                           ~~~~~  ^
   drivers/scsi/fcoe/fcoe_transport.c:866:27: warning: cast to smaller integer type 'enum fip_mode' from 'void *' [-Wvoid-pointer-to-enum-cast]
           enum fip_mode fip_mode = (enum fip_mode)kp->arg;
                                    ^~~~~~~~~~~~~~~~~~~~~~
   1 warning and 8 errors generated.


vim +185 drivers/scsi/fcoe/fcoe_transport.c

03702689fcc985e Yi Zou                  2012-12-06  159  
57c2728fa806aff Yi Zou                  2012-12-06  160  /**
57c2728fa806aff Yi Zou                  2012-12-06  161   * __fcoe_get_lesb() - Get the Link Error Status Block (LESB) for a given lport
57c2728fa806aff Yi Zou                  2012-12-06  162   * @lport: The local port to update speeds for
57c2728fa806aff Yi Zou                  2012-12-06  163   * @fc_lesb: Pointer to the LESB to be filled up
57c2728fa806aff Yi Zou                  2012-12-06  164   * @netdev: Pointer to the netdev that is associated with the lport
57c2728fa806aff Yi Zou                  2012-12-06  165   *
57c2728fa806aff Yi Zou                  2012-12-06  166   * Note, the Link Error Status Block (LESB) for FCoE is defined in FC-BB-6
57c2728fa806aff Yi Zou                  2012-12-06  167   * Clause 7.11 in v1.04.
57c2728fa806aff Yi Zou                  2012-12-06  168   */
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  169  void __fcoe_get_lesb(struct fc_lport *lport,
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  170  		     struct fc_els_lesb *fc_lesb,
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  171  		     struct net_device *netdev)
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  172  {
5d1dbcbfc55bf64 Vladimir Oltean         2021-01-07  173  	struct rtnl_link_stats64 stats;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  174  	unsigned int cpu;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  175  	u32 lfc, vlfc, mdac;
1bd49b482077e23 Vasu Dev                2012-05-25  176  	struct fc_stats *stats;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  177  	struct fcoe_fc_els_lesb *lesb;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  178  
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  179  	lfc = 0;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  180  	vlfc = 0;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  181  	mdac = 0;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  182  	lesb = (struct fcoe_fc_els_lesb *)fc_lesb;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  183  	memset(lesb, 0, sizeof(*lesb));
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  184  	for_each_possible_cpu(cpu) {
1bd49b482077e23 Vasu Dev                2012-05-25 @185  		stats = per_cpu_ptr(lport->stats, cpu);
1bd49b482077e23 Vasu Dev                2012-05-25  186  		lfc += stats->LinkFailureCount;
1bd49b482077e23 Vasu Dev                2012-05-25  187  		vlfc += stats->VLinkFailureCount;
1bd49b482077e23 Vasu Dev                2012-05-25  188  		mdac += stats->MissDiscAdvCount;
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  189  	}
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  190  	lesb->lesb_link_fail = htonl(lfc);
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  191  	lesb->lesb_vlink_fail = htonl(vlfc);
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  192  	lesb->lesb_miss_fka = htonl(mdac);
5d1dbcbfc55bf64 Vladimir Oltean         2021-01-07  193  	dev_get_stats(netdev, &stats);
5d1dbcbfc55bf64 Vladimir Oltean         2021-01-07  194  	lesb->lesb_fcs_error = htonl(stats.rx_crc_errors);
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  195  }
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  196  EXPORT_SYMBOL_GPL(__fcoe_get_lesb);
814740d5f67ae5f Bhanu Prakash Gollapudi 2011-10-03  197  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 47100 bytes --]

Re: [PATCH v3 net-next 08/12] net: make dev_get_stats return void

[Vladimir Oltean]

On Thu, Jan 07, 2021 at 09:01:03PM +0800, kernel test robot wrote:
> Hi Vladimir,
> 
> I love your patch! Yet something to improve:

These are not scheduled to run on RFC series, are they?
This report came within 3 hours of me posting an identical version to
the RFC series from two days ago:
https://patchwork.kernel.org/project/netdevbpf/patch/20210105185902.3922928-9-olteanv@gmail.com/

Re: [PATCH v3 net-next 08/12] net: make dev_get_stats return void

[Rong Chen]

On 1/7/21 9:15 PM, Vladimir Oltean wrote:
> On Thu, Jan 07, 2021 at 09:01:03PM +0800, kernel test robot wrote:
>> Hi Vladimir,
>>
>> I love your patch! Yet something to improve:
> These are not scheduled to run on RFC series, are they?
> This report came within 3 hours of me posting an identical version to
> the RFC series from two days ago:
> https://patchwork.kernel.org/project/netdevbpf/patch/20210105185902.3922928-9-olteanv@gmail.com/
>

Hi Vladimir,

The issue was found with a rand config which was generated in Jan 7,
the bot didn't notice the issue in the RFC series:

url:https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Make-ndo_get_stats64-sleepable/20210107-175746
base:https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git  3db1a3fa98808aa90f95ec3e0fa2fc7abf28f5c9
config: x86_64-randconfig-a005-20210107 (attached as .config)

Best Regards,
Rong Chen

Re: [PATCH v3 net-next 08/12] net: make dev_get_stats return void

[Rong Chen]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]



On 1/7/21 9:15 PM, Vladimir Oltean wrote:
> On Thu, Jan 07, 2021 at 09:01:03PM +0800, kernel test robot wrote:
>> Hi Vladimir,
>>
>> I love your patch! Yet something to improve:
> These are not scheduled to run on RFC series, are they?
> This report came within 3 hours of me posting an identical version to
> the RFC series from two days ago:
> https://patchwork.kernel.org/project/netdevbpf/patch/20210105185902.3922928-9-olteanv(a)gmail.com/
>

Hi Vladimir,

The issue was found with a rand config which was generated in Jan 7,
the bot didn't notice the issue in the RFC series:

url:https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Make-ndo_get_stats64-sleepable/20210107-175746
base:https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git  3db1a3fa98808aa90f95ec3e0fa2fc7abf28f5c9
config: x86_64-randconfig-a005-20210107 (attached as .config)

Best Regards,
Rong Chen

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Saeed Mahameed]

On Thu, 2021-01-07 at 13:58 +0100, Eric Dumazet wrote:
> On Thu, Jan 7, 2021 at 12:33 PM Vladimir Oltean <olteanv@gmail.com>
> wrote:
> > On Thu, Jan 07, 2021 at 12:18:28PM +0100, Eric Dumazet wrote:
> > > What a mess really.
> > 
> > Thanks, that's at least _some_ feedback :)
> 
> Yeah, I was on PTO for the last two weeks.
> 
> > > You chose to keep the assumption that ndo_get_stats() would not
> > > fail,
> > > since we were providing the needed storage from callers.
> > > 
> > > If ndo_get_stats() are now allowed to sleep, and presumably
> > > allocate
> > > memory, we need to make sure
> > > we report potential errors back to the user.
> > > 
> > > I think your patch series is mostly good, but I would prefer not
> > > hiding errors and always report them to user space.
> > > And no, netdev_err() is not appropriate, we do not want tools to
> > > look
> > > at syslog to guess something went wrong.
> > 
> > Well, there are only 22 dev_get_stats callers in the kernel, so I
> > assume
> > that after the conversion to return void, I can do another
> > conversion to
> > return int, and then I can convert the ndo_get_stats64 method to
> > return
> > int too. I will keep the plain ndo_get_stats still void (no reason
> > not
> > to).
> > 
> > > Last point about drivers having to go to slow path, talking to
> > > firmware : Make sure that malicious/innocent users
> > > reading /proc/net/dev from many threads in parallel wont brick
> > > these devices.
> > > 
> > > Maybe they implicitly _relied_ on the fact that firmware was
> > > gently
> > > read every second and results were cached from a work queue or
> > > something.
> > 
> > How? I don't understand how I can make sure of that.
> 
> Your patches do not attempt to change these drivers, but I guess your
> cover letter might send to driver maintainers incentive to get rid of
> their
> logic, that is all.
> 
> We might simply warn maintainers and ask them to test their future
> changes
> with tests using 1000 concurrent theads reading /proc/net/dev
> 
> > There is an effort initiated by Jakub to standardize the ethtool
> > statistics. My objection was that you can't expect that to happen
> > unless
> > dev_get_stats is sleepable just like ethtool -S is. So I think the
> > same
> > reasoning should apply to ethtool -S too, really.
> 
> I think we all agree on the principles, once we make sure to not
> add more pressure on RTNL. It seems you addressed our feedback, all
> is fine.
> 

Eric, about two years ago you were totally against sleeping in
ndo_get_stats, what happened ? :) 
https://lore.kernel.org/netdev/4cc44e85-cb5e-502c-30f3-c6ea564fe9ac@gmail.com/

My approach to solve this was much simpler and didn't require  a new
mutex nor RTNL lock, all i did is to reduce the rcu critical section to
not include the call to the driver by simply holding the netdev via
dev_hold()

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Vladimir Oltean]

On Thu, Jan 07, 2021 at 07:59:37PM -0800, Saeed Mahameed wrote:
> On Thu, 2021-01-07 at 13:58 +0100, Eric Dumazet wrote:
> > On Thu, Jan 7, 2021 at 12:33 PM Vladimir Oltean <olteanv@gmail.com>
> > wrote:
> > > On Thu, Jan 07, 2021 at 12:18:28PM +0100, Eric Dumazet wrote:
> > > > What a mess really.
> > >
> > > Thanks, that's at least _some_ feedback :)
> >
> > Yeah, I was on PTO for the last two weeks.
> >
> > > > You chose to keep the assumption that ndo_get_stats() would not
> > > > fail,
> > > > since we were providing the needed storage from callers.
> > > >
> > > > If ndo_get_stats() are now allowed to sleep, and presumably
> > > > allocate
> > > > memory, we need to make sure
> > > > we report potential errors back to the user.
> > > >
> > > > I think your patch series is mostly good, but I would prefer not
> > > > hiding errors and always report them to user space.
> > > > And no, netdev_err() is not appropriate, we do not want tools to
> > > > look
> > > > at syslog to guess something went wrong.
> > >
> > > Well, there are only 22 dev_get_stats callers in the kernel, so I
> > > assume
> > > that after the conversion to return void, I can do another
> > > conversion to
> > > return int, and then I can convert the ndo_get_stats64 method to
> > > return
> > > int too. I will keep the plain ndo_get_stats still void (no reason
> > > not
> > > to).
> > >
> > > > Last point about drivers having to go to slow path, talking to
> > > > firmware : Make sure that malicious/innocent users
> > > > reading /proc/net/dev from many threads in parallel wont brick
> > > > these devices.
> > > >
> > > > Maybe they implicitly _relied_ on the fact that firmware was
> > > > gently
> > > > read every second and results were cached from a work queue or
> > > > something.
> > >
> > > How? I don't understand how I can make sure of that.
> >
> > Your patches do not attempt to change these drivers, but I guess your
> > cover letter might send to driver maintainers incentive to get rid of
> > their
> > logic, that is all.
> >
> > We might simply warn maintainers and ask them to test their future
> > changes
> > with tests using 1000 concurrent theads reading /proc/net/dev
> >
> > > There is an effort initiated by Jakub to standardize the ethtool
> > > statistics. My objection was that you can't expect that to happen
> > > unless
> > > dev_get_stats is sleepable just like ethtool -S is. So I think the
> > > same
> > > reasoning should apply to ethtool -S too, really.
> >
> > I think we all agree on the principles, once we make sure to not
> > add more pressure on RTNL. It seems you addressed our feedback, all
> > is fine.
> >
>
> Eric, about two years ago you were totally against sleeping in
> ndo_get_stats, what happened ? :)
> https://lore.kernel.org/netdev/4cc44e85-cb5e-502c-30f3-c6ea564fe9ac@gmail.com/

I believe that what is different this time is that DSA switches are
typically connected over a slow and bottlenecked bus (so periodic
driver-level readouts would only make things worse for phc2sys and
such other latency-sensitive programs), plus they are offloading
interfaces for forwarding (so software-based counters could never be
accurate). Support those, and supporting firmware-based high-speed
devices will come as a nice side-effect.
FWIW that discussion took place here:
https://patchwork.ozlabs.org/project/netdev/patch/20201125193740.36825-3-george.mccollister@gmail.com/

> My approach to solve this was much simpler and didn't require  a new
> mutex nor RTNL lock, all i did is to reduce the rcu critical section to
> not include the call to the driver by simply holding the netdev via
> dev_hold()

I feel this is a call for the bonding maintainers to make. If they're
willing to replace rtnl_dereference with bond_dereference throughout the
whole driver, and reduce other guys' amount of work when other NDOs
start losing the rtnl_mutex too, then I can't see what's wrong with my
approach (despite not being "as simple"). If they think that update-side
protection of the slaves array is just fine the way it is, then I
suppose that RCU protection + dev_hold is indeed all that I can do.

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Eric Dumazet]

On Fri, Jan 8, 2021 at 4:59 AM Saeed Mahameed <saeed@kernel.org> wrote:
>
> Eric, about two years ago you were totally against sleeping in
> ndo_get_stats, what happened ? :)
> https://lore.kernel.org/netdev/4cc44e85-cb5e-502c-30f3-c6ea564fe9ac@gmail.com/
>
> My approach to solve this was much simpler and didn't require  a new
> mutex nor RTNL lock, all i did is to reduce the rcu critical section to
> not include the call to the driver by simply holding the netdev via
> dev_hold()
>

Yeah, and how have you dealt with bonding at that time ?

Look, it seems to me Vladimir's work is more polished.

If you disagree, repost a rebased patch series so that we can
test/compare and choose the best solution.

And make sure to test it with LOCKDEP enabled ;)

Thanks.

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Vladimir Oltean]

On Fri, Jan 08, 2021 at 10:14:01AM +0100, Eric Dumazet wrote:
> If you disagree, repost a rebased patch series so that we can
> test/compare and choose the best solution.

I would rather use Saeed's time as a reviewer to my existing and current
patch set.

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Eric Dumazet]

On Fri, Jan 8, 2021 at 10:21 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Fri, Jan 08, 2021 at 10:14:01AM +0100, Eric Dumazet wrote:
> > If you disagree, repost a rebased patch series so that we can
> > test/compare and choose the best solution.
>
> I would rather use Saeed's time as a reviewer to my existing and current
> patch set.

Yes, same feeling here, but Saeed brought back his own old
implementation, so maybe he does not feel the same way ?

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Saeed Mahameed]

On Fri, 2021-01-08 at 10:57 +0200, Vladimir Oltean wrote:
> On Thu, Jan 07, 2021 at 07:59:37PM -0800, Saeed Mahameed wrote:
> > On Thu, 2021-01-07 at 13:58 +0100, Eric Dumazet wrote:
> > > On Thu, Jan 7, 2021 at 12:33 PM Vladimir Oltean <
> > > olteanv@gmail.com>
> > > wrote:

...

> > > 
> > > > There is an effort initiated by Jakub to standardize the
> > > > ethtool
> > > > statistics. My objection was that you can't expect that to
> > > > happen
> > > > unless
> > > > dev_get_stats is sleepable just like ethtool -S is. So I think
> > > > the
> > > > same
> > > > reasoning should apply to ethtool -S too, really.
> > > 
> > > I think we all agree on the principles, once we make sure to not
> > > add more pressure on RTNL. It seems you addressed our feedback,
> > > all
> > > is fine.
> > > 
> > 
> > Eric, about two years ago you were totally against sleeping in
> > ndo_get_stats, what happened ? :)
> > https://lore.kernel.org/netdev/4cc44e85-cb5e-502c-30f3-c6ea564fe9ac@gmail.com/
> 
> I believe that what is different this time is that DSA switches are
> typically connected over a slow and bottlenecked bus (so periodic
> driver-level readouts would only make things worse for phc2sys and
> such other latency-sensitive programs), plus they are offloading
> interfaces for forwarding (so software-based counters could never be
> accurate). Support those, and supporting firmware-based high-speed
> devices will come as a nice side-effect.
> FWIW that discussion took place here:
> https://patchwork.ozlabs.org/project/netdev/patch/20201125193740.36825-3-george.mccollister@gmail.com/
> 

I understand the motivation and I agree with the concept, 
hence my patchset :)

> > My approach to solve this was much simpler and didn't require  a
> > new
> > mutex nor RTNL lock, all i did is to reduce the rcu critical
> > section to
> > not include the call to the driver by simply holding the netdev via
> > dev_hold()
> 
> I feel this is a call for the bonding maintainers to make. If they're
> willing to replace rtnl_dereference with bond_dereference throughout
> the
> whole driver, and reduce other guys' amount of work when other NDOs
> start losing the rtnl_mutex too, then I can't see what's wrong with
> my
> approach (despite not being "as simple"). If they think that update-
> side
> protection of the slaves array is just fine the way it is, then I
> suppose that RCU protection + dev_hold is indeed all that I can do.

To be honest i haven't really looked at your patches, I just quickly
went through them to get an idea of what you did, but let me take a
more careful look and will give my ultimate feedback.

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Saeed Mahameed]

On Fri, 2021-01-08 at 10:14 +0100, Eric Dumazet wrote:
> On Fri, Jan 8, 2021 at 4:59 AM Saeed Mahameed <saeed@kernel.org>
> wrote:
> > Eric, about two years ago you were totally against sleeping in
> > ndo_get_stats, what happened ? :)
> > https://lore.kernel.org/netdev/4cc44e85-cb5e-502c-30f3-c6ea564fe9ac@gmail.com/
> > 
> > My approach to solve this was much simpler and didn't require  a
> > new
> > mutex nor RTNL lock, all i did is to reduce the rcu critical
> > section to
> > not include the call to the driver by simply holding the netdev via
> > dev_hold()
> > 
> 
> Yeah, and how have you dealt with bonding at that time ?
> 

I needed to get the ack on the RFC first, imagine if I'd changed the
whole stack and then got a nack :)

> Look, it seems to me Vladimir's work is more polished.
> 
> If you disagree, repost a rebased patch series so that we can
> test/compare and choose the best solution.
> 

I will need to carefully look at Vladimir's series first.

> And make sure to test it with LOCKDEP enabled ;)
> 

Indeed :)

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Saeed Mahameed]

On Fri, 2021-01-08 at 10:27 +0100, Eric Dumazet wrote:
> On Fri, Jan 8, 2021 at 10:21 AM Vladimir Oltean <olteanv@gmail.com>
> wrote:
> > On Fri, Jan 08, 2021 at 10:14:01AM +0100, Eric Dumazet wrote:
> > > If you disagree, repost a rebased patch series so that we can
> > > test/compare and choose the best solution.
> > 
> > I would rather use Saeed's time as a reviewer to my existing and
> > current
> > patch set.
> 
> Yes, same feeling here, but Saeed brought back his own old
> implementation, so maybe he does not feel the same way ?

Agreed, Vladimir's work is more complete than mine, my work was just a
RFC to show the concept, it was far from being ready or testable with
some use cases such as bonding/bridge/ovs/etc...

Let me take a look at the current series, and if I see that the
rcu/dev_hold approach is more lightweight then i will suggest it to
Vladimir and he can make the final decision.

Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64 can sleep

[Vladimir Oltean]

On Fri, Jan 08, 2021 at 11:38:57AM -0800, Saeed Mahameed wrote:
> Let me take a look at the current series, and if I see that the
> rcu/dev_hold approach is more lightweight then i will suggest it to
> Vladimir and he can make the final decision.

The last version does use temporary RCU protection. I decided to not
change anything about the locking architecture of the drivers.