Skip to contentSkip to conversation

fixes for XPS configuration after Symmetric queue selection

PATCH In review Sabrina Dubroca 2 participants 4 messages Nov 30, 2018

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Sabrina Dubroca

author
#0 lore raw

cover fixes for XPS configuration after Symmetric queue selection

This fixes some bugs introduced by the "Symmetric queue selection using XPS for Rx queues".

First, the refactoring of the cleanup function skipped resetting the queue's NUMA node under some conditions.

Second, the accounting on static keys for XPS and RXQS-XPS is unbalanced, so the static key for XPS won't actually disable itself, once enabled. The RXQS-XPS static key can actually be disabled by reconfiguring a device that didn't have RXQS-XPS configured at all.

Sabrina Dubroca (2):
net: restore call to netdev_queue_numa_node_write when resetting XPS
net: fix XPS static_key accounting

 net/core/dev.c | 53 +++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 24 deletions(-)
Signature · 1 line
2.19.2
author
#1 lore raw

PATCH 1/2 net: restore call to netdev_queue_numa_node_write when resetting XPS

Before commit 80d19669ecd3 ("net: Refactor XPS for CPUs and Rx queues"), netif_reset_xps_queues() did netdev_queue_numa_node_write() for all the queues being reset. Now, this is only done when the "active" variable in clean_xps_maps() is false, ie when on all the CPUs, there's no active XPS mapping left.

Fixes 80d19669ecd3 ("net: Refactor XPS for CPUs and Rx queues")
Signed-off-by Sabrina Dubroca <[email protected]>
---
 net/core/dev.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
net/core/dev.c +9 -7
index ddc551f24ba2..32a63f4c3a92 100644
@@ -2187,17 +2187,19 @@ static void clean_xps_maps(struct net_device *dev, const unsigned long *mask,
 		active |= remove_xps_queue_cpu(dev, dev_maps, j, offset,
 					       count);
 	if (!active) {
-		if (is_rxqs_map) {
+		if (is_rxqs_map)
 			RCU_INIT_POINTER(dev->xps_rxqs_map, NULL);
-		} else {
+		else
 			RCU_INIT_POINTER(dev->xps_cpus_map, 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);
+	if (!is_rxqs_map) {
+		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);
 	}
 }
Signature · 1 line
2.19.2
author
#2 lore raw

PATCH 2/2 net: fix XPS static_key accounting

In reply to Sabrina Dubroca

Commit 04157469b7b8 ("net: Use static_key for XPS maps") introduced a static key for XPS, but the increments/decrements don't match.

First, the static key's counter is incremented once for each queue, but only decremented once for a whole batch of queues, leading to large unbalances.

Second, the xps_rxqs_needed key is decremented whenever we reset a batch of queues, whether they had any rxqs mapping or not, so that if we setup cpu-XPS on em1 and RXQS-XPS on em2, resetting the queues on em1 would decrement the xps_rxqs_needed key.

This reworks the accounting scheme so that the xps_needed key is incremented only once for each type of XPS for all the queues on a device, and the xps_rxqs_needed key is incremented only once for all queues. This is sufficient to let us retrieve queues via get_xps_queue().

This patch introduces a new reset_xps_maps(), which reinitializes and frees the appropriate map (xps_rxqs_map or xps_cpus_map), and drops a reference to the needed keys:
- both xps_needed and xps_rxqs_needed, in case of rxqs maps,
- only xps_needed, in case of CPU maps.

Now, we also need to call reset_xps_maps() at the end of __netif_set_xps_queue() when there's no active map left, for example when writing '00000000,00000000' to all queues' xps_rxqs setting.

Fixes 04157469b7b8 ("net: Use static_key for XPS maps")
Signed-off-by Sabrina Dubroca <[email protected]>
---
 net/core/dev.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)
net/core/dev.c +24 -21
index 32a63f4c3a92..3470e7fff1f4 100644
@@ -2175,6 +2175,20 @@ static bool remove_xps_queue_cpu(struct net_device *dev,
 	return active;
 }
 
+static void reset_xps_maps(struct net_device *dev,
+			   struct xps_dev_maps *dev_maps,
+			   bool is_rxqs_map)
+{
+	if (is_rxqs_map) {
+		static_key_slow_dec_cpuslocked(&xps_rxqs_needed);
+		RCU_INIT_POINTER(dev->xps_rxqs_map, NULL);
+	} else {
+		RCU_INIT_POINTER(dev->xps_cpus_map, NULL);
+	}
+	static_key_slow_dec_cpuslocked(&xps_needed);
+	kfree_rcu(dev_maps, rcu);
+}
+
 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)

@@ -2186,13 +2200,8 @@ static void clean_xps_maps(struct net_device *dev, const unsigned long *mask,
 	     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);
-		kfree_rcu(dev_maps, rcu);
-	}
+	if (!active)
+		reset_xps_maps(dev, dev_maps, is_rxqs_map);
 
 	if (!is_rxqs_map) {
 		for (i = offset + (count - 1); count--; i--) {

@@ -2236,10 +2245,6 @@ 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_cpuslocked(&xps_rxqs_needed);
-
-	static_key_slow_dec_cpuslocked(&xps_needed);
 	mutex_unlock(&xps_map_mutex);
 	cpus_read_unlock();
 }

@@ -2357,9 +2362,12 @@ 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_cpuslocked(&xps_needed);
-	if (is_rxqs_map)
-		static_key_slow_inc_cpuslocked(&xps_rxqs_needed);
+	if (!dev_maps) {
+		/* Increment static keys at most once per type */
+		static_key_slow_inc_cpuslocked(&xps_needed);
+		if (is_rxqs_map)
+			static_key_slow_inc_cpuslocked(&xps_rxqs_needed);
+	}
 
 	for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids),
 	     j < nr_ids;) {

@@ -2457,13 +2465,8 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
 	}
 
 	/* free map if not active */
-	if (!active) {
-		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);
-	}
+	if (!active)
+		reset_xps_maps(dev, dev_maps, is_rxqs_map);
 
 out_no_maps:
 	mutex_unlock(&xps_map_mutex);
Signature · 1 line
2.19.2
maintainer
#3 lore raw
In reply to Sabrina Dubroca

From: Sabrina Dubroca <[email protected]>
Date: Thu, 29 Nov 2018 14:14:47 +0100

Quoted 10 lines

This fixes some bugs introduced by the "Symmetric queue selection using XPS for Rx queues".

First, the refactoring of the cleanup function skipped resetting the queue's NUMA node under some conditions.

Second, the accounting on static keys for XPS and RXQS-XPS is unbalanced, so the static key for XPS won't actually disable itself, once enabled. The RXQS-XPS static key can actually be disabled by reconfiguring a device that didn't have RXQS-XPS configured at all.

Series applied and queued up for -stable, thanks.