[net-next PATCH v6 0/7] Symmetric queue selection using XPS for Rx queues

[net-next PATCH v6 0/7] Symmetric queue selection using XPS for Rx queues

[Amritha Nambiar]

This patch series implements support for Tx queue selection based on
Rx queue(s) map. This is done by configuring Rx queue(s) map per Tx-queue
using sysfs attribute. If the user configuration for Rx queues does
not apply, then the Tx queue selection falls back to XPS using CPUs and
finally to hashing.

XPS is refactored to support Tx queue selection based on either the
CPUs map or the Rx-queues map. The config option CONFIG_XPS needs to be
enabled. By default no receive queues are configured for the Tx queue.

- /sys/class/net/<dev>/queues/tx-*/xps_rxqs

A set of receive queues can be mapped to a set of transmit queues (many:many),
although the common use case is a 1:1 mapping. This will enable sending
packets on the same Tx-Rx queue association as this is useful for busy polling
multi-threaded workloads where it is not possible to pin the threads to
a CPU. This is a rework of Sridhar's patch for symmetric queueing via
socket option:
https://www.spinics.net/lists/netdev/msg453106.html

Testing Hints:
Kernel:  Linux 4.17.0-rc7+
Interface:
driver: ixgbe
version: 5.1.0-k
firmware-version: 0x00015e0b

Configuration:
ethtool -L $iface combined 16
ethtool -C $iface rx-usecs 1000
sysctl net.core.busy_poll=1000
ATR disabled:
ethtool -K $iface ntuple on

Workload:
Modified memcached that changes the thread selection policy to be based
on the incoming rx-queue of a connection using SO_INCOMING_NAPI_ID socket
option. The default is round-robin.

Default: No rxqs_map configured
Symmetric queues: Enable rxqs_map for all queues 1:1 mapped to Tx queue

System:
Architecture:          x86_64
CPU(s):                72
Model name:            Intel(R) Xeon(R) CPU E5-2699 v3 @ 2.30GHz

16 threads  400K requests/sec
=============================
-------------------------------------------------------------------------------
                                Default                 Symmetric queues
-------------------------------------------------------------------------------
RTT min/avg/max                 4/51/2215               2/30/5163
(usec)


intr/sec                        26655                   18606

contextswitch/sec               5145                    4044

insn per cycle                  0.43                    0.72

cache-misses                    6.919                   4.310
(% of all cache refs)

L1-dcache-load-                 4.49                    3.29
-misses
(% of all L1-dcache hits)

LLC-load-misses                 13.26                   8.96
(% of all LL-cache hits)

-------------------------------------------------------------------------------

32 threads  400K requests/sec
=============================
-------------------------------------------------------------------------------
                                Default                 Symmetric queues
-------------------------------------------------------------------------------
RTT min/avg/max                 10/112/5562             9/46/4637
(usec)


intr/sec                        30456                   27666

contextswitch/sec               7552                    5133

insn per cycle                  0.41                    0.49

cache-misses                    9.357                   2.769
(% of all cache refs)

L1-dcache-load-                 4.09                    3.98
-misses
(% of all L1-dcache hits)

LLC-load-misses                 12.96                   3.96
(% of all LL-cache hits)

-------------------------------------------------------------------------------

16 threads  800K requests/sec
=============================
-------------------------------------------------------------------------------
                                Default                 Symmetric queues
-------------------------------------------------------------------------------
RTT min/avg/max                  5/151/4989             9/69/2611
(usec)


intr/sec                        35686                   22907

contextswitch/sec               25522                   12281

insn per cycle                  0.67                    0.74

cache-misses                    8.652                   6.38
(% of all cache refs)

L1-dcache-load-                 3.19                    2.86
-misses
(% of all L1-dcache hits)

LLC-load-misses                 16.53                   11.99
(% of all LL-cache hits)

-------------------------------------------------------------------------------
32 threads  800K requests/sec
=============================
-------------------------------------------------------------------------------
                                Default                 Symmetric queues
-------------------------------------------------------------------------------
RTT min/avg/max                  6/163/6152             8/88/4209
(usec)


intr/sec                        47079                   26548

contextswitch/sec               42190                   39168

insn per cycle                  0.45                    0.54

cache-misses                    8.798                   4.668
(% of all cache refs)

L1-dcache-load-                 6.55                    6.29
-misses
(% of all L1-dcache hits)

LLC-load-misses                 13.91                   10.44
(% of all LL-cache hits)

-------------------------------------------------------------------------------

v6:
- Changed the names of some functions to begin with net_if.
- Cleaned up sk_tx_queue_set/sk_rx_queue_set functions.
- Added sk_rx_queue_clear to make it consistent with tx_queue_mapping
  initialization.

---

Amritha Nambiar (7):
      net: Refactor XPS for CPUs and Rx queues
      net: Use static_key for XPS maps
      net: sock: Change tx_queue_mapping in sock_common to unsigned short
      net: Record receive queue number for a connection
      net: Enable Tx queue selection based on Rx queues
      net-sysfs: Add interface for Rx queue(s) map per Tx queue
      Documentation: Add explanation for XPS using Rx-queue(s) map


 Documentation/ABI/testing/sysfs-class-net-queues |   11 +
 Documentation/networking/scaling.txt             |   61 ++++-
 include/linux/cpumask.h                          |   11 +
 include/linux/netdevice.h                        |   98 +++++++
 include/net/busy_poll.h                          |    1 
 include/net/sock.h                               |   52 ++++
 net/core/dev.c                                   |  288 +++++++++++++++-------
 net/core/net-sysfs.c                             |   87 ++++++-
 net/core/sock.c                                  |    2 
 net/ipv4/tcp_input.c                             |    3 
 10 files changed, 505 insertions(+), 109 deletions(-)

[net-next PATCH v6 1/7] net: Refactor XPS for CPUs and Rx queues

[Amritha Nambiar]

Refactor XPS code to support Tx queue selection based on
CPU(s) map or Rx queue(s) map.

Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
---
 include/linux/cpumask.h   |   11 ++
 include/linux/netdevice.h |   98 ++++++++++++++++++++-
 net/core/dev.c            |  211 ++++++++++++++++++++++++++++++---------------
 net/core/net-sysfs.c      |    4 -
 4 files changed, 244 insertions(+), 80 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index bf53d89..57f20a0 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -115,12 +115,17 @@ extern struct cpumask __cpu_active_mask;
 #define cpu_active(cpu)		((cpu) == 0)
 #endif
 
-/* verify cpu argument to cpumask_* operators */
-static inline unsigned int cpumask_check(unsigned int cpu)
+static inline void cpu_max_bits_warn(unsigned int cpu, unsigned int bits)
 {
 #ifdef CONFIG_DEBUG_PER_CPU_MAPS
-	WARN_ON_ONCE(cpu >= nr_cpumask_bits);
+	WARN_ON_ONCE(cpu >= bits);
 #endif /* CONFIG_DEBUG_PER_CPU_MAPS */
+}
+
+/* verify cpu argument to cpumask_* operators */
+static inline unsigned int cpumask_check(unsigned int cpu)
+{
+	cpu_max_bits_warn(cpu, nr_cpumask_bits);
 	return cpu;
 }
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c6b377a..8bf8d61 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -731,10 +731,15 @@ struct xps_map {
  */
 struct xps_dev_maps {
 	struct rcu_head rcu;
-	struct xps_map __rcu *cpu_map[0];
+	struct xps_map __rcu *attr_map[0]; /* Either CPUs map or RXQs map */
 };
-#define XPS_DEV_MAPS_SIZE(_tcs) (sizeof(struct xps_dev_maps) +		\
+
+#define XPS_CPU_DEV_MAPS_SIZE(_tcs) (sizeof(struct xps_dev_maps) +	\
 	(nr_cpu_ids * (_tcs) * sizeof(struct xps_map *)))
+
+#define XPS_RXQ_DEV_MAPS_SIZE(_tcs, _rxqs) (sizeof(struct xps_dev_maps) +\
+	(_rxqs * (_tcs) * sizeof(struct xps_map *)))
+
 #endif /* CONFIG_XPS */
 
 #define TC_MAX_QUEUE	16
@@ -1910,7 +1915,8 @@ struct net_device {
 	int			watchdog_timeo;
 
 #ifdef CONFIG_XPS
-	struct xps_dev_maps __rcu *xps_maps;
+	struct xps_dev_maps __rcu *xps_cpus_map;
+	struct xps_dev_maps __rcu *xps_rxqs_map;
 #endif
 #ifdef CONFIG_NET_CLS_ACT
 	struct mini_Qdisc __rcu	*miniq_egress;
@@ -3259,6 +3265,92 @@ static inline void netif_wake_subqueue(struct net_device *dev, u16 queue_index)
 #ifdef CONFIG_XPS
 int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
 			u16 index);
+int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
+			  u16 index, bool is_rxqs_map);
+
+/**
+ *	netif_attr_test_mask - Test a CPU or Rx queue set in a mask
+ *	@j: CPU/Rx queue index
+ *	@mask: bitmask of all cpus/rx queues
+ *	@nr_bits: number of bits in the bitmask
+ *
+ * Test if a CPU or Rx queue index is set in a mask of all CPU/Rx queues.
+ */
+static inline bool netif_attr_test_mask(unsigned long j,
+					const unsigned long *mask,
+					unsigned int nr_bits)
+{
+	cpu_max_bits_warn(j, nr_bits);
+	return test_bit(j, mask);
+}
+
+/**
+ *	netif_attr_test_online - Test for online CPU/Rx queue
+ *	@j: CPU/Rx queue index
+ *	@online_mask: bitmask for CPUs/Rx queues that are online
+ *	@nr_bits: number of bits in the bitmask
+ *
+ * Returns true if a CPU/Rx queue is online.
+ */
+static inline bool netif_attr_test_online(unsigned long j,
+					  const unsigned long *online_mask,
+					  unsigned int nr_bits)
+{
+	cpu_max_bits_warn(j, nr_bits);
+
+	if (online_mask)
+		return test_bit(j, online_mask);
+
+	return (j < nr_bits);
+}
+
+/**
+ *	netif_attrmask_next - get the next CPU/Rx queue in a cpu/Rx queues mask
+ *	@n: CPU/Rx queue index
+ *	@srcp: the cpumask/Rx queue mask pointer
+ *	@nr_bits: number of bits in the bitmask
+ *
+ * Returns >= nr_bits if no further CPUs/Rx queues set.
+ */
+static inline unsigned int netif_attrmask_next(int n, const unsigned long *srcp,
+					       unsigned int nr_bits)
+{
+	/* -1 is a legal arg here. */
+	if (n != -1)
+		cpu_max_bits_warn(n, nr_bits);
+
+	if (srcp)
+		return find_next_bit(srcp, nr_bits, n + 1);
+
+	return n + 1;
+}
+
+/**
+ *	netif_attrmask_next_and - get the next CPU/Rx queue in *src1p & *src2p
+ *	@n: CPU/Rx queue index
+ *	@src1p: the first CPUs/Rx queues mask pointer
+ *	@src2p: the second CPUs/Rx queues mask pointer
+ *	@nr_bits: number of bits in the bitmask
+ *
+ * Returns >= nr_bits if no further CPUs/Rx queues set in both.
+ */
+static inline int netif_attrmask_next_and(int n, const unsigned long *src1p,
+					  const unsigned long *src2p,
+					  unsigned int nr_bits)
+{
+	/* -1 is a legal arg here. */
+	if (n != -1)
+		cpu_max_bits_warn(n, nr_bits);
+
+	if (src1p && src2p)
+		return find_next_and_bit(src1p, src2p, nr_bits, n + 1);
+	else if (src1p)
+		return find_next_bit(src1p, nr_bits, n + 1);
+	else if (src2p)
+		return find_next_bit(src2p, nr_bits, n + 1);
+
+	return n + 1;
+}
 #else
 static inline int netif_set_xps_queue(struct net_device *dev,
 				      const struct cpumask *mask,
diff --git a/net/core/dev.c b/net/core/dev.c
index dffed64..7105955 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2092,7 +2092,7 @@ static bool remove_xps_queue(struct xps_dev_maps *dev_maps,
 	int pos;
 
 	if (dev_maps)
-		map = xmap_dereference(dev_maps->cpu_map[tci]);
+		map = xmap_dereference(dev_maps->attr_map[tci]);
 	if (!map)
 		return false;
 
@@ -2105,7 +2105,7 @@ static bool remove_xps_queue(struct xps_dev_maps *dev_maps,
 			break;
 		}
 
-		RCU_INIT_POINTER(dev_maps->cpu_map[tci], NULL);
+		RCU_INIT_POINTER(dev_maps->attr_map[tci], NULL);
 		kfree_rcu(map, rcu);
 		return false;
 	}
@@ -2135,31 +2135,58 @@ static bool remove_xps_queue_cpu(struct net_device *dev,
 	return active;
 }
 
+static void clean_xps_maps(struct net_device *dev, const unsigned long *mask,
+			   struct xps_dev_maps *dev_maps, unsigned int nr_ids,
+			   u16 offset, u16 count, bool is_rxqs_map)
+{
+	bool active = false;
+	int i, j;
+
+	for (j = -1; j = netif_attrmask_next(j, mask, nr_ids),
+	     j < nr_ids;)
+		active |= remove_xps_queue_cpu(dev, dev_maps, j, offset,
+					       count);
+	if (!active) {
+		if (is_rxqs_map) {
+			RCU_INIT_POINTER(dev->xps_rxqs_map, NULL);
+		} else {
+			RCU_INIT_POINTER(dev->xps_cpus_map, NULL);
+
+			for (i = offset + (count - 1); count--; i--)
+				netdev_queue_numa_node_write(
+					netdev_get_tx_queue(dev, i),
+							NUMA_NO_NODE);
+		}
+		kfree_rcu(dev_maps, rcu);
+	}
+}
+
 static void netif_reset_xps_queues(struct net_device *dev, u16 offset,
 				   u16 count)
 {
+	const unsigned long *possible_mask = NULL;
 	struct xps_dev_maps *dev_maps;
-	int cpu, i;
-	bool active = false;
+	unsigned int nr_ids;
 
 	mutex_lock(&xps_map_mutex);
-	dev_maps = xmap_dereference(dev->xps_maps);
 
-	if (!dev_maps)
-		goto out_no_maps;
-
-	for_each_possible_cpu(cpu)
-		active |= remove_xps_queue_cpu(dev, dev_maps, cpu,
-					       offset, count);
+	dev_maps = xmap_dereference(dev->xps_rxqs_map);
+	if (dev_maps) {
+		nr_ids = dev->num_rx_queues;
+		clean_xps_maps(dev, possible_mask, dev_maps, nr_ids, offset,
+			       count, true);
 
-	if (!active) {
-		RCU_INIT_POINTER(dev->xps_maps, NULL);
-		kfree_rcu(dev_maps, rcu);
 	}
 
-	for (i = offset + (count - 1); count--; i--)
-		netdev_queue_numa_node_write(netdev_get_tx_queue(dev, i),
-					     NUMA_NO_NODE);
+	dev_maps = xmap_dereference(dev->xps_cpus_map);
+	if (!dev_maps)
+		goto out_no_maps;
+
+	if (num_possible_cpus() > 1)
+		possible_mask = cpumask_bits(cpu_possible_mask);
+	nr_ids = nr_cpu_ids;
+	clean_xps_maps(dev, possible_mask, dev_maps, nr_ids, offset, count,
+		       false);
 
 out_no_maps:
 	mutex_unlock(&xps_map_mutex);
@@ -2170,8 +2197,8 @@ static void netif_reset_xps_queues_gt(struct net_device *dev, u16 index)
 	netif_reset_xps_queues(dev, index, dev->num_tx_queues - index);
 }
 
-static struct xps_map *expand_xps_map(struct xps_map *map,
-				      int cpu, u16 index)
+static struct xps_map *expand_xps_map(struct xps_map *map, int attr_index,
+				      u16 index, bool is_rxqs_map)
 {
 	struct xps_map *new_map;
 	int alloc_len = XPS_MIN_MAP_ALLOC;
@@ -2183,7 +2210,7 @@ static struct xps_map *expand_xps_map(struct xps_map *map,
 		return map;
 	}
 
-	/* Need to add queue to this CPU's existing map */
+	/* Need to add tx-queue to this CPU's/rx-queue's existing map */
 	if (map) {
 		if (pos < map->alloc_len)
 			return map;
@@ -2191,9 +2218,14 @@ static struct xps_map *expand_xps_map(struct xps_map *map,
 		alloc_len = map->alloc_len * 2;
 	}
 
-	/* Need to allocate new map to store queue on this CPU's map */
-	new_map = kzalloc_node(XPS_MAP_SIZE(alloc_len), GFP_KERNEL,
-			       cpu_to_node(cpu));
+	/* Need to allocate new map to store tx-queue on this CPU's/rx-queue's
+	 *  map
+	 */
+	if (is_rxqs_map)
+		new_map = kzalloc(XPS_MAP_SIZE(alloc_len), GFP_KERNEL);
+	else
+		new_map = kzalloc_node(XPS_MAP_SIZE(alloc_len), GFP_KERNEL,
+				       cpu_to_node(attr_index));
 	if (!new_map)
 		return NULL;
 
@@ -2205,14 +2237,16 @@ static struct xps_map *expand_xps_map(struct xps_map *map,
 	return new_map;
 }
 
-int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
-			u16 index)
+int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
+			  u16 index, bool is_rxqs_map)
 {
+	const unsigned long *online_mask = NULL, *possible_mask = NULL;
 	struct xps_dev_maps *dev_maps, *new_dev_maps = NULL;
-	int i, cpu, tci, numa_node_id = -2;
+	int i, j, tci, numa_node_id = -2;
 	int maps_sz, num_tc = 1, tc = 0;
 	struct xps_map *map, *new_map;
 	bool active = false;
+	unsigned int nr_ids;
 
 	if (dev->num_tc) {
 		num_tc = dev->num_tc;
@@ -2221,16 +2255,27 @@ int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
 			return -EINVAL;
 	}
 
-	maps_sz = XPS_DEV_MAPS_SIZE(num_tc);
-	if (maps_sz < L1_CACHE_BYTES)
-		maps_sz = L1_CACHE_BYTES;
-
 	mutex_lock(&xps_map_mutex);
+	if (is_rxqs_map) {
+		maps_sz = XPS_RXQ_DEV_MAPS_SIZE(num_tc, dev->num_rx_queues);
+		dev_maps = xmap_dereference(dev->xps_rxqs_map);
+		nr_ids = dev->num_rx_queues;
+	} else {
+		maps_sz = XPS_CPU_DEV_MAPS_SIZE(num_tc);
+		if (num_possible_cpus() > 1) {
+			online_mask = cpumask_bits(cpu_online_mask);
+			possible_mask = cpumask_bits(cpu_possible_mask);
+		}
+		dev_maps = xmap_dereference(dev->xps_cpus_map);
+		nr_ids = nr_cpu_ids;
+	}
 
-	dev_maps = xmap_dereference(dev->xps_maps);
+	if (maps_sz < L1_CACHE_BYTES)
+		maps_sz = L1_CACHE_BYTES;
 
 	/* allocate memory for queue storage */
-	for_each_cpu_and(cpu, cpu_online_mask, mask) {
+	for (j = -1; j = netif_attrmask_next_and(j, online_mask, mask, nr_ids),
+	     j < nr_ids;) {
 		if (!new_dev_maps)
 			new_dev_maps = kzalloc(maps_sz, GFP_KERNEL);
 		if (!new_dev_maps) {
@@ -2238,73 +2283,81 @@ int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
 			return -ENOMEM;
 		}
 
-		tci = cpu * num_tc + tc;
-		map = dev_maps ? xmap_dereference(dev_maps->cpu_map[tci]) :
+		tci = j * num_tc + tc;
+		map = dev_maps ? xmap_dereference(dev_maps->attr_map[tci]) :
 				 NULL;
 
-		map = expand_xps_map(map, cpu, index);
+		map = expand_xps_map(map, j, index, is_rxqs_map);
 		if (!map)
 			goto error;
 
-		RCU_INIT_POINTER(new_dev_maps->cpu_map[tci], map);
+		RCU_INIT_POINTER(new_dev_maps->attr_map[tci], map);
 	}
 
 	if (!new_dev_maps)
 		goto out_no_new_maps;
 
-	for_each_possible_cpu(cpu) {
+	for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids),
+	     j < nr_ids;) {
 		/* copy maps belonging to foreign traffic classes */
-		for (i = tc, tci = cpu * num_tc; dev_maps && i--; tci++) {
+		for (i = tc, tci = j * num_tc; dev_maps && i--; tci++) {
 			/* fill in the new device map from the old device map */
-			map = xmap_dereference(dev_maps->cpu_map[tci]);
-			RCU_INIT_POINTER(new_dev_maps->cpu_map[tci], map);
+			map = xmap_dereference(dev_maps->attr_map[tci]);
+			RCU_INIT_POINTER(new_dev_maps->attr_map[tci], map);
 		}
 
 		/* We need to explicitly update tci as prevous loop
 		 * could break out early if dev_maps is NULL.
 		 */
-		tci = cpu * num_tc + tc;
+		tci = j * num_tc + tc;
 
-		if (cpumask_test_cpu(cpu, mask) && cpu_online(cpu)) {
-			/* add queue to CPU maps */
+		if (netif_attr_test_mask(j, mask, nr_ids) &&
+		    netif_attr_test_online(j, online_mask, nr_ids)) {
+			/* add tx-queue to CPU/rx-queue maps */
 			int pos = 0;
 
-			map = xmap_dereference(new_dev_maps->cpu_map[tci]);
+			map = xmap_dereference(new_dev_maps->attr_map[tci]);
 			while ((pos < map->len) && (map->queues[pos] != index))
 				pos++;
 
 			if (pos == map->len)
 				map->queues[map->len++] = index;
 #ifdef CONFIG_NUMA
-			if (numa_node_id == -2)
-				numa_node_id = cpu_to_node(cpu);
-			else if (numa_node_id != cpu_to_node(cpu))
-				numa_node_id = -1;
+			if (!is_rxqs_map) {
+				if (numa_node_id == -2)
+					numa_node_id = cpu_to_node(j);
+				else if (numa_node_id != cpu_to_node(j))
+					numa_node_id = -1;
+			}
 #endif
 		} else if (dev_maps) {
 			/* fill in the new device map from the old device map */
-			map = xmap_dereference(dev_maps->cpu_map[tci]);
-			RCU_INIT_POINTER(new_dev_maps->cpu_map[tci], map);
+			map = xmap_dereference(dev_maps->attr_map[tci]);
+			RCU_INIT_POINTER(new_dev_maps->attr_map[tci], map);
 		}
 
 		/* copy maps belonging to foreign traffic classes */
 		for (i = num_tc - tc, tci++; dev_maps && --i; tci++) {
 			/* fill in the new device map from the old device map */
-			map = xmap_dereference(dev_maps->cpu_map[tci]);
-			RCU_INIT_POINTER(new_dev_maps->cpu_map[tci], map);
+			map = xmap_dereference(dev_maps->attr_map[tci]);
+			RCU_INIT_POINTER(new_dev_maps->attr_map[tci], map);
 		}
 	}
 
-	rcu_assign_pointer(dev->xps_maps, new_dev_maps);
+	if (is_rxqs_map)
+		rcu_assign_pointer(dev->xps_rxqs_map, new_dev_maps);
+	else
+		rcu_assign_pointer(dev->xps_cpus_map, new_dev_maps);
 
 	/* Cleanup old maps */
 	if (!dev_maps)
 		goto out_no_old_maps;
 
-	for_each_possible_cpu(cpu) {
-		for (i = num_tc, tci = cpu * num_tc; i--; tci++) {
-			new_map = xmap_dereference(new_dev_maps->cpu_map[tci]);
-			map = xmap_dereference(dev_maps->cpu_map[tci]);
+	for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids),
+	     j < nr_ids;) {
+		for (i = num_tc, tci = j * num_tc; i--; tci++) {
+			new_map = xmap_dereference(new_dev_maps->attr_map[tci]);
+			map = xmap_dereference(dev_maps->attr_map[tci]);
 			if (map && map != new_map)
 				kfree_rcu(map, rcu);
 		}
@@ -2317,19 +2370,23 @@ int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
 	active = true;
 
 out_no_new_maps:
-	/* update Tx queue numa node */
-	netdev_queue_numa_node_write(netdev_get_tx_queue(dev, index),
-				     (numa_node_id >= 0) ? numa_node_id :
-				     NUMA_NO_NODE);
+	if (!is_rxqs_map) {
+		/* update Tx queue numa node */
+		netdev_queue_numa_node_write(netdev_get_tx_queue(dev, index),
+					     (numa_node_id >= 0) ?
+					     numa_node_id : NUMA_NO_NODE);
+	}
 
 	if (!dev_maps)
 		goto out_no_maps;
 
-	/* removes queue from unused CPUs */
-	for_each_possible_cpu(cpu) {
-		for (i = tc, tci = cpu * num_tc; i--; tci++)
+	/* removes tx-queue from unused CPUs/rx-queues */
+	for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids),
+	     j < nr_ids;) {
+		for (i = tc, tci = j * num_tc; i--; tci++)
 			active |= remove_xps_queue(dev_maps, tci, index);
-		if (!cpumask_test_cpu(cpu, mask) || !cpu_online(cpu))
+		if (!netif_attr_test_mask(j, mask, nr_ids) ||
+		    !netif_attr_test_online(j, online_mask, nr_ids))
 			active |= remove_xps_queue(dev_maps, tci, index);
 		for (i = num_tc - tc, tci++; --i; tci++)
 			active |= remove_xps_queue(dev_maps, tci, index);
@@ -2337,7 +2394,10 @@ int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
 
 	/* free map if not active */
 	if (!active) {
-		RCU_INIT_POINTER(dev->xps_maps, NULL);
+		if (is_rxqs_map)
+			RCU_INIT_POINTER(dev->xps_rxqs_map, NULL);
+		else
+			RCU_INIT_POINTER(dev->xps_cpus_map, NULL);
 		kfree_rcu(dev_maps, rcu);
 	}
 
@@ -2347,11 +2407,12 @@ int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
 	return 0;
 error:
 	/* remove any maps that we added */
-	for_each_possible_cpu(cpu) {
-		for (i = num_tc, tci = cpu * num_tc; i--; tci++) {
-			new_map = xmap_dereference(new_dev_maps->cpu_map[tci]);
+	for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids),
+	     j < nr_ids;) {
+		for (i = num_tc, tci = j * num_tc; i--; tci++) {
+			new_map = xmap_dereference(new_dev_maps->attr_map[tci]);
 			map = dev_maps ?
-			      xmap_dereference(dev_maps->cpu_map[tci]) :
+			      xmap_dereference(dev_maps->attr_map[tci]) :
 			      NULL;
 			if (new_map && new_map != map)
 				kfree(new_map);
@@ -2363,6 +2424,12 @@ int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
 	kfree(new_dev_maps);
 	return -ENOMEM;
 }
+
+int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
+			u16 index)
+{
+	return __netif_set_xps_queue(dev, cpumask_bits(mask), index, false);
+}
 EXPORT_SYMBOL(netif_set_xps_queue);
 
 #endif
@@ -3384,7 +3451,7 @@ static inline int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
 	int queue_index = -1;
 
 	rcu_read_lock();
-	dev_maps = rcu_dereference(dev->xps_maps);
+	dev_maps = rcu_dereference(dev->xps_cpus_map);
 	if (dev_maps) {
 		unsigned int tci = skb->sender_cpu - 1;
 
@@ -3393,7 +3460,7 @@ static inline int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
 			tci += netdev_get_prio_tc_map(dev, skb->priority);
 		}
 
-		map = rcu_dereference(dev_maps->cpu_map[tci]);
+		map = rcu_dereference(dev_maps->attr_map[tci]);
 		if (map) {
 			if (map->len == 1)
 				queue_index = map->queues[0];
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index bb7e80f..b39987c 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1227,13 +1227,13 @@ static ssize_t xps_cpus_show(struct netdev_queue *queue,
 		return -ENOMEM;
 
 	rcu_read_lock();
-	dev_maps = rcu_dereference(dev->xps_maps);
+	dev_maps = rcu_dereference(dev->xps_cpus_map);
 	if (dev_maps) {
 		for_each_possible_cpu(cpu) {
 			int i, tci = cpu * num_tc + tc;
 			struct xps_map *map;
 
-			map = rcu_dereference(dev_maps->cpu_map[tci]);
+			map = rcu_dereference(dev_maps->attr_map[tci]);
 			if (!map)
 				continue;
 

[net-next PATCH v6 2/7] net: Use static_key for XPS maps

[Amritha Nambiar]

Use static_key for XPS maps to reduce the cost of extra map checks,
similar to how it is used for RPS and RFS. This includes static_key
'xps_needed' for XPS and another for 'xps_rxqs_needed' for XPS using
Rx queues map.

Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
---
 net/core/dev.c |   31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 7105955..43b5575 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2081,6 +2081,10 @@ int netdev_txq_to_tc(struct net_device *dev, unsigned int txq)
 EXPORT_SYMBOL(netdev_txq_to_tc);
 
 #ifdef CONFIG_XPS
+struct static_key xps_needed __read_mostly;
+EXPORT_SYMBOL(xps_needed);
+struct static_key xps_rxqs_needed __read_mostly;
+EXPORT_SYMBOL(xps_rxqs_needed);
 static DEFINE_MUTEX(xps_map_mutex);
 #define xmap_dereference(P)		\
 	rcu_dereference_protected((P), lockdep_is_held(&xps_map_mutex))
@@ -2168,14 +2172,18 @@ static void netif_reset_xps_queues(struct net_device *dev, u16 offset,
 	struct xps_dev_maps *dev_maps;
 	unsigned int nr_ids;
 
-	mutex_lock(&xps_map_mutex);
+	if (!static_key_false(&xps_needed))
+		return;
 
-	dev_maps = xmap_dereference(dev->xps_rxqs_map);
-	if (dev_maps) {
-		nr_ids = dev->num_rx_queues;
-		clean_xps_maps(dev, possible_mask, dev_maps, nr_ids, offset,
-			       count, true);
+	mutex_lock(&xps_map_mutex);
 
+	if (static_key_false(&xps_rxqs_needed)) {
+		dev_maps = xmap_dereference(dev->xps_rxqs_map);
+		if (dev_maps) {
+			nr_ids = dev->num_rx_queues;
+			clean_xps_maps(dev, possible_mask, dev_maps, nr_ids,
+				       offset, count, true);
+		}
 	}
 
 	dev_maps = xmap_dereference(dev->xps_cpus_map);
@@ -2189,6 +2197,10 @@ static void netif_reset_xps_queues(struct net_device *dev, u16 offset,
 		       false);
 
 out_no_maps:
+	if (static_key_enabled(&xps_rxqs_needed))
+		static_key_slow_dec(&xps_rxqs_needed);
+
+	static_key_slow_dec(&xps_needed);
 	mutex_unlock(&xps_map_mutex);
 }
 
@@ -2297,6 +2309,10 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
 	if (!new_dev_maps)
 		goto out_no_new_maps;
 
+	static_key_slow_inc(&xps_needed);
+	if (is_rxqs_map)
+		static_key_slow_inc(&xps_rxqs_needed);
+
 	for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids),
 	     j < nr_ids;) {
 		/* copy maps belonging to foreign traffic classes */
@@ -3450,6 +3466,9 @@ static inline int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
 	struct xps_map *map;
 	int queue_index = -1;
 
+	if (!static_key_false(&xps_needed))
+		return -1;
+
 	rcu_read_lock();
 	dev_maps = rcu_dereference(dev->xps_cpus_map);
 	if (dev_maps) {

[net-next PATCH v6 3/7] net: sock: Change tx_queue_mapping in sock_common to unsigned short

[Amritha Nambiar]

Change 'skc_tx_queue_mapping' field in sock_common structure from
'int' to 'unsigned short' type with ~0 indicating unset and
other positive queue values being set. This will accommodate adding
a new 'unsigned short' field in sock_common in the next patch for
rx_queue_mapping.

Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
---
 include/net/sock.h |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index b3b7541..37b09c8 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -214,7 +214,7 @@ struct sock_common {
 		struct hlist_node	skc_node;
 		struct hlist_nulls_node skc_nulls_node;
 	};
-	int			skc_tx_queue_mapping;
+	unsigned short		skc_tx_queue_mapping;
 	union {
 		int		skc_incoming_cpu;
 		u32		skc_rcv_wnd;
@@ -1681,17 +1681,25 @@ static inline int sk_receive_skb(struct sock *sk, struct sk_buff *skb,
 
 static inline void sk_tx_queue_set(struct sock *sk, int tx_queue)
 {
+	/* sk_tx_queue_mapping accept only upto a 16-bit value */
+	if (WARN_ON_ONCE((unsigned short)tx_queue >= USHRT_MAX))
+		return;
 	sk->sk_tx_queue_mapping = tx_queue;
 }
 
+#define NO_QUEUE_MAPPING	USHRT_MAX
+
 static inline void sk_tx_queue_clear(struct sock *sk)
 {
-	sk->sk_tx_queue_mapping = -1;
+	sk->sk_tx_queue_mapping = NO_QUEUE_MAPPING;
 }
 
 static inline int sk_tx_queue_get(const struct sock *sk)
 {
-	return sk ? sk->sk_tx_queue_mapping : -1;
+	if (sk && sk->sk_tx_queue_mapping != NO_QUEUE_MAPPING)
+		return sk->sk_tx_queue_mapping;
+
+	return -1;
 }
 
 static inline void sk_set_socket(struct sock *sk, struct socket *sock)

[net-next PATCH v6 4/7] net: Record receive queue number for a connection

[Amritha Nambiar]

This patch adds a new field to sock_common 'skc_rx_queue_mapping'
which holds the receive queue number for the connection. The Rx queue
is marked in tcp_finish_connect() to allow a client app to do
SO_INCOMING_NAPI_ID after a connect() call to get the right queue
association for a socket. Rx queue is also marked in tcp_conn_request()
to allow syn-ack to go on the right tx-queue associated with
the queue on which syn is received.

Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
 include/net/busy_poll.h |    1 +
 include/net/sock.h      |   28 ++++++++++++++++++++++++++++
 net/core/sock.c         |    2 ++
 net/ipv4/tcp_input.c    |    3 +++
 4 files changed, 34 insertions(+)

diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h
index c518743..9e36fda6 100644
--- a/include/net/busy_poll.h
+++ b/include/net/busy_poll.h
@@ -151,6 +151,7 @@ static inline void sk_mark_napi_id(struct sock *sk, const struct sk_buff *skb)
 #ifdef CONFIG_NET_RX_BUSY_POLL
 	sk->sk_napi_id = skb->napi_id;
 #endif
+	sk_rx_queue_set(sk, skb);
 }
 
 /* variant used for unconnected sockets */
diff --git a/include/net/sock.h b/include/net/sock.h
index 37b09c8..2b097cc 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -139,6 +139,7 @@ typedef __u64 __bitwise __addrpair;
  *	@skc_node: main hash linkage for various protocol lookup tables
  *	@skc_nulls_node: main hash linkage for TCP/UDP/UDP-Lite protocol
  *	@skc_tx_queue_mapping: tx queue number for this connection
+ *	@skc_rx_queue_mapping: rx queue number for this connection
  *	@skc_flags: place holder for sk_flags
  *		%SO_LINGER (l_onoff), %SO_BROADCAST, %SO_KEEPALIVE,
  *		%SO_OOBINLINE settings, %SO_TIMESTAMPING settings
@@ -215,6 +216,9 @@ struct sock_common {
 		struct hlist_nulls_node skc_nulls_node;
 	};
 	unsigned short		skc_tx_queue_mapping;
+#ifdef CONFIG_XPS
+	unsigned short		skc_rx_queue_mapping;
+#endif
 	union {
 		int		skc_incoming_cpu;
 		u32		skc_rcv_wnd;
@@ -326,6 +330,9 @@ struct sock {
 #define sk_nulls_node		__sk_common.skc_nulls_node
 #define sk_refcnt		__sk_common.skc_refcnt
 #define sk_tx_queue_mapping	__sk_common.skc_tx_queue_mapping
+#ifdef CONFIG_XPS
+#define sk_rx_queue_mapping	__sk_common.skc_rx_queue_mapping
+#endif
 
 #define sk_dontcopy_begin	__sk_common.skc_dontcopy_begin
 #define sk_dontcopy_end		__sk_common.skc_dontcopy_end
@@ -1702,6 +1709,27 @@ static inline int sk_tx_queue_get(const struct sock *sk)
 	return -1;
 }
 
+static inline void sk_rx_queue_set(struct sock *sk, const struct sk_buff *skb)
+{
+#ifdef CONFIG_XPS
+	if (skb_rx_queue_recorded(skb)) {
+		u16 rx_queue = skb_get_rx_queue(skb);
+
+		if (WARN_ON_ONCE(rx_queue == NO_QUEUE_MAPPING))
+			return;
+
+		sk->sk_rx_queue_mapping = rx_queue;
+	}
+#endif
+}
+
+static inline void sk_rx_queue_clear(struct sock *sk)
+{
+#ifdef CONFIG_XPS
+	sk->sk_rx_queue_mapping = NO_QUEUE_MAPPING;
+#endif
+}
+
 static inline void sk_set_socket(struct sock *sk, struct socket *sock)
 {
 	sk_tx_queue_clear(sk);
diff --git a/net/core/sock.c b/net/core/sock.c
index bcc4182..dac6d78 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2818,6 +2818,8 @@ void sock_init_data(struct socket *sock, struct sock *sk)
 	sk->sk_pacing_rate = ~0U;
 	sk->sk_pacing_shift = 10;
 	sk->sk_incoming_cpu = -1;
+
+	sk_rx_queue_clear(sk);
 	/*
 	 * Before updating sk_refcnt, we must commit prior changes to memory
 	 * (Documentation/RCU/rculist_nulls.txt for details)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 9c5b341..b3b5aef 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -78,6 +78,7 @@
 #include <linux/errqueue.h>
 #include <trace/events/tcp.h>
 #include <linux/static_key.h>
+#include <net/busy_poll.h>
 
 int sysctl_tcp_max_orphans __read_mostly = NR_FILE;
 
@@ -5588,6 +5589,7 @@ void tcp_finish_connect(struct sock *sk, struct sk_buff *skb)
 	if (skb) {
 		icsk->icsk_af_ops->sk_rx_dst_set(sk, skb);
 		security_inet_conn_established(sk, skb);
+		sk_mark_napi_id(sk, skb);
 	}
 
 	tcp_init_transfer(sk, BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB);
@@ -6416,6 +6418,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	tcp_rsk(req)->snt_isn = isn;
 	tcp_rsk(req)->txhash = net_tx_rndhash();
 	tcp_openreq_init_rwin(req, sk, dst);
+	sk_rx_queue_set(req_to_sk(req), skb);
 	if (!want_cookie) {
 		tcp_reqsk_record_syn(sk, req, skb);
 		fastopen_sk = tcp_try_fastopen(sk, skb, req, &foc, dst);

[net-next PATCH v6 5/7] net: Enable Tx queue selection based on Rx queues

[Amritha Nambiar]

This patch adds support to pick Tx queue based on the Rx queue(s) map
configuration set by the admin through the sysfs attribute
for each Tx queue. If the user configuration for receive queue(s) map
does not apply, then the Tx queue selection falls back to CPU(s) map
based selection and finally to hashing.

Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
---
 include/net/sock.h |   10 ++++++++
 net/core/dev.c     |   62 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 2b097cc..2ed99bf 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1730,6 +1730,16 @@ static inline void sk_rx_queue_clear(struct sock *sk)
 #endif
 }
 
+#ifdef CONFIG_XPS
+static inline int sk_rx_queue_get(const struct sock *sk)
+{
+	if (sk && sk->sk_rx_queue_mapping != NO_QUEUE_MAPPING)
+		return sk->sk_rx_queue_mapping;
+
+	return -1;
+}
+#endif
+
 static inline void sk_set_socket(struct sock *sk, struct socket *sock)
 {
 	sk_tx_queue_clear(sk);
diff --git a/net/core/dev.c b/net/core/dev.c
index 43b5575..08d58e0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3459,35 +3459,63 @@ sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
 }
 #endif /* CONFIG_NET_EGRESS */
 
-static inline int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
+#ifdef CONFIG_XPS
+static int __get_xps_queue_idx(struct net_device *dev, struct sk_buff *skb,
+			       struct xps_dev_maps *dev_maps, unsigned int tci)
+{
+	struct xps_map *map;
+	int queue_index = -1;
+
+	if (dev->num_tc) {
+		tci *= dev->num_tc;
+		tci += netdev_get_prio_tc_map(dev, skb->priority);
+	}
+
+	map = rcu_dereference(dev_maps->attr_map[tci]);
+	if (map) {
+		if (map->len == 1)
+			queue_index = map->queues[0];
+		else
+			queue_index = map->queues[reciprocal_scale(
+						skb_get_hash(skb), map->len)];
+		if (unlikely(queue_index >= dev->real_num_tx_queues))
+			queue_index = -1;
+	}
+	return queue_index;
+}
+#endif
+
+static int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
 {
 #ifdef CONFIG_XPS
 	struct xps_dev_maps *dev_maps;
-	struct xps_map *map;
+	struct sock *sk = skb->sk;
 	int queue_index = -1;
 
 	if (!static_key_false(&xps_needed))
 		return -1;
 
 	rcu_read_lock();
-	dev_maps = rcu_dereference(dev->xps_cpus_map);
+	if (!static_key_false(&xps_rxqs_needed))
+		goto get_cpus_map;
+
+	dev_maps = rcu_dereference(dev->xps_rxqs_map);
 	if (dev_maps) {
-		unsigned int tci = skb->sender_cpu - 1;
+		int tci = sk_rx_queue_get(sk);
 
-		if (dev->num_tc) {
-			tci *= dev->num_tc;
-			tci += netdev_get_prio_tc_map(dev, skb->priority);
-		}
+		if (tci >= 0 && tci < dev->num_rx_queues)
+			queue_index = __get_xps_queue_idx(dev, skb, dev_maps,
+							  tci);
+	}
 
-		map = rcu_dereference(dev_maps->attr_map[tci]);
-		if (map) {
-			if (map->len == 1)
-				queue_index = map->queues[0];
-			else
-				queue_index = map->queues[reciprocal_scale(skb_get_hash(skb),
-									   map->len)];
-			if (unlikely(queue_index >= dev->real_num_tx_queues))
-				queue_index = -1;
+get_cpus_map:
+	if (queue_index < 0) {
+		dev_maps = rcu_dereference(dev->xps_cpus_map);
+		if (dev_maps) {
+			unsigned int tci = skb->sender_cpu - 1;
+
+			queue_index = __get_xps_queue_idx(dev, skb, dev_maps,
+							  tci);
 		}
 	}
 	rcu_read_unlock();

[net-next PATCH v6 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Amritha Nambiar]

Extend transmit queue sysfs attribute to configure Rx queue(s) map
per Tx queue. By default no receive queues are configured for the
Tx queue.

- /sys/class/net/eth0/queues/tx-*/xps_rxqs

Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
---
 net/core/net-sysfs.c |   83 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index b39987c..f25ac5f 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1283,6 +1283,88 @@ static ssize_t xps_cpus_store(struct netdev_queue *queue,
 
 static struct netdev_queue_attribute xps_cpus_attribute __ro_after_init
 	= __ATTR_RW(xps_cpus);
+
+static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf)
+{
+	struct net_device *dev = queue->dev;
+	struct xps_dev_maps *dev_maps;
+	unsigned long *mask, index;
+	int j, len, num_tc = 1, tc = 0;
+
+	index = get_netdev_queue_index(queue);
+
+	if (dev->num_tc) {
+		num_tc = dev->num_tc;
+		tc = netdev_txq_to_tc(dev, index);
+		if (tc < 0)
+			return -EINVAL;
+	}
+	mask = kcalloc(BITS_TO_LONGS(dev->num_rx_queues), sizeof(long),
+		       GFP_KERNEL);
+	if (!mask)
+		return -ENOMEM;
+
+	rcu_read_lock();
+	dev_maps = rcu_dereference(dev->xps_rxqs_map);
+	if (!dev_maps)
+		goto out_no_maps;
+
+	for (j = -1; j = netif_attrmask_next(j, NULL, dev->num_rx_queues),
+	     j < dev->num_rx_queues;) {
+		int i, tci = j * num_tc + tc;
+		struct xps_map *map;
+
+		map = rcu_dereference(dev_maps->attr_map[tci]);
+		if (!map)
+			continue;
+
+		for (i = map->len; i--;) {
+			if (map->queues[i] == index) {
+				set_bit(j, mask);
+				break;
+			}
+		}
+	}
+out_no_maps:
+	rcu_read_unlock();
+
+	len = bitmap_print_to_pagebuf(false, buf, mask, dev->num_rx_queues);
+	kfree(mask);
+
+	return len < PAGE_SIZE ? len : -EINVAL;
+}
+
+static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf,
+			      size_t len)
+{
+	struct net_device *dev = queue->dev;
+	struct net *net = dev_net(dev);
+	unsigned long *mask, index;
+	int err;
+
+	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
+		return -EPERM;
+
+	mask = kcalloc(BITS_TO_LONGS(dev->num_rx_queues), sizeof(long),
+		       GFP_KERNEL);
+	if (!mask)
+		return -ENOMEM;
+
+	index = get_netdev_queue_index(queue);
+
+	err = bitmap_parse(buf, len, mask, dev->num_rx_queues);
+	if (err) {
+		kfree(mask);
+		return err;
+	}
+
+	err = __netif_set_xps_queue(dev, mask, index, true);
+	kfree(mask);
+	return err ? : len;
+}
+
+static struct netdev_queue_attribute xps_rxqs_attribute __ro_after_init
+	= __ATTR_RW(xps_rxqs);
 #endif /* CONFIG_XPS */
 
 static struct attribute *netdev_queue_default_attrs[] __ro_after_init = {
@@ -1290,6 +1372,7 @@ static struct attribute *netdev_queue_default_attrs[] __ro_after_init = {
 	&queue_traffic_class.attr,
 #ifdef CONFIG_XPS
 	&xps_cpus_attribute.attr,
+	&xps_rxqs_attribute.attr,
 	&queue_tx_maxrate.attr,
 #endif
 	NULL

[net-next PATCH v6 7/7] Documentation: Add explanation for XPS using Rx-queue(s) map

[Amritha Nambiar]

Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
---
 Documentation/ABI/testing/sysfs-class-net-queues |   11 ++++
 Documentation/networking/scaling.txt             |   61 ++++++++++++++++++----
 2 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-net-queues b/Documentation/ABI/testing/sysfs-class-net-queues
index 0c0df91..978b763 100644
--- a/Documentation/ABI/testing/sysfs-class-net-queues
+++ b/Documentation/ABI/testing/sysfs-class-net-queues
@@ -42,6 +42,17 @@ Description:
 		network device transmit queue. Possible vaules depend on the
 		number of available CPU(s) in the system.
 
+What:		/sys/class/<iface>/queues/tx-<queue>/xps_rxqs
+Date:		June 2018
+KernelVersion:	4.18.0
+Contact:	netdev@vger.kernel.org
+Description:
+		Mask of the receive queue(s) currently enabled to participate
+		into the Transmit Packet Steering packet processing flow for this
+		network device transmit queue. Possible values depend on the
+		number of available receive queue(s) in the network device.
+		Default is disabled.
+
 What:		/sys/class/<iface>/queues/tx-<queue>/byte_queue_limits/hold_time
 Date:		November 2011
 KernelVersion:	3.3
diff --git a/Documentation/networking/scaling.txt b/Documentation/networking/scaling.txt
index f55639d..b7056a8 100644
--- a/Documentation/networking/scaling.txt
+++ b/Documentation/networking/scaling.txt
@@ -366,8 +366,13 @@ XPS: Transmit Packet Steering
 
 Transmit Packet Steering is a mechanism for intelligently selecting
 which transmit queue to use when transmitting a packet on a multi-queue
-device. To accomplish this, a mapping from CPU to hardware queue(s) is
-recorded. The goal of this mapping is usually to assign queues
+device. This can be accomplished by recording two kinds of maps, either
+a mapping of CPU to hardware queue(s) or a mapping of receive queue(s)
+to hardware transmit queue(s).
+
+1. XPS using CPUs map
+
+The goal of this mapping is usually to assign queues
 exclusively to a subset of CPUs, where the transmit completions for
 these queues are processed on a CPU within this set. This choice
 provides two benefits. First, contention on the device queue lock is
@@ -377,15 +382,40 @@ transmit queue). Secondly, cache miss rate on transmit completion is
 reduced, in particular for data cache lines that hold the sk_buff
 structures.
 
-XPS is configured per transmit queue by setting a bitmap of CPUs that
-may use that queue to transmit. The reverse mapping, from CPUs to
-transmit queues, is computed and maintained for each network device.
-When transmitting the first packet in a flow, the function
-get_xps_queue() is called to select a queue. This function uses the ID
-of the running CPU as a key into the CPU-to-queue lookup table. If the
+2. XPS using receive queues map
+
+This mapping is used to pick transmit queue based on the receive
+queue(s) map configuration set by the administrator. A set of receive
+queues can be mapped to a set of transmit queues (many:many), although
+the common use case is a 1:1 mapping. This will enable sending packets
+on the same queue associations for transmit and receive. This is useful for
+busy polling multi-threaded workloads where there are challenges in
+associating a given CPU to a given application thread. The application
+threads are not pinned to CPUs and each thread handles packets
+received on a single queue. The receive queue number is cached in the
+socket for the connection. In this model, sending the packets on the same
+transmit queue corresponding to the associated receive queue has benefits
+in keeping the CPU overhead low. Transmit completion work is locked into
+the same queue-association that a given application is polling on. This
+avoids the overhead of triggering an interrupt on another CPU. When the
+application cleans up the packets during the busy poll, transmit completion
+may be processed along with it in the same thread context and so result in
+reduced latency.
+
+XPS is configured per transmit queue by setting a bitmap of
+CPUs/receive-queues that may use that queue to transmit. The reverse
+mapping, from CPUs to transmit queues or from receive-queues to transmit
+queues, is computed and maintained for each network device. When
+transmitting the first packet in a flow, the function get_xps_queue() is
+called to select a queue. This function uses the ID of the receive queue
+for the socket connection for a match in the receive queue-to-transmit queue
+lookup table. Alternatively, this function can also use the ID of the
+running CPU as a key into the CPU-to-queue lookup table. If the
 ID matches a single queue, that is used for transmission. If multiple
 queues match, one is selected by using the flow hash to compute an index
-into the set.
+into the set. When selecting the transmit queue based on receive queue(s)
+map, the transmit device is not validated against the receive device as it
+requires expensive lookup operation in the datapath.
 
 The queue chosen for transmitting a particular flow is saved in the
 corresponding socket structure for the flow (e.g. a TCP connection).
@@ -404,11 +434,15 @@ acknowledged.
 
 XPS is only available if the kconfig symbol CONFIG_XPS is enabled (on by
 default for SMP). The functionality remains disabled until explicitly
-configured. To enable XPS, the bitmap of CPUs that may use a transmit
-queue is configured using the sysfs file entry:
+configured. To enable XPS, the bitmap of CPUs/receive-queues that may
+use a transmit queue is configured using the sysfs file entry:
 
+For selection based on CPUs map:
 /sys/class/net/<dev>/queues/tx-<n>/xps_cpus
 
+For selection based on receive-queues map:
+/sys/class/net/<dev>/queues/tx-<n>/xps_rxqs
+
 == Suggested Configuration
 
 For a network device with a single transmission queue, XPS configuration
@@ -421,6 +455,11 @@ best CPUs to share a given queue are probably those that share the cache
 with the CPU that processes transmit completions for that queue
 (transmit interrupts).
 
+For transmit queue selection based on receive queue(s), XPS has to be
+explicitly configured mapping receive-queue(s) to transmit queue(s). If the
+user configuration for receive-queue map does not apply, then the transmit
+queue is selected based on the CPUs map.
+
 Per TX Queue rate limitation:
 =============================
 

Re: [net-next PATCH v6 0/7] Symmetric queue selection using XPS for Rx queues

[David Miller]

From: Amritha Nambiar <amritha.nambiar@intel.com>
Date: Fri, 29 Jun 2018 21:26:35 -0700

> This patch series implements support for Tx queue selection based on
> Rx queue(s) map. This is done by configuring Rx queue(s) map per Tx-queue
> using sysfs attribute. If the user configuration for Rx queues does
> not apply, then the Tx queue selection falls back to XPS using CPUs and
> finally to hashing.
 ...

Series applied, thanks.

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Andrei Vagin]

Hello Amritha,

I see a following warning on 4.18.0-rc3-next-20180703.
It looks like a problem is in this series.

[    1.084722] ============================================
[    1.084797] WARNING: possible recursive locking detected
[    1.084872] 4.18.0-rc3-next-20180703+ #1 Not tainted
[    1.084949] --------------------------------------------
[    1.085024] swapper/0/1 is trying to acquire lock:
[    1.085100] 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: static_key_slow_inc+0xe/0x20
[    1.085189] 
[    1.085189] but task is already holding lock:
[    1.085271] 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: init_vqs+0x513/0x5a0
[    1.085357] 
[    1.085357] other info that might help us debug this:
[    1.085450]  Possible unsafe locking scenario:
[    1.085450] 
[    1.085531]        CPU0
[    1.085605]        ----
[    1.085679]   lock(cpu_hotplug_lock.rw_sem);
[    1.085753]   lock(cpu_hotplug_lock.rw_sem);
[    1.085828] 
[    1.085828]  *** DEADLOCK ***
[    1.085828] 
[    1.085916]  May be due to missing lock nesting notation
[    1.085916] 
[    1.085998] 3 locks held by swapper/0/1:
[    1.086074]  #0: 00000000244bc7da (&dev->mutex){....}, at: __driver_attach+0x5a/0x110
[    1.086164]  #1: 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: init_vqs+0x513/0x5a0
[    1.086248]  #2: 000000005cd8463f (xps_map_mutex){+.+.}, at: __netif_set_xps_queue+0x8d/0xc60
[    1.086336] 
[    1.086336] stack backtrace:
[    1.086419] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.18.0-rc3-next-20180703+ #1
[    1.086504] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
[    1.086587] Call Trace:
[    1.086667]  dump_stack+0x85/0xcb
[    1.086744]  __lock_acquire+0x68a/0x1330
[    1.086821]  ? lock_acquire+0x9f/0x200
[    1.086900]  ? find_held_lock+0x2d/0x90
[    1.086976]  ? lock_acquire+0x9f/0x200
[    1.087051]  lock_acquire+0x9f/0x200
[    1.087126]  ? static_key_slow_inc+0xe/0x20
[    1.087205]  cpus_read_lock+0x3e/0x80
[    1.087280]  ? static_key_slow_inc+0xe/0x20
[    1.087355]  static_key_slow_inc+0xe/0x20
[    1.087435]  __netif_set_xps_queue+0x216/0xc60
[    1.087512]  virtnet_set_affinity+0xf0/0x130
[    1.087589]  init_vqs+0x51b/0x5a0
[    1.087665]  virtnet_probe+0x39f/0x870
[    1.087742]  virtio_dev_probe+0x170/0x220
[    1.087819]  driver_probe_device+0x30b/0x480
[    1.087897]  ? set_debug_rodata+0x11/0x11
[    1.087972]  __driver_attach+0xe0/0x110
[    1.088064]  ? driver_probe_device+0x480/0x480
[    1.088141]  bus_for_each_dev+0x79/0xc0
[    1.088221]  bus_add_driver+0x164/0x260
[    1.088302]  ? veth_init+0x11/0x11
[    1.088379]  driver_register+0x5b/0xe0
[    1.088402]  ? veth_init+0x11/0x11
[    1.088402]  virtio_net_driver_init+0x6d/0x90
[    1.088402]  do_one_initcall+0x5d/0x34c
[    1.088402]  ? set_debug_rodata+0x11/0x11
[    1.088402]  ? rcu_read_lock_sched_held+0x6b/0x80
[    1.088402]  kernel_init_freeable+0x1ea/0x27b
[    1.088402]  ? rest_init+0xd0/0xd0
[    1.088402]  kernel_init+0xa/0x110
[    1.088402]  ret_from_fork+0x3a/0x50
[    1.094190] i8042: PNP: PS/2 Controller [PNP0303:KBD,PNP0f13:MOU] at 0x60,0x64 irq 1,12


https://travis-ci.org/avagin/linux/jobs/399867744

On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:
> Extend transmit queue sysfs attribute to configure Rx queue(s) map
> per Tx queue. By default no receive queues are configured for the
> Tx queue.
> 
> - /sys/class/net/eth0/queues/tx-*/xps_rxqs
> 
> Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
> ---
>  net/core/net-sysfs.c |   83 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index b39987c..f25ac5f 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -1283,6 +1283,88 @@ static ssize_t xps_cpus_store(struct netdev_queue *queue,
>  
>  static struct netdev_queue_attribute xps_cpus_attribute __ro_after_init
>  	= __ATTR_RW(xps_cpus);
> +
> +static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf)
> +{
> +	struct net_device *dev = queue->dev;
> +	struct xps_dev_maps *dev_maps;
> +	unsigned long *mask, index;
> +	int j, len, num_tc = 1, tc = 0;
> +
> +	index = get_netdev_queue_index(queue);
> +
> +	if (dev->num_tc) {
> +		num_tc = dev->num_tc;
> +		tc = netdev_txq_to_tc(dev, index);
> +		if (tc < 0)
> +			return -EINVAL;
> +	}
> +	mask = kcalloc(BITS_TO_LONGS(dev->num_rx_queues), sizeof(long),
> +		       GFP_KERNEL);
> +	if (!mask)
> +		return -ENOMEM;
> +
> +	rcu_read_lock();
> +	dev_maps = rcu_dereference(dev->xps_rxqs_map);
> +	if (!dev_maps)
> +		goto out_no_maps;
> +
> +	for (j = -1; j = netif_attrmask_next(j, NULL, dev->num_rx_queues),
> +	     j < dev->num_rx_queues;) {
> +		int i, tci = j * num_tc + tc;
> +		struct xps_map *map;
> +
> +		map = rcu_dereference(dev_maps->attr_map[tci]);
> +		if (!map)
> +			continue;
> +
> +		for (i = map->len; i--;) {
> +			if (map->queues[i] == index) {
> +				set_bit(j, mask);
> +				break;
> +			}
> +		}
> +	}
> +out_no_maps:
> +	rcu_read_unlock();
> +
> +	len = bitmap_print_to_pagebuf(false, buf, mask, dev->num_rx_queues);
> +	kfree(mask);
> +
> +	return len < PAGE_SIZE ? len : -EINVAL;
> +}
> +
> +static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf,
> +			      size_t len)
> +{
> +	struct net_device *dev = queue->dev;
> +	struct net *net = dev_net(dev);
> +	unsigned long *mask, index;
> +	int err;
> +
> +	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	mask = kcalloc(BITS_TO_LONGS(dev->num_rx_queues), sizeof(long),
> +		       GFP_KERNEL);
> +	if (!mask)
> +		return -ENOMEM;
> +
> +	index = get_netdev_queue_index(queue);
> +
> +	err = bitmap_parse(buf, len, mask, dev->num_rx_queues);
> +	if (err) {
> +		kfree(mask);
> +		return err;
> +	}
> +
> +	err = __netif_set_xps_queue(dev, mask, index, true);
> +	kfree(mask);
> +	return err ? : len;
> +}
> +
> +static struct netdev_queue_attribute xps_rxqs_attribute __ro_after_init
> +	= __ATTR_RW(xps_rxqs);
>  #endif /* CONFIG_XPS */
>  
>  static struct attribute *netdev_queue_default_attrs[] __ro_after_init = {
> @@ -1290,6 +1372,7 @@ static struct attribute *netdev_queue_default_attrs[] __ro_after_init = {
>  	&queue_traffic_class.attr,
>  #ifdef CONFIG_XPS
>  	&xps_cpus_attribute.attr,
> +	&xps_rxqs_attribute.attr,
>  	&queue_tx_maxrate.attr,
>  #endif
>  	NULL

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Nambiar, Amritha]

On 7/4/2018 12:20 AM, Andrei Vagin wrote:
> Hello Amritha,
> 
> I see a following warning on 4.18.0-rc3-next-20180703.
> It looks like a problem is in this series.
> 
> [    1.084722] ============================================
> [    1.084797] WARNING: possible recursive locking detected
> [    1.084872] 4.18.0-rc3-next-20180703+ #1 Not tainted
> [    1.084949] --------------------------------------------
> [    1.085024] swapper/0/1 is trying to acquire lock:
> [    1.085100] 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: static_key_slow_inc+0xe/0x20
> [    1.085189] 
> [    1.085189] but task is already holding lock:
> [    1.085271] 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: init_vqs+0x513/0x5a0
> [    1.085357] 
> [    1.085357] other info that might help us debug this:
> [    1.085450]  Possible unsafe locking scenario:
> [    1.085450] 
> [    1.085531]        CPU0
> [    1.085605]        ----
> [    1.085679]   lock(cpu_hotplug_lock.rw_sem);
> [    1.085753]   lock(cpu_hotplug_lock.rw_sem);
> [    1.085828] 
> [    1.085828]  *** DEADLOCK ***
> [    1.085828] 
> [    1.085916]  May be due to missing lock nesting notation
> [    1.085916] 
> [    1.085998] 3 locks held by swapper/0/1:
> [    1.086074]  #0: 00000000244bc7da (&dev->mutex){....}, at: __driver_attach+0x5a/0x110
> [    1.086164]  #1: 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: init_vqs+0x513/0x5a0
> [    1.086248]  #2: 000000005cd8463f (xps_map_mutex){+.+.}, at: __netif_set_xps_queue+0x8d/0xc60
> [    1.086336] 
> [    1.086336] stack backtrace:
> [    1.086419] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.18.0-rc3-next-20180703+ #1
> [    1.086504] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> [    1.086587] Call Trace:
> [    1.086667]  dump_stack+0x85/0xcb
> [    1.086744]  __lock_acquire+0x68a/0x1330
> [    1.086821]  ? lock_acquire+0x9f/0x200
> [    1.086900]  ? find_held_lock+0x2d/0x90
> [    1.086976]  ? lock_acquire+0x9f/0x200
> [    1.087051]  lock_acquire+0x9f/0x200
> [    1.087126]  ? static_key_slow_inc+0xe/0x20
> [    1.087205]  cpus_read_lock+0x3e/0x80
> [    1.087280]  ? static_key_slow_inc+0xe/0x20
> [    1.087355]  static_key_slow_inc+0xe/0x20
> [    1.087435]  __netif_set_xps_queue+0x216/0xc60
> [    1.087512]  virtnet_set_affinity+0xf0/0x130
> [    1.087589]  init_vqs+0x51b/0x5a0
> [    1.087665]  virtnet_probe+0x39f/0x870
> [    1.087742]  virtio_dev_probe+0x170/0x220
> [    1.087819]  driver_probe_device+0x30b/0x480
> [    1.087897]  ? set_debug_rodata+0x11/0x11
> [    1.087972]  __driver_attach+0xe0/0x110
> [    1.088064]  ? driver_probe_device+0x480/0x480
> [    1.088141]  bus_for_each_dev+0x79/0xc0
> [    1.088221]  bus_add_driver+0x164/0x260
> [    1.088302]  ? veth_init+0x11/0x11
> [    1.088379]  driver_register+0x5b/0xe0
> [    1.088402]  ? veth_init+0x11/0x11
> [    1.088402]  virtio_net_driver_init+0x6d/0x90
> [    1.088402]  do_one_initcall+0x5d/0x34c
> [    1.088402]  ? set_debug_rodata+0x11/0x11
> [    1.088402]  ? rcu_read_lock_sched_held+0x6b/0x80
> [    1.088402]  kernel_init_freeable+0x1ea/0x27b
> [    1.088402]  ? rest_init+0xd0/0xd0
> [    1.088402]  kernel_init+0xa/0x110
> [    1.088402]  ret_from_fork+0x3a/0x50
> [    1.094190] i8042: PNP: PS/2 Controller [PNP0303:KBD,PNP0f13:MOU] at 0x60,0x64 irq 1,12
> 
> 
> https://travis-ci.org/avagin/linux/jobs/399867744
> 

With this patch series, I introduced static_key for XPS maps
(xps_needed), so static_key_slow_inc() is used to switch branches. The
definition of static_key_slow_inc() has cpus_read_lock in place. In the
virtio_net driver, XPS queues are initialized after setting the
queue:cpu affinity in virtnet_set_affinity() which is already protected
within cpus_read_lock. Hence, the warning here trying to acquire
cpus_read_lock when it is already held.

A quick fix for this would be to just extract netif_set_xps_queue() out
of the lock by simply wrapping it with another put/get_online_cpus
(unlock right before and hold lock right after). But this may not a
clean solution. It'd help if I can get suggestions on what would be a
clean option to fix this without extensively changing the code in
virtio_net. Is it mandatory to protect the affinitization with
read_lock? I don't see similar lock in other drivers while setting the
affinity. I understand this warning should go away, but isn't it safe to
have multiple readers.

> On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:
>> Extend transmit queue sysfs attribute to configure Rx queue(s) map
>> per Tx queue. By default no receive queues are configured for the
>> Tx queue.
>>
>> - /sys/class/net/eth0/queues/tx-*/xps_rxqs
>>
>> Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
>> ---
>>  net/core/net-sysfs.c |   83 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>>
>> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>> index b39987c..f25ac5f 100644
>> --- a/net/core/net-sysfs.c
>> +++ b/net/core/net-sysfs.c
>> @@ -1283,6 +1283,88 @@ static ssize_t xps_cpus_store(struct netdev_queue *queue,
>>  
>>  static struct netdev_queue_attribute xps_cpus_attribute __ro_after_init
>>  	= __ATTR_RW(xps_cpus);
>> +
>> +static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf)
>> +{
>> +	struct net_device *dev = queue->dev;
>> +	struct xps_dev_maps *dev_maps;
>> +	unsigned long *mask, index;
>> +	int j, len, num_tc = 1, tc = 0;
>> +
>> +	index = get_netdev_queue_index(queue);
>> +
>> +	if (dev->num_tc) {
>> +		num_tc = dev->num_tc;
>> +		tc = netdev_txq_to_tc(dev, index);
>> +		if (tc < 0)
>> +			return -EINVAL;
>> +	}
>> +	mask = kcalloc(BITS_TO_LONGS(dev->num_rx_queues), sizeof(long),
>> +		       GFP_KERNEL);
>> +	if (!mask)
>> +		return -ENOMEM;
>> +
>> +	rcu_read_lock();
>> +	dev_maps = rcu_dereference(dev->xps_rxqs_map);
>> +	if (!dev_maps)
>> +		goto out_no_maps;
>> +
>> +	for (j = -1; j = netif_attrmask_next(j, NULL, dev->num_rx_queues),
>> +	     j < dev->num_rx_queues;) {
>> +		int i, tci = j * num_tc + tc;
>> +		struct xps_map *map;
>> +
>> +		map = rcu_dereference(dev_maps->attr_map[tci]);
>> +		if (!map)
>> +			continue;
>> +
>> +		for (i = map->len; i--;) {
>> +			if (map->queues[i] == index) {
>> +				set_bit(j, mask);
>> +				break;
>> +			}
>> +		}
>> +	}
>> +out_no_maps:
>> +	rcu_read_unlock();
>> +
>> +	len = bitmap_print_to_pagebuf(false, buf, mask, dev->num_rx_queues);
>> +	kfree(mask);
>> +
>> +	return len < PAGE_SIZE ? len : -EINVAL;
>> +}
>> +
>> +static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf,
>> +			      size_t len)
>> +{
>> +	struct net_device *dev = queue->dev;
>> +	struct net *net = dev_net(dev);
>> +	unsigned long *mask, index;
>> +	int err;
>> +
>> +	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
>> +		return -EPERM;
>> +
>> +	mask = kcalloc(BITS_TO_LONGS(dev->num_rx_queues), sizeof(long),
>> +		       GFP_KERNEL);
>> +	if (!mask)
>> +		return -ENOMEM;
>> +
>> +	index = get_netdev_queue_index(queue);
>> +
>> +	err = bitmap_parse(buf, len, mask, dev->num_rx_queues);
>> +	if (err) {
>> +		kfree(mask);
>> +		return err;
>> +	}
>> +
>> +	err = __netif_set_xps_queue(dev, mask, index, true);
>> +	kfree(mask);
>> +	return err ? : len;
>> +}
>> +
>> +static struct netdev_queue_attribute xps_rxqs_attribute __ro_after_init
>> +	= __ATTR_RW(xps_rxqs);
>>  #endif /* CONFIG_XPS */
>>  
>>  static struct attribute *netdev_queue_default_attrs[] __ro_after_init = {
>> @@ -1290,6 +1372,7 @@ static struct attribute *netdev_queue_default_attrs[] __ro_after_init = {
>>  	&queue_traffic_class.attr,
>>  #ifdef CONFIG_XPS
>>  	&xps_cpus_attribute.attr,
>> +	&xps_rxqs_attribute.attr,
>>  	&queue_tx_maxrate.attr,
>>  #endif
>>  	NULL

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Andrei Vagin]

On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
> On 7/4/2018 12:20 AM, Andrei Vagin wrote:
> > Hello Amritha,
> > 
> > I see a following warning on 4.18.0-rc3-next-20180703.
> > It looks like a problem is in this series.
> > 
> > [    1.084722] ============================================
> > [    1.084797] WARNING: possible recursive locking detected
> > [    1.084872] 4.18.0-rc3-next-20180703+ #1 Not tainted
> > [    1.084949] --------------------------------------------
> > [    1.085024] swapper/0/1 is trying to acquire lock:
> > [    1.085100] 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: static_key_slow_inc+0xe/0x20
> > [    1.085189] 
> > [    1.085189] but task is already holding lock:
> > [    1.085271] 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: init_vqs+0x513/0x5a0
> > [    1.085357] 
> > [    1.085357] other info that might help us debug this:
> > [    1.085450]  Possible unsafe locking scenario:
> > [    1.085450] 
> > [    1.085531]        CPU0
> > [    1.085605]        ----
> > [    1.085679]   lock(cpu_hotplug_lock.rw_sem);
> > [    1.085753]   lock(cpu_hotplug_lock.rw_sem);
> > [    1.085828] 
> > [    1.085828]  *** DEADLOCK ***
> > [    1.085828] 
> > [    1.085916]  May be due to missing lock nesting notation
> > [    1.085916] 
> > [    1.085998] 3 locks held by swapper/0/1:
> > [    1.086074]  #0: 00000000244bc7da (&dev->mutex){....}, at: __driver_attach+0x5a/0x110
> > [    1.086164]  #1: 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: init_vqs+0x513/0x5a0
> > [    1.086248]  #2: 000000005cd8463f (xps_map_mutex){+.+.}, at: __netif_set_xps_queue+0x8d/0xc60
> > [    1.086336] 
> > [    1.086336] stack backtrace:
> > [    1.086419] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.18.0-rc3-next-20180703+ #1
> > [    1.086504] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > [    1.086587] Call Trace:
> > [    1.086667]  dump_stack+0x85/0xcb
> > [    1.086744]  __lock_acquire+0x68a/0x1330
> > [    1.086821]  ? lock_acquire+0x9f/0x200
> > [    1.086900]  ? find_held_lock+0x2d/0x90
> > [    1.086976]  ? lock_acquire+0x9f/0x200
> > [    1.087051]  lock_acquire+0x9f/0x200
> > [    1.087126]  ? static_key_slow_inc+0xe/0x20
> > [    1.087205]  cpus_read_lock+0x3e/0x80
> > [    1.087280]  ? static_key_slow_inc+0xe/0x20
> > [    1.087355]  static_key_slow_inc+0xe/0x20
> > [    1.087435]  __netif_set_xps_queue+0x216/0xc60
> > [    1.087512]  virtnet_set_affinity+0xf0/0x130
> > [    1.087589]  init_vqs+0x51b/0x5a0
> > [    1.087665]  virtnet_probe+0x39f/0x870
> > [    1.087742]  virtio_dev_probe+0x170/0x220
> > [    1.087819]  driver_probe_device+0x30b/0x480
> > [    1.087897]  ? set_debug_rodata+0x11/0x11
> > [    1.087972]  __driver_attach+0xe0/0x110
> > [    1.088064]  ? driver_probe_device+0x480/0x480
> > [    1.088141]  bus_for_each_dev+0x79/0xc0
> > [    1.088221]  bus_add_driver+0x164/0x260
> > [    1.088302]  ? veth_init+0x11/0x11
> > [    1.088379]  driver_register+0x5b/0xe0
> > [    1.088402]  ? veth_init+0x11/0x11
> > [    1.088402]  virtio_net_driver_init+0x6d/0x90
> > [    1.088402]  do_one_initcall+0x5d/0x34c
> > [    1.088402]  ? set_debug_rodata+0x11/0x11
> > [    1.088402]  ? rcu_read_lock_sched_held+0x6b/0x80
> > [    1.088402]  kernel_init_freeable+0x1ea/0x27b
> > [    1.088402]  ? rest_init+0xd0/0xd0
> > [    1.088402]  kernel_init+0xa/0x110
> > [    1.088402]  ret_from_fork+0x3a/0x50
> > [    1.094190] i8042: PNP: PS/2 Controller [PNP0303:KBD,PNP0f13:MOU] at 0x60,0x64 irq 1,12
> > 
> > 
> > https://travis-ci.org/avagin/linux/jobs/399867744
> > 
> 
> With this patch series, I introduced static_key for XPS maps
> (xps_needed), so static_key_slow_inc() is used to switch branches. The
> definition of static_key_slow_inc() has cpus_read_lock in place. In the
> virtio_net driver, XPS queues are initialized after setting the
> queue:cpu affinity in virtnet_set_affinity() which is already protected
> within cpus_read_lock. Hence, the warning here trying to acquire
> cpus_read_lock when it is already held.
> 
> A quick fix for this would be to just extract netif_set_xps_queue() out
> of the lock by simply wrapping it with another put/get_online_cpus
> (unlock right before and hold lock right after). But this may not a
> clean solution. It'd help if I can get suggestions on what would be a
> clean option to fix this without extensively changing the code in
> virtio_net. Is it mandatory to protect the affinitization with
> read_lock? I don't see similar lock in other drivers while setting the
> affinity.

> I understand this warning should go away, but isn't it safe to
> have multiple readers.

Peter and Ingo, maybe you could explain why it isn't safe to take one
reader lock twice?

Thanks,
Andrei

> 
> > On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:
> >> Extend transmit queue sysfs attribute to configure Rx queue(s) map
> >> per Tx queue. By default no receive queues are configured for the
> >> Tx queue.
> >>
> >> - /sys/class/net/eth0/queues/tx-*/xps_rxqs
> >>
> >> Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
> >> ---
> >>  net/core/net-sysfs.c |   83 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 83 insertions(+)
> >>
> >> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> >> index b39987c..f25ac5f 100644
> >> --- a/net/core/net-sysfs.c
> >> +++ b/net/core/net-sysfs.c
> >> @@ -1283,6 +1283,88 @@ static ssize_t xps_cpus_store(struct netdev_queue *queue,
> >>  
> >>  static struct netdev_queue_attribute xps_cpus_attribute __ro_after_init
> >>  	= __ATTR_RW(xps_cpus);
> >> +
> >> +static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf)
> >> +{
> >> +	struct net_device *dev = queue->dev;
> >> +	struct xps_dev_maps *dev_maps;
> >> +	unsigned long *mask, index;
> >> +	int j, len, num_tc = 1, tc = 0;
> >> +
> >> +	index = get_netdev_queue_index(queue);
> >> +
> >> +	if (dev->num_tc) {
> >> +		num_tc = dev->num_tc;
> >> +		tc = netdev_txq_to_tc(dev, index);
> >> +		if (tc < 0)
> >> +			return -EINVAL;
> >> +	}
> >> +	mask = kcalloc(BITS_TO_LONGS(dev->num_rx_queues), sizeof(long),
> >> +		       GFP_KERNEL);
> >> +	if (!mask)
> >> +		return -ENOMEM;
> >> +
> >> +	rcu_read_lock();
> >> +	dev_maps = rcu_dereference(dev->xps_rxqs_map);
> >> +	if (!dev_maps)
> >> +		goto out_no_maps;
> >> +
> >> +	for (j = -1; j = netif_attrmask_next(j, NULL, dev->num_rx_queues),
> >> +	     j < dev->num_rx_queues;) {
> >> +		int i, tci = j * num_tc + tc;
> >> +		struct xps_map *map;
> >> +
> >> +		map = rcu_dereference(dev_maps->attr_map[tci]);
> >> +		if (!map)
> >> +			continue;
> >> +
> >> +		for (i = map->len; i--;) {
> >> +			if (map->queues[i] == index) {
> >> +				set_bit(j, mask);
> >> +				break;
> >> +			}
> >> +		}
> >> +	}
> >> +out_no_maps:
> >> +	rcu_read_unlock();
> >> +
> >> +	len = bitmap_print_to_pagebuf(false, buf, mask, dev->num_rx_queues);
> >> +	kfree(mask);
> >> +
> >> +	return len < PAGE_SIZE ? len : -EINVAL;
> >> +}
> >> +
> >> +static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf,
> >> +			      size_t len)
> >> +{
> >> +	struct net_device *dev = queue->dev;
> >> +	struct net *net = dev_net(dev);
> >> +	unsigned long *mask, index;
> >> +	int err;
> >> +
> >> +	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> >> +		return -EPERM;
> >> +
> >> +	mask = kcalloc(BITS_TO_LONGS(dev->num_rx_queues), sizeof(long),
> >> +		       GFP_KERNEL);
> >> +	if (!mask)
> >> +		return -ENOMEM;
> >> +
> >> +	index = get_netdev_queue_index(queue);
> >> +
> >> +	err = bitmap_parse(buf, len, mask, dev->num_rx_queues);
> >> +	if (err) {
> >> +		kfree(mask);
> >> +		return err;
> >> +	}
> >> +
> >> +	err = __netif_set_xps_queue(dev, mask, index, true);
> >> +	kfree(mask);
> >> +	return err ? : len;
> >> +}
> >> +
> >> +static struct netdev_queue_attribute xps_rxqs_attribute __ro_after_init
> >> +	= __ATTR_RW(xps_rxqs);
> >>  #endif /* CONFIG_XPS */
> >>  
> >>  static struct attribute *netdev_queue_default_attrs[] __ro_after_init = {
> >> @@ -1290,6 +1372,7 @@ static struct attribute *netdev_queue_default_attrs[] __ro_after_init = {
> >>  	&queue_traffic_class.attr,
> >>  #ifdef CONFIG_XPS
> >>  	&xps_cpus_attribute.attr,
> >> +	&xps_rxqs_attribute.attr,
> >>  	&queue_tx_maxrate.attr,
> >>  #endif
> >>  	NULL

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Stephen Hemminger]

On Wed, 18 Jul 2018 11:22:36 -0700
Andrei Vagin <avagin@virtuozzo.com> wrote:

> On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
> > On 7/4/2018 12:20 AM, Andrei Vagin wrote:  
> > > Hello Amritha,
> > > 
> > > I see a following warning on 4.18.0-rc3-next-20180703.
> > > It looks like a problem is in this series.
> > > 
> > > [    1.084722] ============================================
> > > [    1.084797] WARNING: possible recursive locking detected
> > > [    1.084872] 4.18.0-rc3-next-20180703+ #1 Not tainted
> > > [    1.084949] --------------------------------------------
> > > [    1.085024] swapper/0/1 is trying to acquire lock:
> > > [    1.085100] 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: static_key_slow_inc+0xe/0x20
> > > [    1.085189] 
> > > [    1.085189] but task is already holding lock:
> > > [    1.085271] 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: init_vqs+0x513/0x5a0
> > > [    1.085357] 
> > > [    1.085357] other info that might help us debug this:
> > > [    1.085450]  Possible unsafe locking scenario:
> > > [    1.085450] 
> > > [    1.085531]        CPU0
> > > [    1.085605]        ----
> > > [    1.085679]   lock(cpu_hotplug_lock.rw_sem);
> > > [    1.085753]   lock(cpu_hotplug_lock.rw_sem);
> > > [    1.085828] 
> > > [    1.085828]  *** DEADLOCK ***
> > > [    1.085828] 
> > > [    1.085916]  May be due to missing lock nesting notation
> > > [    1.085916] 
> > > [    1.085998] 3 locks held by swapper/0/1:
> > > [    1.086074]  #0: 00000000244bc7da (&dev->mutex){....}, at: __driver_attach+0x5a/0x110
> > > [    1.086164]  #1: 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: init_vqs+0x513/0x5a0
> > > [    1.086248]  #2: 000000005cd8463f (xps_map_mutex){+.+.}, at: __netif_set_xps_queue+0x8d/0xc60
> > > [    1.086336] 
> > > [    1.086336] stack backtrace:
> > > [    1.086419] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.18.0-rc3-next-20180703+ #1
> > > [    1.086504] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > [    1.086587] Call Trace:
> > > [    1.086667]  dump_stack+0x85/0xcb
> > > [    1.086744]  __lock_acquire+0x68a/0x1330
> > > [    1.086821]  ? lock_acquire+0x9f/0x200
> > > [    1.086900]  ? find_held_lock+0x2d/0x90
> > > [    1.086976]  ? lock_acquire+0x9f/0x200
> > > [    1.087051]  lock_acquire+0x9f/0x200
> > > [    1.087126]  ? static_key_slow_inc+0xe/0x20
> > > [    1.087205]  cpus_read_lock+0x3e/0x80
> > > [    1.087280]  ? static_key_slow_inc+0xe/0x20
> > > [    1.087355]  static_key_slow_inc+0xe/0x20
> > > [    1.087435]  __netif_set_xps_queue+0x216/0xc60
> > > [    1.087512]  virtnet_set_affinity+0xf0/0x130
> > > [    1.087589]  init_vqs+0x51b/0x5a0
> > > [    1.087665]  virtnet_probe+0x39f/0x870
> > > [    1.087742]  virtio_dev_probe+0x170/0x220
> > > [    1.087819]  driver_probe_device+0x30b/0x480
> > > [    1.087897]  ? set_debug_rodata+0x11/0x11
> > > [    1.087972]  __driver_attach+0xe0/0x110
> > > [    1.088064]  ? driver_probe_device+0x480/0x480
> > > [    1.088141]  bus_for_each_dev+0x79/0xc0
> > > [    1.088221]  bus_add_driver+0x164/0x260
> > > [    1.088302]  ? veth_init+0x11/0x11
> > > [    1.088379]  driver_register+0x5b/0xe0
> > > [    1.088402]  ? veth_init+0x11/0x11
> > > [    1.088402]  virtio_net_driver_init+0x6d/0x90
> > > [    1.088402]  do_one_initcall+0x5d/0x34c
> > > [    1.088402]  ? set_debug_rodata+0x11/0x11
> > > [    1.088402]  ? rcu_read_lock_sched_held+0x6b/0x80
> > > [    1.088402]  kernel_init_freeable+0x1ea/0x27b
> > > [    1.088402]  ? rest_init+0xd0/0xd0
> > > [    1.088402]  kernel_init+0xa/0x110
> > > [    1.088402]  ret_from_fork+0x3a/0x50
> > > [    1.094190] i8042: PNP: PS/2 Controller [PNP0303:KBD,PNP0f13:MOU] at 0x60,0x64 irq 1,12
> > > 
> > > 
> > > https://travis-ci.org/avagin/linux/jobs/399867744
> > >   
> > 
> > With this patch series, I introduced static_key for XPS maps
> > (xps_needed), so static_key_slow_inc() is used to switch branches. The
> > definition of static_key_slow_inc() has cpus_read_lock in place. In the
> > virtio_net driver, XPS queues are initialized after setting the
> > queue:cpu affinity in virtnet_set_affinity() which is already protected
> > within cpus_read_lock. Hence, the warning here trying to acquire
> > cpus_read_lock when it is already held.
> > 
> > A quick fix for this would be to just extract netif_set_xps_queue() out
> > of the lock by simply wrapping it with another put/get_online_cpus
> > (unlock right before and hold lock right after). But this may not a
> > clean solution. It'd help if I can get suggestions on what would be a
> > clean option to fix this without extensively changing the code in
> > virtio_net. Is it mandatory to protect the affinitization with
> > read_lock? I don't see similar lock in other drivers while setting the
> > affinity.  
> 
> > I understand this warning should go away, but isn't it safe to
> > have multiple readers.  
> 
> Peter and Ingo, maybe you could explain why it isn't safe to take one
> reader lock twice?
> 
> Thanks,
> Andrei

I think the issue was that some architectures, I think read lock is equivalent to a spin lock.
But maybe that is no longer true, or know one remembers.

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Peter Zijlstra]

On Wed, Jul 18, 2018 at 11:22:36AM -0700, Andrei Vagin wrote:
> > > [    1.085679]   lock(cpu_hotplug_lock.rw_sem);
> > > [    1.085753]   lock(cpu_hotplug_lock.rw_sem);
> > > [    1.085828] 
> > > [    1.085828]  *** DEADLOCK ***

> Peter and Ingo, maybe you could explain why it isn't safe to take one
> reader lock twice?

Very simple, because rwsems are fair and !recursive.

So if another CPU were to issue a write-lock in between these, then the
second would block because there is a writer pending. But because we
then already have a reader we're deadlocked.

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Andrei Vagin]

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

On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
> With this patch series, I introduced static_key for XPS maps
> (xps_needed), so static_key_slow_inc() is used to switch branches. The
> definition of static_key_slow_inc() has cpus_read_lock in place. In the
> virtio_net driver, XPS queues are initialized after setting the
> queue:cpu affinity in virtnet_set_affinity() which is already protected
> within cpus_read_lock. Hence, the warning here trying to acquire
> cpus_read_lock when it is already held.
> 
> A quick fix for this would be to just extract netif_set_xps_queue() out
> of the lock by simply wrapping it with another put/get_online_cpus
> (unlock right before and hold lock right after).

virtnet_set_affinity() is called from virtnet_cpu_online(), which is
called under cpus_read_lock too.

It looks like now we can't call netif_set_xps_queue() from cpu hotplug
callbacks.

I can suggest a very straightforward fix for this problem. The patch is
attached.

> But this may not a
> clean solution. It'd help if I can get suggestions on what would be a
> clean option to fix this without extensively changing the code in
> virtio_net. Is it mandatory to protect the affinitization with
> read_lock? I don't see similar lock in other drivers while setting the
> affinity. I understand this warning should go away, but isn't it safe to
> have multiple readers.
> 
> > On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4258 bytes --]

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1880c86e84b4..ebb5470090e5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1941,7 +1941,7 @@ static void virtnet_set_affinity(struct virtnet_info *vi)
 	for_each_online_cpu(cpu) {
 		virtqueue_set_affinity(vi->rq[i].vq, cpu);
 		virtqueue_set_affinity(vi->sq[i].vq, cpu);
-		netif_set_xps_queue(vi->dev, cpumask_of(cpu), i);
+		__netif_set_xps_queue(vi->dev, cpumask_bits(cpumask_of(cpu)), i, false, true);
 		i++;
 	}
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9c917467a2c7..37c88e566806 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3320,7 +3320,7 @@ static inline void netif_wake_subqueue(struct net_device *dev, u16 queue_index)
 int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
 			u16 index);
 int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
-			  u16 index, bool is_rxqs_map);
+			  u16 index, bool is_rxqs_map, bool cpuslocked);
 
 /**
  *	netif_attr_test_mask - Test a CPU or Rx queue set in a mask
diff --git a/net/core/dev.c b/net/core/dev.c
index 38b0c414d780..96ade614a23e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2176,6 +2176,7 @@ static void netif_reset_xps_queues(struct net_device *dev, u16 offset,
 	if (!static_key_false(&xps_needed))
 		return;
 
+	cpus_read_lock();
 	mutex_lock(&xps_map_mutex);
 
 	if (static_key_false(&xps_rxqs_needed)) {
@@ -2199,10 +2200,11 @@ static void netif_reset_xps_queues(struct net_device *dev, u16 offset,
 
 out_no_maps:
 	if (static_key_enabled(&xps_rxqs_needed))
-		static_key_slow_dec(&xps_rxqs_needed);
+		static_key_slow_dec_cpuslocked(&xps_rxqs_needed);
 
-	static_key_slow_dec(&xps_needed);
+	static_key_slow_dec_cpuslocked(&xps_needed);
 	mutex_unlock(&xps_map_mutex);
+	cpus_read_unlock();
 }
 
 static void netif_reset_xps_queues_gt(struct net_device *dev, u16 index)
@@ -2251,7 +2253,7 @@ static struct xps_map *expand_xps_map(struct xps_map *map, int attr_index,
 }
 
 int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
-			  u16 index, bool is_rxqs_map)
+			  u16 index, bool is_rxqs_map, bool cpuslocked)
 {
 	const unsigned long *online_mask = NULL, *possible_mask = NULL;
 	struct xps_dev_maps *dev_maps, *new_dev_maps = NULL;
@@ -2275,6 +2277,9 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
 			return -EINVAL;
 	}
 
+	if (!cpuslocked)
+		cpus_read_lock();
+
 	mutex_lock(&xps_map_mutex);
 	if (is_rxqs_map) {
 		maps_sz = XPS_RXQ_DEV_MAPS_SIZE(num_tc, dev->num_rx_queues);
@@ -2317,9 +2322,9 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
 	if (!new_dev_maps)
 		goto out_no_new_maps;
 
-	static_key_slow_inc(&xps_needed);
+	static_key_slow_inc_cpuslocked(&xps_needed);
 	if (is_rxqs_map)
-		static_key_slow_inc(&xps_rxqs_needed);
+		static_key_slow_inc_cpuslocked(&xps_rxqs_needed);
 
 	for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids),
 	     j < nr_ids;) {
@@ -2427,6 +2432,8 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
 
 out_no_maps:
 	mutex_unlock(&xps_map_mutex);
+	if (!cpuslocked)
+		cpus_read_unlock();
 
 	return 0;
 error:
@@ -2444,15 +2451,18 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
 	}
 
 	mutex_unlock(&xps_map_mutex);
+	if (!cpuslocked)
+		cpus_read_unlock();
 
 	kfree(new_dev_maps);
 	return -ENOMEM;
 }
+EXPORT_SYMBOL_GPL(__netif_set_xps_queue);
 
 int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
 			u16 index)
 {
-	return __netif_set_xps_queue(dev, cpumask_bits(mask), index, false);
+	return __netif_set_xps_queue(dev, cpumask_bits(mask), index, false, false);
 }
 EXPORT_SYMBOL(netif_set_xps_queue);
 
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 0a95bcf64cdc..06a141445d80 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1400,7 +1400,7 @@ static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf,
 		return err;
 	}
 
-	err = __netif_set_xps_queue(dev, mask, index, true);
+	err = __netif_set_xps_queue(dev, mask, index, true, false);
 	kfree(mask);
 	return err ? : len;
 }

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Nambiar, Amritha]

On 8/1/2018 5:11 PM, Andrei Vagin wrote:
> On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
>> With this patch series, I introduced static_key for XPS maps
>> (xps_needed), so static_key_slow_inc() is used to switch branches. The
>> definition of static_key_slow_inc() has cpus_read_lock in place. In the
>> virtio_net driver, XPS queues are initialized after setting the
>> queue:cpu affinity in virtnet_set_affinity() which is already protected
>> within cpus_read_lock. Hence, the warning here trying to acquire
>> cpus_read_lock when it is already held.
>>
>> A quick fix for this would be to just extract netif_set_xps_queue() out
>> of the lock by simply wrapping it with another put/get_online_cpus
>> (unlock right before and hold lock right after).
> 
> virtnet_set_affinity() is called from virtnet_cpu_online(), which is
> called under cpus_read_lock too.
> 
> It looks like now we can't call netif_set_xps_queue() from cpu hotplug
> callbacks.
> 
> I can suggest a very straightforward fix for this problem. The patch is
> attached.
> 

Thanks for looking into this. I was thinking of fixing this in the
virtio_net driver by moving the XPS initialization (and have a new
get_affinity utility) in the ndo_open (so it is together with other tx
preparation) instead of probe. Your patch solves this in general for
setting up cpu hotplug callbacks which is under cpus_read_lock.

>> But this may not a
>> clean solution. It'd help if I can get suggestions on what would be a
>> clean option to fix this without extensively changing the code in
>> virtio_net. Is it mandatory to protect the affinitization with
>> read_lock? I don't see similar lock in other drivers while setting the
>> affinity. I understand this warning should go away, but isn't it safe to
>> have multiple readers.
>>
>>> On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Michael S. Tsirkin]

On Thu, Aug 02, 2018 at 02:04:12PM -0700, Nambiar, Amritha wrote:
> On 8/1/2018 5:11 PM, Andrei Vagin wrote:
> > On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
> >> With this patch series, I introduced static_key for XPS maps
> >> (xps_needed), so static_key_slow_inc() is used to switch branches. The
> >> definition of static_key_slow_inc() has cpus_read_lock in place. In the
> >> virtio_net driver, XPS queues are initialized after setting the
> >> queue:cpu affinity in virtnet_set_affinity() which is already protected
> >> within cpus_read_lock. Hence, the warning here trying to acquire
> >> cpus_read_lock when it is already held.
> >>
> >> A quick fix for this would be to just extract netif_set_xps_queue() out
> >> of the lock by simply wrapping it with another put/get_online_cpus
> >> (unlock right before and hold lock right after).
> > 
> > virtnet_set_affinity() is called from virtnet_cpu_online(), which is
> > called under cpus_read_lock too.
> > 
> > It looks like now we can't call netif_set_xps_queue() from cpu hotplug
> > callbacks.
> > 
> > I can suggest a very straightforward fix for this problem. The patch is
> > attached.
> > 
> 
> Thanks for looking into this. I was thinking of fixing this in the
> virtio_net driver by moving the XPS initialization (and have a new
> get_affinity utility) in the ndo_open (so it is together with other tx
> preparation) instead of probe. Your patch solves this in general for
> setting up cpu hotplug callbacks which is under cpus_read_lock.


I like this too. Could you repost in a standard way
(inline, with your signoff etc) so we can ack this for
net-next?

> >> But this may not a
> >> clean solution. It'd help if I can get suggestions on what would be a
> >> clean option to fix this without extensively changing the code in
> >> virtio_net. Is it mandatory to protect the affinitization with
> >> read_lock? I don't see similar lock in other drivers while setting the
> >> affinity. I understand this warning should go away, but isn't it safe to
> >> have multiple readers.
> >>
> >>> On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Michael S. Tsirkin]

On Thu, Aug 02, 2018 at 02:04:12PM -0700, Nambiar, Amritha wrote:
> On 8/1/2018 5:11 PM, Andrei Vagin wrote:
> > On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
> >> With this patch series, I introduced static_key for XPS maps
> >> (xps_needed), so static_key_slow_inc() is used to switch branches. The
> >> definition of static_key_slow_inc() has cpus_read_lock in place. In the
> >> virtio_net driver, XPS queues are initialized after setting the
> >> queue:cpu affinity in virtnet_set_affinity() which is already protected
> >> within cpus_read_lock. Hence, the warning here trying to acquire
> >> cpus_read_lock when it is already held.
> >>
> >> A quick fix for this would be to just extract netif_set_xps_queue() out
> >> of the lock by simply wrapping it with another put/get_online_cpus
> >> (unlock right before and hold lock right after).
> > 
> > virtnet_set_affinity() is called from virtnet_cpu_online(), which is
> > called under cpus_read_lock too.
> > 
> > It looks like now we can't call netif_set_xps_queue() from cpu hotplug
> > callbacks.
> > 
> > I can suggest a very straightforward fix for this problem. The patch is
> > attached.
> > 
> 
> Thanks for looking into this. I was thinking of fixing this in the
> virtio_net driver by moving the XPS initialization (and have a new
> get_affinity utility) in the ndo_open (so it is together with other tx
> preparation) instead of probe. Your patch solves this in general for
> setting up cpu hotplug callbacks which is under cpus_read_lock.


I like this too. Could you repost in a standard way
(inline, with your signoff etc) so we can ack this for
net-next?

> >> But this may not a
> >> clean solution. It'd help if I can get suggestions on what would be a
> >> clean option to fix this without extensively changing the code in
> >> virtio_net. Is it mandatory to protect the affinitization with
> >> read_lock? I don't see similar lock in other drivers while setting the
> >> affinity. I understand this warning should go away, but isn't it safe to
> >> have multiple readers.
> >>
> >>> On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Andrei Vagin]

On Fri, Aug 03, 2018 at 12:08:05AM +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 02, 2018 at 02:04:12PM -0700, Nambiar, Amritha wrote:
> > On 8/1/2018 5:11 PM, Andrei Vagin wrote:
> > > On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
> > >> With this patch series, I introduced static_key for XPS maps
> > >> (xps_needed), so static_key_slow_inc() is used to switch branches. The
> > >> definition of static_key_slow_inc() has cpus_read_lock in place. In the
> > >> virtio_net driver, XPS queues are initialized after setting the
> > >> queue:cpu affinity in virtnet_set_affinity() which is already protected
> > >> within cpus_read_lock. Hence, the warning here trying to acquire
> > >> cpus_read_lock when it is already held.
> > >>
> > >> A quick fix for this would be to just extract netif_set_xps_queue() out
> > >> of the lock by simply wrapping it with another put/get_online_cpus
> > >> (unlock right before and hold lock right after).
> > > 
> > > virtnet_set_affinity() is called from virtnet_cpu_online(), which is
> > > called under cpus_read_lock too.
> > > 
> > > It looks like now we can't call netif_set_xps_queue() from cpu hotplug
> > > callbacks.
> > > 
> > > I can suggest a very straightforward fix for this problem. The patch is
> > > attached.
> > > 
> > 
> > Thanks for looking into this. I was thinking of fixing this in the
> > virtio_net driver by moving the XPS initialization (and have a new
> > get_affinity utility) in the ndo_open (so it is together with other tx
> > preparation) instead of probe. Your patch solves this in general for
> > setting up cpu hotplug callbacks which is under cpus_read_lock.
> 
> 
> I like this too. Could you repost in a standard way
> (inline, with your signoff etc) so we can ack this for
> net-next?

When I was testing this patch, I got the following kasan warning. Michael,
could you take a look at it. Maybe you will understand what was going wrong there.

https://api.travis-ci.org/v3/job/410701353/log.txt

[    7.275033] ==================================================================
[    7.275226] BUG: KASAN: slab-out-of-bounds in virtnet_poll+0xaa1/0xd00
[    7.275359] Read of size 8 at addr ffff8801d444a000 by task ip/370
[    7.275488] 
[    7.275610] CPU: 1 PID: 370 Comm: ip Not tainted 4.18.0-rc6+ #1
[    7.275613] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
[    7.275616] Call Trace:
[    7.275621]  <IRQ>
[    7.275630]  dump_stack+0x71/0xab
[    7.275640]  print_address_description+0x6a/0x270
[    7.275648]  kasan_report+0x258/0x380
[    7.275653]  ? virtnet_poll+0xaa1/0xd00
[    7.275661]  virtnet_poll+0xaa1/0xd00
[    7.275680]  ? receive_buf+0x5920/0x5920
[    7.275689]  ? do_raw_spin_unlock+0x54/0x220
[    7.275699]  ? find_held_lock+0x32/0x1c0
[    7.275710]  ? rcu_process_callbacks+0xa60/0xd20
[    7.275736]  net_rx_action+0x2ee/0xad0
[    7.275748]  ? rcu_note_context_switch+0x320/0x320
[    7.275754]  ? napi_complete_done+0x300/0x300
[    7.275763]  ? native_apic_msr_write+0x27/0x30
[    7.275768]  ? lapic_next_event+0x5b/0x90
[    7.275775]  ? clockevents_program_event+0x21d/0x2f0
[    7.275791]  __do_softirq+0x19a/0x623
[    7.275807]  do_softirq_own_stack+0x2a/0x40
[    7.275811]  </IRQ>
[    7.275818]  do_softirq.part.18+0x6a/0x80
[    7.275825]  __local_bh_enable_ip+0x49/0x50
[    7.275829]  virtnet_open+0x129/0x440
[    7.275841]  __dev_open+0x189/0x2c0
[    7.275848]  ? dev_set_rx_mode+0x30/0x30
[    7.275857]  ? do_raw_spin_unlock+0x54/0x220
[    7.275866]  __dev_change_flags+0x3a9/0x4f0
[    7.275873]  ? dev_set_allmulti+0x10/0x10
[    7.275889]  dev_change_flags+0x7a/0x150
[    7.275900]  do_setlink+0x9fe/0x2e40
[    7.275910]  ? deref_stack_reg+0xad/0xe0
[    7.275917]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
[    7.275922]  ? find_held_lock+0x32/0x1c0
[    7.275929]  ? rtnetlink_put_metrics+0x460/0x460
[    7.275935]  ? virtqueue_add_sgs+0x9e2/0xde0
[    7.275953]  ? virtscsi_add_cmd+0x454/0x780
[    7.275964]  ? find_held_lock+0x32/0x1c0
[    7.275973]  ? deref_stack_reg+0xad/0xe0
[    7.275979]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
[    7.275985]  ? lock_downgrade+0x5e0/0x5e0
[    7.275993]  ? memset+0x1f/0x40
[    7.276008]  ? nla_parse+0x33/0x290
[    7.276016]  rtnl_newlink+0x954/0x1120
[    7.276030]  ? rtnl_link_unregister+0x250/0x250
[    7.276044]  ? is_bpf_text_address+0x5/0x60
[    7.276054]  ? lock_downgrade+0x5e0/0x5e0
[    7.276057]  ? lock_acquire+0x10b/0x2a0
[    7.276072]  ? deref_stack_reg+0xad/0xe0
[    7.276078]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
[    7.276085]  ? __kernel_text_address+0xe/0x30
[    7.276090]  ? unwind_get_return_address+0x5f/0xa0
[    7.276103]  ? find_held_lock+0x32/0x1c0
[    7.276110]  ? is_bpf_text_address+0x5/0x60
[    7.276124]  ? deref_stack_reg+0xad/0xe0
[    7.276131]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
[    7.276136]  ? depot_save_stack+0x2d9/0x460
[    7.276142]  ? deref_stack_reg+0xad/0xe0
[    7.276156]  ? find_held_lock+0x32/0x1c0
[    7.276164]  ? is_bpf_text_address+0x5/0x60
[    7.276170]  ? __lock_acquire.isra.29+0xe8/0x1bb0
[    7.276212]  ? rtnetlink_rcv_msg+0x5d6/0x930
[    7.276222]  ? lock_downgrade+0x5e0/0x5e0
[    7.276226]  ? lock_acquire+0x10b/0x2a0
[    7.276240]  rtnetlink_rcv_msg+0x69e/0x930
[    7.276249]  ? rtnl_calcit.isra.31+0x2f0/0x2f0
[    7.276255]  ? __lock_acquire.isra.29+0xe8/0x1bb0
[    7.276265]  ? netlink_deliver_tap+0x8d/0x8e0
[    7.276276]  netlink_rcv_skb+0x127/0x350
[    7.276281]  ? rtnl_calcit.isra.31+0x2f0/0x2f0
[    7.276289]  ? netlink_ack+0x970/0x970
[    7.276299]  ? __alloc_skb+0xc2/0x520
[    7.276311]  netlink_unicast+0x40f/0x5d0
[    7.276320]  ? netlink_attachskb+0x580/0x580
[    7.276325]  ? _copy_from_iter_full+0x157/0x6f0
[    7.276331]  ? import_iovec+0x90/0x390
[    7.276338]  ? get_page_from_freelist+0x1e89/0x3e30
[    7.276347]  ? apparmor_socket_sock_rcv_skb+0x10/0x10
[    7.276357]  netlink_sendmsg+0x65e/0xb00
[    7.276367]  ? netlink_unicast+0x5d0/0x5d0
[    7.276373]  ? copy_msghdr_from_user+0x206/0x340
[    7.276388]  ? netlink_unicast+0x5d0/0x5d0
[    7.276394]  sock_sendmsg+0xb3/0xf0
[    7.276401]  ___sys_sendmsg+0x604/0x8b0
[    7.276410]  ? copy_msghdr_from_user+0x340/0x340
[    7.276416]  ? find_held_lock+0x32/0x1c0
[    7.276424]  ? __handle_mm_fault+0xc85/0x3140
[    7.276433]  ? lock_downgrade+0x5e0/0x5e0
[    7.276439]  ? mem_cgroup_commit_charge+0xb4/0xf80
[    7.276453]  ? _raw_spin_unlock+0x24/0x30
[    7.276458]  ? __handle_mm_fault+0xc85/0x3140
[    7.276467]  ? __pmd_alloc+0x430/0x430
[    7.276473]  ? find_held_lock+0x32/0x1c0
[    7.276485]  ? __fget_light+0x55/0x1f0
[    7.276497]  ? __sys_sendmsg+0xd2/0x170
[    7.276502]  __sys_sendmsg+0xd2/0x170
[    7.276508]  ? __ia32_sys_shutdown+0x70/0x70
[    7.276516]  ? handle_mm_fault+0x1f9/0x6a0
[    7.276528]  ? up_read+0x1c/0x110
[    7.276534]  ? __do_page_fault+0x4a6/0xa80
[    7.276554]  do_syscall_64+0xa0/0x280
[    7.276558]  ? prepare_exit_to_usermode+0x88/0x130
[    7.276566]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[    7.276572] RIP: 0033:0x7ffbe9a2f160
[    7.276574] Code: c3 48 8b 05 2a 2d 2c 00 f7 db 64 89 18 48 83 cb ff eb dd 0f 1f 80 00 00 00 00 83 3d 1d 8f 2c 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 ee cb 00 00 48 89 04 24 
[    7.276728] RSP: 002b:00007ffc5a2d6108 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[    7.276735] RAX: ffffffffffffffda RBX: 00007ffc5a2da220 RCX: 00007ffbe9a2f160
[    7.276738] RDX: 0000000000000000 RSI: 00007ffc5a2d6150 RDI: 0000000000000003
[    7.276741] RBP: 00007ffc5a2d6150 R08: 0000000000000000 R09: 0000000000000003
[    7.276744] R10: 00007ffc5a2d5ed0 R11: 0000000000000246 R12: 000000005b6177de
[    7.276748] R13: 0000000000000000 R14: 00000000006473a0 R15: 00007ffc5a2da918
[    7.276763] 
[    7.276895] Allocated by task 1:
[    7.277026]  kasan_kmalloc+0xa0/0xd0
[    7.277030]  __kmalloc+0x13a/0x250
[    7.277034]  init_vqs+0xd0/0x11c0
[    7.277038]  virtnet_probe+0xb99/0x1ad0
[    7.277045]  virtio_dev_probe+0x3fc/0x890
[    7.277052]  driver_probe_device+0x6c4/0xcc0
[    7.277056]  __driver_attach+0x232/0x2c0
[    7.277060]  bus_for_each_dev+0x118/0x1a0
[    7.277064]  bus_add_driver+0x390/0x6e0
[    7.277068]  driver_register+0x18e/0x400
[    7.277076]  virtio_net_driver_init+0x6d/0x90
[    7.277080]  do_one_initcall+0xa8/0x348
[    7.277085]  kernel_init_freeable+0x42d/0x4c8
[    7.277090]  kernel_init+0xf/0x130
[    7.277095]  ret_from_fork+0x35/0x40
[    7.277097] 
[    7.277223] Freed by task 0:
[    7.277347] (stack is not available)
[    7.277473] 
[    7.277596] The buggy address belongs to the object at ffff8801d4449100
[    7.277596]  which belongs to the cache kmalloc-4096 of size 4096
[    7.277769] The buggy address is located 3840 bytes inside of
[    7.277769]  4096-byte region [ffff8801d4449100, ffff8801d444a100)
[    7.277932] The buggy address belongs to the page:
[    7.278066] page:ffffea0007511200 count:1 mapcount:0 mapping:ffff8801db002600 index:0x0 compound_mapcount: 0
[    7.278230] flags: 0x17fff8000008100(slab|head)
[    7.278363] raw: 017fff8000008100 dead000000000100 dead000000000200 ffff8801db002600
[    7.278516] raw: 0000000000000000 0000000000070007 00000001ffffffff 0000000000000000
[    7.278664] page dumped because: kasan: bad access detected
[    7.278790] 
[    7.278904] Memory state around the buggy address:
[    7.279031]  ffff8801d4449f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[    7.279175]  ffff8801d4449f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[    7.279323] >ffff8801d444a000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[    7.279468]                    ^
[    7.279584]  ffff8801d444a080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[    7.279729]  ffff8801d444a100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[    7.279870] ==================================================================
[    7.280011] Disabling lock debugging due to kernel taint
[    7.632219] random: mktemp: uninitialized urandom read (6 bytes read)
[    8.052850] random: mktemp: uninitialized urandom read (6 bytes read)
[    8.335209] random: cloud-init: uninitialized urandom read (32 bytes read)
[    8.796331] random: cloud-init: uninitialized urandom read (32 bytes read)
[    9.162551] random: mktemp: uninitialized urandom read (12 bytes read)
[    9.384504] random: ssh-keygen: uninitialized urandom read (32 bytes read)
[    9.839586] init: failsafe main process (724) killed by TERM signal
[   14.865443] postgres (1245): /proc/1245/oom_adj is deprecated, please use /proc/1245/oom_score_adj instead.
[   17.213418] random: crng init done
[   17.580892] init: plymouth-upstart-bridge main process ended, respawning

> 
> > >> But this may not a
> > >> clean solution. It'd help if I can get suggestions on what would be a
> > >> clean option to fix this without extensively changing the code in
> > >> virtio_net. Is it mandatory to protect the affinitization with
> > >> read_lock? I don't see similar lock in other drivers while setting the
> > >> affinity. I understand this warning should go away, but isn't it safe to
> > >> have multiple readers.
> > >>
> > >>> On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Michael S. Tsirkin]

On Fri, Aug 03, 2018 at 12:06:51PM -0700, Andrei Vagin wrote:
> On Fri, Aug 03, 2018 at 12:08:05AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Aug 02, 2018 at 02:04:12PM -0700, Nambiar, Amritha wrote:
> > > On 8/1/2018 5:11 PM, Andrei Vagin wrote:
> > > > On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
> > > >> With this patch series, I introduced static_key for XPS maps
> > > >> (xps_needed), so static_key_slow_inc() is used to switch branches. The
> > > >> definition of static_key_slow_inc() has cpus_read_lock in place. In the
> > > >> virtio_net driver, XPS queues are initialized after setting the
> > > >> queue:cpu affinity in virtnet_set_affinity() which is already protected
> > > >> within cpus_read_lock. Hence, the warning here trying to acquire
> > > >> cpus_read_lock when it is already held.
> > > >>
> > > >> A quick fix for this would be to just extract netif_set_xps_queue() out
> > > >> of the lock by simply wrapping it with another put/get_online_cpus
> > > >> (unlock right before and hold lock right after).
> > > > 
> > > > virtnet_set_affinity() is called from virtnet_cpu_online(), which is
> > > > called under cpus_read_lock too.
> > > > 
> > > > It looks like now we can't call netif_set_xps_queue() from cpu hotplug
> > > > callbacks.
> > > > 
> > > > I can suggest a very straightforward fix for this problem. The patch is
> > > > attached.
> > > > 
> > > 
> > > Thanks for looking into this. I was thinking of fixing this in the
> > > virtio_net driver by moving the XPS initialization (and have a new
> > > get_affinity utility) in the ndo_open (so it is together with other tx
> > > preparation) instead of probe. Your patch solves this in general for
> > > setting up cpu hotplug callbacks which is under cpus_read_lock.
> > 
> > 
> > I like this too. Could you repost in a standard way
> > (inline, with your signoff etc) so we can ack this for
> > net-next?
> 
> When I was testing this patch, I got the following kasan warning. Michael,
> could you take a look at it. Maybe you will understand what was going wrong there.
> 
> https://api.travis-ci.org/v3/job/410701353/log.txt
> 
> [    7.275033] ==================================================================
> [    7.275226] BUG: KASAN: slab-out-of-bounds in virtnet_poll+0xaa1/0xd00
> [    7.275359] Read of size 8 at addr ffff8801d444a000 by task ip/370
> [    7.275488] 
> [    7.275610] CPU: 1 PID: 370 Comm: ip Not tainted 4.18.0-rc6+ #1
> [    7.275613] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> [    7.275616] Call Trace:
> [    7.275621]  <IRQ>
> [    7.275630]  dump_stack+0x71/0xab
> [    7.275640]  print_address_description+0x6a/0x270
> [    7.275648]  kasan_report+0x258/0x380
> [    7.275653]  ? virtnet_poll+0xaa1/0xd00
> [    7.275661]  virtnet_poll+0xaa1/0xd00
> [    7.275680]  ? receive_buf+0x5920/0x5920
> [    7.275689]  ? do_raw_spin_unlock+0x54/0x220
> [    7.275699]  ? find_held_lock+0x32/0x1c0
> [    7.275710]  ? rcu_process_callbacks+0xa60/0xd20
> [    7.275736]  net_rx_action+0x2ee/0xad0
> [    7.275748]  ? rcu_note_context_switch+0x320/0x320
> [    7.275754]  ? napi_complete_done+0x300/0x300
> [    7.275763]  ? native_apic_msr_write+0x27/0x30
> [    7.275768]  ? lapic_next_event+0x5b/0x90
> [    7.275775]  ? clockevents_program_event+0x21d/0x2f0
> [    7.275791]  __do_softirq+0x19a/0x623
> [    7.275807]  do_softirq_own_stack+0x2a/0x40
> [    7.275811]  </IRQ>
> [    7.275818]  do_softirq.part.18+0x6a/0x80
> [    7.275825]  __local_bh_enable_ip+0x49/0x50
> [    7.275829]  virtnet_open+0x129/0x440
> [    7.275841]  __dev_open+0x189/0x2c0
> [    7.275848]  ? dev_set_rx_mode+0x30/0x30
> [    7.275857]  ? do_raw_spin_unlock+0x54/0x220
> [    7.275866]  __dev_change_flags+0x3a9/0x4f0
> [    7.275873]  ? dev_set_allmulti+0x10/0x10
> [    7.275889]  dev_change_flags+0x7a/0x150
> [    7.275900]  do_setlink+0x9fe/0x2e40
> [    7.275910]  ? deref_stack_reg+0xad/0xe0
> [    7.275917]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> [    7.275922]  ? find_held_lock+0x32/0x1c0
> [    7.275929]  ? rtnetlink_put_metrics+0x460/0x460
> [    7.275935]  ? virtqueue_add_sgs+0x9e2/0xde0
> [    7.275953]  ? virtscsi_add_cmd+0x454/0x780
> [    7.275964]  ? find_held_lock+0x32/0x1c0
> [    7.275973]  ? deref_stack_reg+0xad/0xe0
> [    7.275979]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> [    7.275985]  ? lock_downgrade+0x5e0/0x5e0
> [    7.275993]  ? memset+0x1f/0x40
> [    7.276008]  ? nla_parse+0x33/0x290
> [    7.276016]  rtnl_newlink+0x954/0x1120
> [    7.276030]  ? rtnl_link_unregister+0x250/0x250
> [    7.276044]  ? is_bpf_text_address+0x5/0x60
> [    7.276054]  ? lock_downgrade+0x5e0/0x5e0
> [    7.276057]  ? lock_acquire+0x10b/0x2a0
> [    7.276072]  ? deref_stack_reg+0xad/0xe0
> [    7.276078]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> [    7.276085]  ? __kernel_text_address+0xe/0x30
> [    7.276090]  ? unwind_get_return_address+0x5f/0xa0
> [    7.276103]  ? find_held_lock+0x32/0x1c0
> [    7.276110]  ? is_bpf_text_address+0x5/0x60
> [    7.276124]  ? deref_stack_reg+0xad/0xe0
> [    7.276131]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> [    7.276136]  ? depot_save_stack+0x2d9/0x460
> [    7.276142]  ? deref_stack_reg+0xad/0xe0
> [    7.276156]  ? find_held_lock+0x32/0x1c0
> [    7.276164]  ? is_bpf_text_address+0x5/0x60
> [    7.276170]  ? __lock_acquire.isra.29+0xe8/0x1bb0
> [    7.276212]  ? rtnetlink_rcv_msg+0x5d6/0x930
> [    7.276222]  ? lock_downgrade+0x5e0/0x5e0
> [    7.276226]  ? lock_acquire+0x10b/0x2a0
> [    7.276240]  rtnetlink_rcv_msg+0x69e/0x930
> [    7.276249]  ? rtnl_calcit.isra.31+0x2f0/0x2f0
> [    7.276255]  ? __lock_acquire.isra.29+0xe8/0x1bb0
> [    7.276265]  ? netlink_deliver_tap+0x8d/0x8e0
> [    7.276276]  netlink_rcv_skb+0x127/0x350
> [    7.276281]  ? rtnl_calcit.isra.31+0x2f0/0x2f0
> [    7.276289]  ? netlink_ack+0x970/0x970
> [    7.276299]  ? __alloc_skb+0xc2/0x520
> [    7.276311]  netlink_unicast+0x40f/0x5d0
> [    7.276320]  ? netlink_attachskb+0x580/0x580
> [    7.276325]  ? _copy_from_iter_full+0x157/0x6f0
> [    7.276331]  ? import_iovec+0x90/0x390
> [    7.276338]  ? get_page_from_freelist+0x1e89/0x3e30
> [    7.276347]  ? apparmor_socket_sock_rcv_skb+0x10/0x10
> [    7.276357]  netlink_sendmsg+0x65e/0xb00
> [    7.276367]  ? netlink_unicast+0x5d0/0x5d0
> [    7.276373]  ? copy_msghdr_from_user+0x206/0x340
> [    7.276388]  ? netlink_unicast+0x5d0/0x5d0
> [    7.276394]  sock_sendmsg+0xb3/0xf0
> [    7.276401]  ___sys_sendmsg+0x604/0x8b0
> [    7.276410]  ? copy_msghdr_from_user+0x340/0x340
> [    7.276416]  ? find_held_lock+0x32/0x1c0
> [    7.276424]  ? __handle_mm_fault+0xc85/0x3140
> [    7.276433]  ? lock_downgrade+0x5e0/0x5e0
> [    7.276439]  ? mem_cgroup_commit_charge+0xb4/0xf80
> [    7.276453]  ? _raw_spin_unlock+0x24/0x30
> [    7.276458]  ? __handle_mm_fault+0xc85/0x3140
> [    7.276467]  ? __pmd_alloc+0x430/0x430
> [    7.276473]  ? find_held_lock+0x32/0x1c0
> [    7.276485]  ? __fget_light+0x55/0x1f0
> [    7.276497]  ? __sys_sendmsg+0xd2/0x170
> [    7.276502]  __sys_sendmsg+0xd2/0x170
> [    7.276508]  ? __ia32_sys_shutdown+0x70/0x70
> [    7.276516]  ? handle_mm_fault+0x1f9/0x6a0
> [    7.276528]  ? up_read+0x1c/0x110
> [    7.276534]  ? __do_page_fault+0x4a6/0xa80
> [    7.276554]  do_syscall_64+0xa0/0x280
> [    7.276558]  ? prepare_exit_to_usermode+0x88/0x130
> [    7.276566]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [    7.276572] RIP: 0033:0x7ffbe9a2f160
> [    7.276574] Code: c3 48 8b 05 2a 2d 2c 00 f7 db 64 89 18 48 83 cb ff eb dd 0f 1f 80 00 00 00 00 83 3d 1d 8f 2c 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 ee cb 00 00 48 89 04 24 
> [    7.276728] RSP: 002b:00007ffc5a2d6108 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> [    7.276735] RAX: ffffffffffffffda RBX: 00007ffc5a2da220 RCX: 00007ffbe9a2f160
> [    7.276738] RDX: 0000000000000000 RSI: 00007ffc5a2d6150 RDI: 0000000000000003
> [    7.276741] RBP: 00007ffc5a2d6150 R08: 0000000000000000 R09: 0000000000000003
> [    7.276744] R10: 00007ffc5a2d5ed0 R11: 0000000000000246 R12: 000000005b6177de
> [    7.276748] R13: 0000000000000000 R14: 00000000006473a0 R15: 00007ffc5a2da918
> [    7.276763] 
> [    7.276895] Allocated by task 1:
> [    7.277026]  kasan_kmalloc+0xa0/0xd0
> [    7.277030]  __kmalloc+0x13a/0x250
> [    7.277034]  init_vqs+0xd0/0x11c0
> [    7.277038]  virtnet_probe+0xb99/0x1ad0
> [    7.277045]  virtio_dev_probe+0x3fc/0x890
> [    7.277052]  driver_probe_device+0x6c4/0xcc0
> [    7.277056]  __driver_attach+0x232/0x2c0
> [    7.277060]  bus_for_each_dev+0x118/0x1a0
> [    7.277064]  bus_add_driver+0x390/0x6e0
> [    7.277068]  driver_register+0x18e/0x400
> [    7.277076]  virtio_net_driver_init+0x6d/0x90
> [    7.277080]  do_one_initcall+0xa8/0x348
> [    7.277085]  kernel_init_freeable+0x42d/0x4c8
> [    7.277090]  kernel_init+0xf/0x130
> [    7.277095]  ret_from_fork+0x35/0x40
> [    7.277097] 
> [    7.277223] Freed by task 0:
> [    7.277347] (stack is not available)
> [    7.277473] 
> [    7.277596] The buggy address belongs to the object at ffff8801d4449100
> [    7.277596]  which belongs to the cache kmalloc-4096 of size 4096
> [    7.277769] The buggy address is located 3840 bytes inside of
> [    7.277769]  4096-byte region [ffff8801d4449100, ffff8801d444a100)
> [    7.277932] The buggy address belongs to the page:
> [    7.278066] page:ffffea0007511200 count:1 mapcount:0 mapping:ffff8801db002600 index:0x0 compound_mapcount: 0
> [    7.278230] flags: 0x17fff8000008100(slab|head)
> [    7.278363] raw: 017fff8000008100 dead000000000100 dead000000000200 ffff8801db002600
> [    7.278516] raw: 0000000000000000 0000000000070007 00000001ffffffff 0000000000000000
> [    7.278664] page dumped because: kasan: bad access detected
> [    7.278790] 
> [    7.278904] Memory state around the buggy address:
> [    7.279031]  ffff8801d4449f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [    7.279175]  ffff8801d4449f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [    7.279323] >ffff8801d444a000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [    7.279468]                    ^
> [    7.279584]  ffff8801d444a080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [    7.279729]  ffff8801d444a100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [    7.279870] ==================================================================
> [    7.280011] Disabling lock debugging due to kernel taint
> [    7.632219] random: mktemp: uninitialized urandom read (6 bytes read)
> [    8.052850] random: mktemp: uninitialized urandom read (6 bytes read)
> [    8.335209] random: cloud-init: uninitialized urandom read (32 bytes read)
> [    8.796331] random: cloud-init: uninitialized urandom read (32 bytes read)
> [    9.162551] random: mktemp: uninitialized urandom read (12 bytes read)
> [    9.384504] random: ssh-keygen: uninitialized urandom read (32 bytes read)
> [    9.839586] init: failsafe main process (724) killed by TERM signal
> [   14.865443] postgres (1245): /proc/1245/oom_adj is deprecated, please use /proc/1245/oom_score_adj instead.
> [   17.213418] random: crng init done
> [   17.580892] init: plymouth-upstart-bridge main process ended, respawning

I suspect an off by one somewhere. I'm looking at the patch
and I don't see it but these things are hard to spot sometimes ...

> > 
> > > >> But this may not a
> > > >> clean solution. It'd help if I can get suggestions on what would be a
> > > >> clean option to fix this without extensively changing the code in
> > > >> virtio_net. Is it mandatory to protect the affinitization with
> > > >> read_lock? I don't see similar lock in other drivers while setting the
> > > >> affinity. I understand this warning should go away, but isn't it safe to
> > > >> have multiple readers.
> > > >>
> > > >>> On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Michael S. Tsirkin]

On Fri, Aug 03, 2018 at 12:06:51PM -0700, Andrei Vagin wrote:
> On Fri, Aug 03, 2018 at 12:08:05AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Aug 02, 2018 at 02:04:12PM -0700, Nambiar, Amritha wrote:
> > > On 8/1/2018 5:11 PM, Andrei Vagin wrote:
> > > > On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
> > > >> With this patch series, I introduced static_key for XPS maps
> > > >> (xps_needed), so static_key_slow_inc() is used to switch branches. The
> > > >> definition of static_key_slow_inc() has cpus_read_lock in place. In the
> > > >> virtio_net driver, XPS queues are initialized after setting the
> > > >> queue:cpu affinity in virtnet_set_affinity() which is already protected
> > > >> within cpus_read_lock. Hence, the warning here trying to acquire
> > > >> cpus_read_lock when it is already held.
> > > >>
> > > >> A quick fix for this would be to just extract netif_set_xps_queue() out
> > > >> of the lock by simply wrapping it with another put/get_online_cpus
> > > >> (unlock right before and hold lock right after).
> > > > 
> > > > virtnet_set_affinity() is called from virtnet_cpu_online(), which is
> > > > called under cpus_read_lock too.
> > > > 
> > > > It looks like now we can't call netif_set_xps_queue() from cpu hotplug
> > > > callbacks.
> > > > 
> > > > I can suggest a very straightforward fix for this problem. The patch is
> > > > attached.
> > > > 
> > > 
> > > Thanks for looking into this. I was thinking of fixing this in the
> > > virtio_net driver by moving the XPS initialization (and have a new
> > > get_affinity utility) in the ndo_open (so it is together with other tx
> > > preparation) instead of probe. Your patch solves this in general for
> > > setting up cpu hotplug callbacks which is under cpus_read_lock.
> > 
> > 
> > I like this too. Could you repost in a standard way
> > (inline, with your signoff etc) so we can ack this for
> > net-next?
> 
> When I was testing this patch, I got the following kasan warning. Michael,
> could you take a look at it. Maybe you will understand what was going wrong there.
> 
> https://api.travis-ci.org/v3/job/410701353/log.txt
> 
> [    7.275033] ==================================================================
> [    7.275226] BUG: KASAN: slab-out-of-bounds in virtnet_poll+0xaa1/0xd00
> [    7.275359] Read of size 8 at addr ffff8801d444a000 by task ip/370
> [    7.275488] 
> [    7.275610] CPU: 1 PID: 370 Comm: ip Not tainted 4.18.0-rc6+ #1
> [    7.275613] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> [    7.275616] Call Trace:
> [    7.275621]  <IRQ>
> [    7.275630]  dump_stack+0x71/0xab
> [    7.275640]  print_address_description+0x6a/0x270
> [    7.275648]  kasan_report+0x258/0x380
> [    7.275653]  ? virtnet_poll+0xaa1/0xd00
> [    7.275661]  virtnet_poll+0xaa1/0xd00
> [    7.275680]  ? receive_buf+0x5920/0x5920
> [    7.275689]  ? do_raw_spin_unlock+0x54/0x220
> [    7.275699]  ? find_held_lock+0x32/0x1c0
> [    7.275710]  ? rcu_process_callbacks+0xa60/0xd20
> [    7.275736]  net_rx_action+0x2ee/0xad0
> [    7.275748]  ? rcu_note_context_switch+0x320/0x320
> [    7.275754]  ? napi_complete_done+0x300/0x300
> [    7.275763]  ? native_apic_msr_write+0x27/0x30
> [    7.275768]  ? lapic_next_event+0x5b/0x90
> [    7.275775]  ? clockevents_program_event+0x21d/0x2f0
> [    7.275791]  __do_softirq+0x19a/0x623
> [    7.275807]  do_softirq_own_stack+0x2a/0x40
> [    7.275811]  </IRQ>
> [    7.275818]  do_softirq.part.18+0x6a/0x80
> [    7.275825]  __local_bh_enable_ip+0x49/0x50
> [    7.275829]  virtnet_open+0x129/0x440
> [    7.275841]  __dev_open+0x189/0x2c0
> [    7.275848]  ? dev_set_rx_mode+0x30/0x30
> [    7.275857]  ? do_raw_spin_unlock+0x54/0x220
> [    7.275866]  __dev_change_flags+0x3a9/0x4f0
> [    7.275873]  ? dev_set_allmulti+0x10/0x10
> [    7.275889]  dev_change_flags+0x7a/0x150
> [    7.275900]  do_setlink+0x9fe/0x2e40
> [    7.275910]  ? deref_stack_reg+0xad/0xe0
> [    7.275917]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> [    7.275922]  ? find_held_lock+0x32/0x1c0
> [    7.275929]  ? rtnetlink_put_metrics+0x460/0x460
> [    7.275935]  ? virtqueue_add_sgs+0x9e2/0xde0
> [    7.275953]  ? virtscsi_add_cmd+0x454/0x780
> [    7.275964]  ? find_held_lock+0x32/0x1c0
> [    7.275973]  ? deref_stack_reg+0xad/0xe0
> [    7.275979]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> [    7.275985]  ? lock_downgrade+0x5e0/0x5e0
> [    7.275993]  ? memset+0x1f/0x40
> [    7.276008]  ? nla_parse+0x33/0x290
> [    7.276016]  rtnl_newlink+0x954/0x1120
> [    7.276030]  ? rtnl_link_unregister+0x250/0x250
> [    7.276044]  ? is_bpf_text_address+0x5/0x60
> [    7.276054]  ? lock_downgrade+0x5e0/0x5e0
> [    7.276057]  ? lock_acquire+0x10b/0x2a0
> [    7.276072]  ? deref_stack_reg+0xad/0xe0
> [    7.276078]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> [    7.276085]  ? __kernel_text_address+0xe/0x30
> [    7.276090]  ? unwind_get_return_address+0x5f/0xa0
> [    7.276103]  ? find_held_lock+0x32/0x1c0
> [    7.276110]  ? is_bpf_text_address+0x5/0x60
> [    7.276124]  ? deref_stack_reg+0xad/0xe0
> [    7.276131]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> [    7.276136]  ? depot_save_stack+0x2d9/0x460
> [    7.276142]  ? deref_stack_reg+0xad/0xe0
> [    7.276156]  ? find_held_lock+0x32/0x1c0
> [    7.276164]  ? is_bpf_text_address+0x5/0x60
> [    7.276170]  ? __lock_acquire.isra.29+0xe8/0x1bb0
> [    7.276212]  ? rtnetlink_rcv_msg+0x5d6/0x930
> [    7.276222]  ? lock_downgrade+0x5e0/0x5e0
> [    7.276226]  ? lock_acquire+0x10b/0x2a0
> [    7.276240]  rtnetlink_rcv_msg+0x69e/0x930
> [    7.276249]  ? rtnl_calcit.isra.31+0x2f0/0x2f0
> [    7.276255]  ? __lock_acquire.isra.29+0xe8/0x1bb0
> [    7.276265]  ? netlink_deliver_tap+0x8d/0x8e0
> [    7.276276]  netlink_rcv_skb+0x127/0x350
> [    7.276281]  ? rtnl_calcit.isra.31+0x2f0/0x2f0
> [    7.276289]  ? netlink_ack+0x970/0x970
> [    7.276299]  ? __alloc_skb+0xc2/0x520
> [    7.276311]  netlink_unicast+0x40f/0x5d0
> [    7.276320]  ? netlink_attachskb+0x580/0x580
> [    7.276325]  ? _copy_from_iter_full+0x157/0x6f0
> [    7.276331]  ? import_iovec+0x90/0x390
> [    7.276338]  ? get_page_from_freelist+0x1e89/0x3e30
> [    7.276347]  ? apparmor_socket_sock_rcv_skb+0x10/0x10
> [    7.276357]  netlink_sendmsg+0x65e/0xb00
> [    7.276367]  ? netlink_unicast+0x5d0/0x5d0
> [    7.276373]  ? copy_msghdr_from_user+0x206/0x340
> [    7.276388]  ? netlink_unicast+0x5d0/0x5d0
> [    7.276394]  sock_sendmsg+0xb3/0xf0
> [    7.276401]  ___sys_sendmsg+0x604/0x8b0
> [    7.276410]  ? copy_msghdr_from_user+0x340/0x340
> [    7.276416]  ? find_held_lock+0x32/0x1c0
> [    7.276424]  ? __handle_mm_fault+0xc85/0x3140
> [    7.276433]  ? lock_downgrade+0x5e0/0x5e0
> [    7.276439]  ? mem_cgroup_commit_charge+0xb4/0xf80
> [    7.276453]  ? _raw_spin_unlock+0x24/0x30
> [    7.276458]  ? __handle_mm_fault+0xc85/0x3140
> [    7.276467]  ? __pmd_alloc+0x430/0x430
> [    7.276473]  ? find_held_lock+0x32/0x1c0
> [    7.276485]  ? __fget_light+0x55/0x1f0
> [    7.276497]  ? __sys_sendmsg+0xd2/0x170
> [    7.276502]  __sys_sendmsg+0xd2/0x170
> [    7.276508]  ? __ia32_sys_shutdown+0x70/0x70
> [    7.276516]  ? handle_mm_fault+0x1f9/0x6a0
> [    7.276528]  ? up_read+0x1c/0x110
> [    7.276534]  ? __do_page_fault+0x4a6/0xa80
> [    7.276554]  do_syscall_64+0xa0/0x280
> [    7.276558]  ? prepare_exit_to_usermode+0x88/0x130
> [    7.276566]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [    7.276572] RIP: 0033:0x7ffbe9a2f160
> [    7.276574] Code: c3 48 8b 05 2a 2d 2c 00 f7 db 64 89 18 48 83 cb ff eb dd 0f 1f 80 00 00 00 00 83 3d 1d 8f 2c 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 ee cb 00 00 48 89 04 24 
> [    7.276728] RSP: 002b:00007ffc5a2d6108 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> [    7.276735] RAX: ffffffffffffffda RBX: 00007ffc5a2da220 RCX: 00007ffbe9a2f160
> [    7.276738] RDX: 0000000000000000 RSI: 00007ffc5a2d6150 RDI: 0000000000000003
> [    7.276741] RBP: 00007ffc5a2d6150 R08: 0000000000000000 R09: 0000000000000003
> [    7.276744] R10: 00007ffc5a2d5ed0 R11: 0000000000000246 R12: 000000005b6177de
> [    7.276748] R13: 0000000000000000 R14: 00000000006473a0 R15: 00007ffc5a2da918
> [    7.276763] 
> [    7.276895] Allocated by task 1:
> [    7.277026]  kasan_kmalloc+0xa0/0xd0
> [    7.277030]  __kmalloc+0x13a/0x250
> [    7.277034]  init_vqs+0xd0/0x11c0
> [    7.277038]  virtnet_probe+0xb99/0x1ad0
> [    7.277045]  virtio_dev_probe+0x3fc/0x890
> [    7.277052]  driver_probe_device+0x6c4/0xcc0
> [    7.277056]  __driver_attach+0x232/0x2c0
> [    7.277060]  bus_for_each_dev+0x118/0x1a0
> [    7.277064]  bus_add_driver+0x390/0x6e0
> [    7.277068]  driver_register+0x18e/0x400
> [    7.277076]  virtio_net_driver_init+0x6d/0x90
> [    7.277080]  do_one_initcall+0xa8/0x348
> [    7.277085]  kernel_init_freeable+0x42d/0x4c8
> [    7.277090]  kernel_init+0xf/0x130
> [    7.277095]  ret_from_fork+0x35/0x40
> [    7.277097] 
> [    7.277223] Freed by task 0:
> [    7.277347] (stack is not available)
> [    7.277473] 
> [    7.277596] The buggy address belongs to the object at ffff8801d4449100
> [    7.277596]  which belongs to the cache kmalloc-4096 of size 4096
> [    7.277769] The buggy address is located 3840 bytes inside of
> [    7.277769]  4096-byte region [ffff8801d4449100, ffff8801d444a100)
> [    7.277932] The buggy address belongs to the page:
> [    7.278066] page:ffffea0007511200 count:1 mapcount:0 mapping:ffff8801db002600 index:0x0 compound_mapcount: 0
> [    7.278230] flags: 0x17fff8000008100(slab|head)
> [    7.278363] raw: 017fff8000008100 dead000000000100 dead000000000200 ffff8801db002600
> [    7.278516] raw: 0000000000000000 0000000000070007 00000001ffffffff 0000000000000000
> [    7.278664] page dumped because: kasan: bad access detected
> [    7.278790] 
> [    7.278904] Memory state around the buggy address:
> [    7.279031]  ffff8801d4449f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [    7.279175]  ffff8801d4449f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [    7.279323] >ffff8801d444a000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [    7.279468]                    ^
> [    7.279584]  ffff8801d444a080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [    7.279729]  ffff8801d444a100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [    7.279870] ==================================================================
> [    7.280011] Disabling lock debugging due to kernel taint
> [    7.632219] random: mktemp: uninitialized urandom read (6 bytes read)
> [    8.052850] random: mktemp: uninitialized urandom read (6 bytes read)
> [    8.335209] random: cloud-init: uninitialized urandom read (32 bytes read)
> [    8.796331] random: cloud-init: uninitialized urandom read (32 bytes read)
> [    9.162551] random: mktemp: uninitialized urandom read (12 bytes read)
> [    9.384504] random: ssh-keygen: uninitialized urandom read (32 bytes read)
> [    9.839586] init: failsafe main process (724) killed by TERM signal
> [   14.865443] postgres (1245): /proc/1245/oom_adj is deprecated, please use /proc/1245/oom_score_adj instead.
> [   17.213418] random: crng init done
> [   17.580892] init: plymouth-upstart-bridge main process ended, respawning

I suspect an off by one somewhere. I'm looking at the patch
and I don't see it but these things are hard to spot sometimes ...

> > 
> > > >> But this may not a
> > > >> clean solution. It'd help if I can get suggestions on what would be a
> > > >> clean option to fix this without extensively changing the code in
> > > >> virtio_net. Is it mandatory to protect the affinitization with
> > > >> read_lock? I don't see similar lock in other drivers while setting the
> > > >> affinity. I understand this warning should go away, but isn't it safe to
> > > >> have multiple readers.
> > > >>
> > > >>> On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Andrei Vagin]

On Fri, Aug 03, 2018 at 10:12:53PM +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 03, 2018 at 12:06:51PM -0700, Andrei Vagin wrote:
> > On Fri, Aug 03, 2018 at 12:08:05AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Aug 02, 2018 at 02:04:12PM -0700, Nambiar, Amritha wrote:
> > > > On 8/1/2018 5:11 PM, Andrei Vagin wrote:
> > > > > On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
> > > > >> With this patch series, I introduced static_key for XPS maps
> > > > >> (xps_needed), so static_key_slow_inc() is used to switch branches. The
> > > > >> definition of static_key_slow_inc() has cpus_read_lock in place. In the
> > > > >> virtio_net driver, XPS queues are initialized after setting the
> > > > >> queue:cpu affinity in virtnet_set_affinity() which is already protected
> > > > >> within cpus_read_lock. Hence, the warning here trying to acquire
> > > > >> cpus_read_lock when it is already held.
> > > > >>
> > > > >> A quick fix for this would be to just extract netif_set_xps_queue() out
> > > > >> of the lock by simply wrapping it with another put/get_online_cpus
> > > > >> (unlock right before and hold lock right after).
> > > > > 
> > > > > virtnet_set_affinity() is called from virtnet_cpu_online(), which is
> > > > > called under cpus_read_lock too.
> > > > > 
> > > > > It looks like now we can't call netif_set_xps_queue() from cpu hotplug
> > > > > callbacks.
> > > > > 
> > > > > I can suggest a very straightforward fix for this problem. The patch is
> > > > > attached.
> > > > > 
> > > > 
> > > > Thanks for looking into this. I was thinking of fixing this in the
> > > > virtio_net driver by moving the XPS initialization (and have a new
> > > > get_affinity utility) in the ndo_open (so it is together with other tx
> > > > preparation) instead of probe. Your patch solves this in general for
> > > > setting up cpu hotplug callbacks which is under cpus_read_lock.
> > > 
> > > 
> > > I like this too. Could you repost in a standard way
> > > (inline, with your signoff etc) so we can ack this for
> > > net-next?
> > 
> > When I was testing this patch, I got the following kasan warning. Michael,
> > could you take a look at it. Maybe you will understand what was going wrong there.
> > 
> > https://api.travis-ci.org/v3/job/410701353/log.txt
> > 
> > [    7.275033] ==================================================================
> > [    7.275226] BUG: KASAN: slab-out-of-bounds in virtnet_poll+0xaa1/0xd00
> > [    7.275359] Read of size 8 at addr ffff8801d444a000 by task ip/370
> > [    7.275488] 
> > [    7.275610] CPU: 1 PID: 370 Comm: ip Not tainted 4.18.0-rc6+ #1
> > [    7.275613] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > [    7.275616] Call Trace:
> > [    7.275621]  <IRQ>
> > [    7.275630]  dump_stack+0x71/0xab
> > [    7.275640]  print_address_description+0x6a/0x270
> > [    7.275648]  kasan_report+0x258/0x380
> > [    7.275653]  ? virtnet_poll+0xaa1/0xd00
> > [    7.275661]  virtnet_poll+0xaa1/0xd00
> > [    7.275680]  ? receive_buf+0x5920/0x5920
> > [    7.275689]  ? do_raw_spin_unlock+0x54/0x220
> > [    7.275699]  ? find_held_lock+0x32/0x1c0
> > [    7.275710]  ? rcu_process_callbacks+0xa60/0xd20
> > [    7.275736]  net_rx_action+0x2ee/0xad0
> > [    7.275748]  ? rcu_note_context_switch+0x320/0x320
> > [    7.275754]  ? napi_complete_done+0x300/0x300
> > [    7.275763]  ? native_apic_msr_write+0x27/0x30
> > [    7.275768]  ? lapic_next_event+0x5b/0x90
> > [    7.275775]  ? clockevents_program_event+0x21d/0x2f0
> > [    7.275791]  __do_softirq+0x19a/0x623
> > [    7.275807]  do_softirq_own_stack+0x2a/0x40
> > [    7.275811]  </IRQ>
> > [    7.275818]  do_softirq.part.18+0x6a/0x80
> > [    7.275825]  __local_bh_enable_ip+0x49/0x50
> > [    7.275829]  virtnet_open+0x129/0x440
> > [    7.275841]  __dev_open+0x189/0x2c0
> > [    7.275848]  ? dev_set_rx_mode+0x30/0x30
> > [    7.275857]  ? do_raw_spin_unlock+0x54/0x220
> > [    7.275866]  __dev_change_flags+0x3a9/0x4f0
> > [    7.275873]  ? dev_set_allmulti+0x10/0x10
> > [    7.275889]  dev_change_flags+0x7a/0x150
> > [    7.275900]  do_setlink+0x9fe/0x2e40
> > [    7.275910]  ? deref_stack_reg+0xad/0xe0
> > [    7.275917]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> > [    7.275922]  ? find_held_lock+0x32/0x1c0
> > [    7.275929]  ? rtnetlink_put_metrics+0x460/0x460
> > [    7.275935]  ? virtqueue_add_sgs+0x9e2/0xde0
> > [    7.275953]  ? virtscsi_add_cmd+0x454/0x780
> > [    7.275964]  ? find_held_lock+0x32/0x1c0
> > [    7.275973]  ? deref_stack_reg+0xad/0xe0
> > [    7.275979]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> > [    7.275985]  ? lock_downgrade+0x5e0/0x5e0
> > [    7.275993]  ? memset+0x1f/0x40
> > [    7.276008]  ? nla_parse+0x33/0x290
> > [    7.276016]  rtnl_newlink+0x954/0x1120
> > [    7.276030]  ? rtnl_link_unregister+0x250/0x250
> > [    7.276044]  ? is_bpf_text_address+0x5/0x60
> > [    7.276054]  ? lock_downgrade+0x5e0/0x5e0
> > [    7.276057]  ? lock_acquire+0x10b/0x2a0
> > [    7.276072]  ? deref_stack_reg+0xad/0xe0
> > [    7.276078]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> > [    7.276085]  ? __kernel_text_address+0xe/0x30
> > [    7.276090]  ? unwind_get_return_address+0x5f/0xa0
> > [    7.276103]  ? find_held_lock+0x32/0x1c0
> > [    7.276110]  ? is_bpf_text_address+0x5/0x60
> > [    7.276124]  ? deref_stack_reg+0xad/0xe0
> > [    7.276131]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> > [    7.276136]  ? depot_save_stack+0x2d9/0x460
> > [    7.276142]  ? deref_stack_reg+0xad/0xe0
> > [    7.276156]  ? find_held_lock+0x32/0x1c0
> > [    7.276164]  ? is_bpf_text_address+0x5/0x60
> > [    7.276170]  ? __lock_acquire.isra.29+0xe8/0x1bb0
> > [    7.276212]  ? rtnetlink_rcv_msg+0x5d6/0x930
> > [    7.276222]  ? lock_downgrade+0x5e0/0x5e0
> > [    7.276226]  ? lock_acquire+0x10b/0x2a0
> > [    7.276240]  rtnetlink_rcv_msg+0x69e/0x930
> > [    7.276249]  ? rtnl_calcit.isra.31+0x2f0/0x2f0
> > [    7.276255]  ? __lock_acquire.isra.29+0xe8/0x1bb0
> > [    7.276265]  ? netlink_deliver_tap+0x8d/0x8e0
> > [    7.276276]  netlink_rcv_skb+0x127/0x350
> > [    7.276281]  ? rtnl_calcit.isra.31+0x2f0/0x2f0
> > [    7.276289]  ? netlink_ack+0x970/0x970
> > [    7.276299]  ? __alloc_skb+0xc2/0x520
> > [    7.276311]  netlink_unicast+0x40f/0x5d0
> > [    7.276320]  ? netlink_attachskb+0x580/0x580
> > [    7.276325]  ? _copy_from_iter_full+0x157/0x6f0
> > [    7.276331]  ? import_iovec+0x90/0x390
> > [    7.276338]  ? get_page_from_freelist+0x1e89/0x3e30
> > [    7.276347]  ? apparmor_socket_sock_rcv_skb+0x10/0x10
> > [    7.276357]  netlink_sendmsg+0x65e/0xb00
> > [    7.276367]  ? netlink_unicast+0x5d0/0x5d0
> > [    7.276373]  ? copy_msghdr_from_user+0x206/0x340
> > [    7.276388]  ? netlink_unicast+0x5d0/0x5d0
> > [    7.276394]  sock_sendmsg+0xb3/0xf0
> > [    7.276401]  ___sys_sendmsg+0x604/0x8b0
> > [    7.276410]  ? copy_msghdr_from_user+0x340/0x340
> > [    7.276416]  ? find_held_lock+0x32/0x1c0
> > [    7.276424]  ? __handle_mm_fault+0xc85/0x3140
> > [    7.276433]  ? lock_downgrade+0x5e0/0x5e0
> > [    7.276439]  ? mem_cgroup_commit_charge+0xb4/0xf80
> > [    7.276453]  ? _raw_spin_unlock+0x24/0x30
> > [    7.276458]  ? __handle_mm_fault+0xc85/0x3140
> > [    7.276467]  ? __pmd_alloc+0x430/0x430
> > [    7.276473]  ? find_held_lock+0x32/0x1c0
> > [    7.276485]  ? __fget_light+0x55/0x1f0
> > [    7.276497]  ? __sys_sendmsg+0xd2/0x170
> > [    7.276502]  __sys_sendmsg+0xd2/0x170
> > [    7.276508]  ? __ia32_sys_shutdown+0x70/0x70
> > [    7.276516]  ? handle_mm_fault+0x1f9/0x6a0
> > [    7.276528]  ? up_read+0x1c/0x110
> > [    7.276534]  ? __do_page_fault+0x4a6/0xa80
> > [    7.276554]  do_syscall_64+0xa0/0x280
> > [    7.276558]  ? prepare_exit_to_usermode+0x88/0x130
> > [    7.276566]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [    7.276572] RIP: 0033:0x7ffbe9a2f160
> > [    7.276574] Code: c3 48 8b 05 2a 2d 2c 00 f7 db 64 89 18 48 83 cb ff eb dd 0f 1f 80 00 00 00 00 83 3d 1d 8f 2c 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 ee cb 00 00 48 89 04 24 
> > [    7.276728] RSP: 002b:00007ffc5a2d6108 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> > [    7.276735] RAX: ffffffffffffffda RBX: 00007ffc5a2da220 RCX: 00007ffbe9a2f160
> > [    7.276738] RDX: 0000000000000000 RSI: 00007ffc5a2d6150 RDI: 0000000000000003
> > [    7.276741] RBP: 00007ffc5a2d6150 R08: 0000000000000000 R09: 0000000000000003
> > [    7.276744] R10: 00007ffc5a2d5ed0 R11: 0000000000000246 R12: 000000005b6177de
> > [    7.276748] R13: 0000000000000000 R14: 00000000006473a0 R15: 00007ffc5a2da918
> > [    7.276763] 
> > [    7.276895] Allocated by task 1:
> > [    7.277026]  kasan_kmalloc+0xa0/0xd0
> > [    7.277030]  __kmalloc+0x13a/0x250
> > [    7.277034]  init_vqs+0xd0/0x11c0
> > [    7.277038]  virtnet_probe+0xb99/0x1ad0
> > [    7.277045]  virtio_dev_probe+0x3fc/0x890
> > [    7.277052]  driver_probe_device+0x6c4/0xcc0
> > [    7.277056]  __driver_attach+0x232/0x2c0
> > [    7.277060]  bus_for_each_dev+0x118/0x1a0
> > [    7.277064]  bus_add_driver+0x390/0x6e0
> > [    7.277068]  driver_register+0x18e/0x400
> > [    7.277076]  virtio_net_driver_init+0x6d/0x90
> > [    7.277080]  do_one_initcall+0xa8/0x348
> > [    7.277085]  kernel_init_freeable+0x42d/0x4c8
> > [    7.277090]  kernel_init+0xf/0x130
> > [    7.277095]  ret_from_fork+0x35/0x40
> > [    7.277097] 
> > [    7.277223] Freed by task 0:
> > [    7.277347] (stack is not available)
> > [    7.277473] 
> > [    7.277596] The buggy address belongs to the object at ffff8801d4449100
> > [    7.277596]  which belongs to the cache kmalloc-4096 of size 4096
> > [    7.277769] The buggy address is located 3840 bytes inside of
> > [    7.277769]  4096-byte region [ffff8801d4449100, ffff8801d444a100)
> > [    7.277932] The buggy address belongs to the page:
> > [    7.278066] page:ffffea0007511200 count:1 mapcount:0 mapping:ffff8801db002600 index:0x0 compound_mapcount: 0
> > [    7.278230] flags: 0x17fff8000008100(slab|head)
> > [    7.278363] raw: 017fff8000008100 dead000000000100 dead000000000200 ffff8801db002600
> > [    7.278516] raw: 0000000000000000 0000000000070007 00000001ffffffff 0000000000000000
> > [    7.278664] page dumped because: kasan: bad access detected
> > [    7.278790] 
> > [    7.278904] Memory state around the buggy address:
> > [    7.279031]  ffff8801d4449f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [    7.279175]  ffff8801d4449f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [    7.279323] >ffff8801d444a000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [    7.279468]                    ^
> > [    7.279584]  ffff8801d444a080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [    7.279729]  ffff8801d444a100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [    7.279870] ==================================================================
> > [    7.280011] Disabling lock debugging due to kernel taint
> > [    7.632219] random: mktemp: uninitialized urandom read (6 bytes read)
> > [    8.052850] random: mktemp: uninitialized urandom read (6 bytes read)
> > [    8.335209] random: cloud-init: uninitialized urandom read (32 bytes read)
> > [    8.796331] random: cloud-init: uninitialized urandom read (32 bytes read)
> > [    9.162551] random: mktemp: uninitialized urandom read (12 bytes read)
> > [    9.384504] random: ssh-keygen: uninitialized urandom read (32 bytes read)
> > [    9.839586] init: failsafe main process (724) killed by TERM signal
> > [   14.865443] postgres (1245): /proc/1245/oom_adj is deprecated, please use /proc/1245/oom_score_adj instead.
> > [   17.213418] random: crng init done
> > [   17.580892] init: plymouth-upstart-bridge main process ended, respawning
> 
> I suspect an off by one somewhere. I'm looking at the patch
> and I don't see it but these things are hard to spot sometimes ...
>

It was reproduced only once, so the problem can be unrelated with this
patch.

> > > 
> > > > >> But this may not a
> > > > >> clean solution. It'd help if I can get suggestions on what would be a
> > > > >> clean option to fix this without extensively changing the code in
> > > > >> virtio_net. Is it mandatory to protect the affinitization with
> > > > >> read_lock? I don't see similar lock in other drivers while setting the
> > > > >> affinity. I understand this warning should go away, but isn't it safe to
> > > > >> have multiple readers.
> > > > >>
> > > > >>> On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote:

Re: [net-next, v6, 6/7] net-sysfs: Add interface for Rx queue(s) map per Tx queue

[Andrei Vagin]

On Fri, Aug 03, 2018 at 10:12:53PM +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 03, 2018 at 12:06:51PM -0700, Andrei Vagin wrote:
> > On Fri, Aug 03, 2018 at 12:08:05AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Aug 02, 2018 at 02:04:12PM -0700, Nambiar, Amritha wrote:
> > > > On 8/1/2018 5:11 PM, Andrei Vagin wrote:
> > > > > On Tue, Jul 10, 2018 at 07:28:49PM -0700, Nambiar, Amritha wrote:
> > > > >> With this patch series, I introduced static_key for XPS maps
> > > > >> (xps_needed), so static_key_slow_inc() is used to switch branches. The
> > > > >> definition of static_key_slow_inc() has cpus_read_lock in place. In the
> > > > >> virtio_net driver, XPS queues are initialized after setting the
> > > > >> queue:cpu affinity in virtnet_set_affinity() which is already protected
> > > > >> within cpus_read_lock. Hence, the warning here trying to acquire
> > > > >> cpus_read_lock when it is already held.
> > > > >>
> > > > >> A quick fix for this would be to just extract netif_set_xps_queue() out
> > > > >> of the lock by simply wrapping it with another put/get_online_cpus
> > > > >> (unlock right before and hold lock right after).
> > > > > 
> > > > > virtnet_set_affinity() is called from virtnet_cpu_online(), which is
> > > > > called under cpus_read_lock too.
> > > > > 
> > > > > It looks like now we can't call netif_set_xps_queue() from cpu hotplug
> > > > > callbacks.
> > > > > 
> > > > > I can suggest a very straightforward fix for this problem. The patch is
> > > > > attached.
> > > > > 
> > > > 
> > > > Thanks for looking into this. I was thinking of fixing this in the
> > > > virtio_net driver by moving the XPS initialization (and have a new
> > > > get_affinity utility) in the ndo_open (so it is together with other tx
> > > > preparation) instead of probe. Your patch solves this in general for
> > > > setting up cpu hotplug callbacks which is under cpus_read_lock.
> > > 
> > > 
> > > I like this too. Could you repost in a standard way
> > > (inline, with your signoff etc) so we can ack this for
> > > net-next?
> > 
> > When I was testing this patch, I got the following kasan warning. Michael,
> > could you take a look at it. Maybe you will understand what was going wrong there.
> > 
> > https://api.travis-ci.org/v3/job/410701353/log.txt
> > 
> > [    7.275033] ==================================================================
> > [    7.275226] BUG: KASAN: slab-out-of-bounds in virtnet_poll+0xaa1/0xd00
> > [    7.275359] Read of size 8 at addr ffff8801d444a000 by task ip/370
> > [    7.275488] 
> > [    7.275610] CPU: 1 PID: 370 Comm: ip Not tainted 4.18.0-rc6+ #1
> > [    7.275613] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > [    7.275616] Call Trace:
> > [    7.275621]  <IRQ>
> > [    7.275630]  dump_stack+0x71/0xab
> > [    7.275640]  print_address_description+0x6a/0x270
> > [    7.275648]  kasan_report+0x258/0x380
> > [    7.275653]  ? virtnet_poll+0xaa1/0xd00
> > [    7.275661]  virtnet_poll+0xaa1/0xd00
> > [    7.275680]  ? receive_buf+0x5920/0x5920
> > [    7.275689]  ? do_raw_spin_unlock+0x54/0x220
> > [    7.275699]  ? find_held_lock+0x32/0x1c0
> > [    7.275710]  ? rcu_process_callbacks+0xa60/0xd20
> > [    7.275736]  net_rx_action+0x2ee/0xad0
> > [    7.275748]  ? rcu_note_context_switch+0x320/0x320
> > [    7.275754]  ? napi_complete_done+0x300/0x300
> > [    7.275763]  ? native_apic_msr_write+0x27/0x30
> > [    7.275768]  ? lapic_next_event+0x5b/0x90
> > [    7.275775]  ? clockevents_program_event+0x21d/0x2f0
> > [    7.275791]  __do_softirq+0x19a/0x623
> > [    7.275807]  do_softirq_own_stack+0x2a/0x40
> > [    7.275811]  </IRQ>
> > [    7.275818]  do_softirq.part.18+0x6a/0x80
> > [    7.275825]  __local_bh_enable_ip+0x49/0x50
> > [    7.275829]  virtnet_open+0x129/0x440
> > [    7.275841]  __dev_open+0x189/0x2c0
> > [    7.275848]  ? dev_set_rx_mode+0x30/0x30
> > [    7.275857]  ? do_raw_spin_unlock+0x54/0x220
> > [    7.275866]  __dev_change_flags+0x3a9/0x4f0
> > [    7.275873]  ? dev_set_allmulti+0x10/0x10
> > [    7.275889]  dev_change_flags+0x7a/0x150
> > [    7.275900]  do_setlink+0x9fe/0x2e40
> > [    7.275910]  ? deref_stack_reg+0xad/0xe0
> > [    7.275917]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> > [    7.275922]  ? find_held_lock+0x32/0x1c0
> > [    7.275929]  ? rtnetlink_put_metrics+0x460/0x460
> > [    7.275935]  ? virtqueue_add_sgs+0x9e2/0xde0
> > [    7.275953]  ? virtscsi_add_cmd+0x454/0x780
> > [    7.275964]  ? find_held_lock+0x32/0x1c0
> > [    7.275973]  ? deref_stack_reg+0xad/0xe0
> > [    7.275979]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> > [    7.275985]  ? lock_downgrade+0x5e0/0x5e0
> > [    7.275993]  ? memset+0x1f/0x40
> > [    7.276008]  ? nla_parse+0x33/0x290
> > [    7.276016]  rtnl_newlink+0x954/0x1120
> > [    7.276030]  ? rtnl_link_unregister+0x250/0x250
> > [    7.276044]  ? is_bpf_text_address+0x5/0x60
> > [    7.276054]  ? lock_downgrade+0x5e0/0x5e0
> > [    7.276057]  ? lock_acquire+0x10b/0x2a0
> > [    7.276072]  ? deref_stack_reg+0xad/0xe0
> > [    7.276078]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> > [    7.276085]  ? __kernel_text_address+0xe/0x30
> > [    7.276090]  ? unwind_get_return_address+0x5f/0xa0
> > [    7.276103]  ? find_held_lock+0x32/0x1c0
> > [    7.276110]  ? is_bpf_text_address+0x5/0x60
> > [    7.276124]  ? deref_stack_reg+0xad/0xe0
> > [    7.276131]  ? __read_once_size_nocheck.constprop.6+0x10/0x10
> > [    7.276136]  ? depot_save_stack+0x2d9/0x460
> > [    7.276142]  ? deref_stack_reg+0xad/0xe0
> > [    7.276156]  ? find_held_lock+0x32/0x1c0
> > [    7.276164]  ? is_bpf_text_address+0x5/0x60
> > [    7.276170]  ? __lock_acquire.isra.29+0xe8/0x1bb0
> > [    7.276212]  ? rtnetlink_rcv_msg+0x5d6/0x930
> > [    7.276222]  ? lock_downgrade+0x5e0/0x5e0
> > [    7.276226]  ? lock_acquire+0x10b/0x2a0
> > [    7.276240]  rtnetlink_rcv_msg+0x69e/0x930
> > [    7.276249]  ? rtnl_calcit.isra.31+0x2f0/0x2f0
> > [    7.276255]  ? __lock_acquire.isra.29+0xe8/0x1bb0
> > [    7.276265]  ? netlink_deliver_tap+0x8d/0x8e0
> > [    7.276276]  netlink_rcv_skb+0x127/0x350
> > [    7.276281]  ? rtnl_calcit.isra.31+0x2f0/0x2f0
> > [    7.276289]  ? netlink_ack+0x970/0x970
> > [    7.276299]  ? __alloc_skb+0xc2/0x520
> > [    7.276311]  netlink_unicast+0x40f/0x5d0
> > [    7.276320]  ? netlink_attachskb+0x580/0x580
> > [    7.276325]  ? _copy_from_iter_full+0x157/0x6f0
> > [    7.276331]  ? import_iovec+0x90/0x390
> > [    7.276338]  ? get_page_from_freelist+0x1e89/0x3e30
> > [    7.276347]  ? apparmor_socket_sock_rcv_skb+0x10/0x10
> > [    7.276357]  netlink_sendmsg+0x65e/0xb00
> > [    7.276367]  ? netlink_unicast+0x5d0/0x5d0
> > [    7.276373]  ? copy_msghdr_from_user+0x206/0x340
> > [    7.276388]  ? netlink_unicast+0x5d0/0x5d0
> > [    7.276394]  sock_sendmsg+0xb3/0xf0
> > [    7.276401]  ___sys_sendmsg+0x604/0x8b0
> > [    7.276410]  ? copy_msghdr_from_user+0x340/0x340
> > [    7.276416]  ? find_held_lock+0x32/0x1c0
> > [    7.276424]  ? __handle_mm_fault+0xc85/0x3140
> > [    7.276433]  ? lock_downgrade+0x5e0/0x5e0
> > [    7.276439]  ? mem_cgroup_commit_charge+0xb4/0xf80
> > [    7.276453]  ? _raw_spin_unlock+0x24/0x30
> > [    7.276458]  ? __handle_mm_fault+0xc85/0x3140
> > [    7.276467]  ? __pmd_alloc+0x430/0x430
> > [    7.276473]  ? find_held_lock+0x32/0x1c0
> > [    7.276485]  ? __fget_light+0x55/0x1f0
> > [    7.276497]  ? __sys_sendmsg+0xd2/0x170
> > [    7.276502]  __sys_sendmsg+0xd2/0x170
> > [    7.276508]  ? __ia32_sys_shutdown+0x70/0x70
> > [    7.276516]  ? handle_mm_fault+0x1f9/0x6a0
> > [    7.276528]  ? up_read+0x1c/0x110
> > [    7.276534]  ? __do_page_fault+0x4a6/0xa80
> > [    7.276554]  do_syscall_64+0xa0/0x280
> > [    7.276558]  ? prepare_exit_to_usermode+0x88/0x130
> > [    7.276566]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [    7.276572] RIP: 0033:0x7ffbe9a2f160
> > [    7.276574] Code: c3 48 8b 05 2a 2d 2c 00 f7 db 64 89 18 48 83 cb ff eb dd 0f 1f 80 00 00 00 00 83 3d 1d 8f 2c 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 ee cb 00 00 48 89 04 24 
> > [    7.276728] RSP: 002b:00007ffc5a2d6108 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> > [    7.276735] RAX: ffffffffffffffda RBX: 00007ffc5a2da220 RCX: 00007ffbe9a2f160
> > [    7.276738] RDX: 0000000000000000 RSI: 00007ffc5a2d6150 RDI: 0000000000000003
> > [    7.276741] RBP: 00007ffc5a2d6150 R08: 0000000000000000 R09: 0000000000000003
> > [    7.276744] R10: 00007ffc5a2d5ed0 R11: 0000000000000246 R12: 000000005b6177de
> > [    7.276748] R13: 0000000000000000 R14: 00000000006473a0 R15: 00007ffc5a2da918
> > [    7.276763] 
> > [    7.276895] Allocated by task 1:
> > [    7.277026]  kasan_kmalloc+0xa0/0xd0
> > [    7.277030]  __kmalloc+0x13a/0x250
> > [    7.277034]  init_vqs+0xd0/0x11c0
> > [    7.277038]  virtnet_probe+0xb99/0x1ad0
> > [    7.277045]  virtio_dev_probe+0x3fc/0x890
> > [    7.277052]  driver_probe_device+0x6c4/0xcc0
> > [    7.277056]  __driver_attach+0x232/0x2c0
> > [    7.277060]  bus_for_each_dev+0x118/0x1a0
> > [    7.277064]  bus_add_driver+0x390/0x6e0
> > [    7.277068]  driver_register+0x18e/0x400
> > [    7.277076]  virtio_net_driver_init+0x6d/0x90
> > [    7.277080]  do_one_initcall+0xa8/0x348
> > [    7.277085]  kernel_init_freeable+0x42d/0x4c8
> > [    7.277090]  kernel_init+0xf/0x130
> > [    7.277095]  ret_from_fork+0x35/0x40
> > [    7.277097] 
> > [    7.277223] Freed by task 0:
> > [    7.277347] (stack is not available)
> > [    7.277473] 
> > [    7.277596] The buggy address belongs to the object at ffff8801d4449100
> > [    7.277596]  which belongs to the cache kmalloc-4096 of size 4096
> > [    7.277769] The buggy address is located 3840 bytes inside of
> > [    7.277769]  4096-byte region [ffff8801d4449100, ffff8801d444a100)
> > [    7.277932] The buggy address belongs to the page:
> > [    7.278066] page:ffffea0007511200 count:1 mapcount:0 mapping:ffff8801db002600 index:0x0 compound_mapcount: 0
> > [    7.278230] flags: 0x17fff8000008100(slab|head)
> > [    7.278363] raw: 017fff8000008100 dead000000000100 dead000000000200 ffff8801db002600
> > [    7.278516] raw: 0000000000000000 0000000000070007 00000001ffffffff 0000000000000000
> > [    7.278664] page dumped because: kasan: bad access detected
> > [    7.278790] 
> > [    7.278904] Memory state around the buggy address:
> > [    7.279031]  ffff8801d4449f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [    7.279175]  ffff8801d4449f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [    7.279323] >ffff8801d444a000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [    7.279468]                    ^
> > [    7.279584]  ffff8801d444a080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [    7.279729]  ffff8801d444a100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [    7.279870] ==================================================================
> > [    7.280011] Disabling lock debugging due to kernel taint
> > [    7.632219] random: mktemp: uninitialized urandom read (6 bytes read)
> > [    8.052850] random: mktemp: uninitialized urandom read (6 bytes read)
> > [    8.335209] random: cloud-init: uninitialized urandom read (32 bytes read)
> > [    8.796331] random: cloud-init: uninitialized urandom read (32 bytes read)
> > [    9.162551] random: mktemp: uninitialized urandom read (12 bytes read)
> > [    9.384504] random: ssh-keygen: uninitialized urandom read (32 bytes read)
> > [    9.839586] init: failsafe main process (724) killed by TERM signal
> > [   14.865443] postgres (1245): /proc/1245/oom_adj is deprecated, please use /proc/1245/oom_score_adj instead.
> > [   17.213418] random: crng init done
> > [   17.580892] init: plymouth-upstart-bridge main process ended, respawning
> 
> I suspect an off by one somewhere. I'm looking at the patch
> and I don't see it but these things are hard to spot sometimes ...

This bug was fixed in this commit:
commit ca9e83b4a55bfa1cc1395b48c3bf70381833526b
Author: Jason Wang <jasowang@redhat.com>
Date:   Tue Jul 31 17:43:38 2018 +0800

    virtio-net: correctly update XDP_TX counters

> 
> > > 
> > > > >> But this may not a
> > > > >> clean solution. It'd help if I can get suggestions on what would be a
> > > > >> clean option to fix this without extensively changing the code in
> > > > >> virtio_net. Is it mandatory to protect the affinitization with
> > > > >> read_lock? I don't see similar lock in other drivers while setting the
> > > > >> affinity. I understand this warning should go away, but isn't it safe to
> > > > >> have multiple readers.
> > > > >>
> > > > >>> On Fri, Jun 29, 2018 at 09:27:07PM -0700, Amritha Nambiar wrote: