[PATCH v2 0/3] Preventing job distribution to isolated CPUs

                                                                           
Testing                                                                    
=======                                                                    
* Patch 1:                                                                 
  Fix for cpumask_local_spread() is tested by creating VFs, loading        
  iavf module and by adding a tracepoint to confirm that only housekeeping 
  CPUs are picked when an appropriate profile is set up and all remaining  
  CPUs when no CPU isolation is configured.                                
                                                                           
* Patch 2:                                                                 
  To test the PCI fix, I hotplugged a virtio-net-pci from qemu console     
  and forced its addition to a specific node to trigger the code path that 
  includes the proposed fix and verified that only housekeeping CPUs       
  are included via tracepoint.                                             
                                                                           
* Patch 3:                                                                 
  To test the fix in store_rps_map(), I tried configuring an isolated      
  CPU by writing to /sys/class/net/en*/queues/rx*/rps_cpus which           
  resulted in 'write error: Invalid argument' error. For the case          
  where a non-isolated CPU is writing in rps_cpus the above operation      
  succeeded without any error.                                             
                                                                           
                                                                           
Changes from v1:                                                           
===============                                                            
- Included the suggestions made by Bjorn Helgaas in the commit messages.    
- Included the 'Reviewed-by' and 'Acked-by' received for Patch-2.          
                                                                           
[1] https://patchwork.ozlabs.org/project/netdev/patch/51102eebe62336c6a4e584c7a503553b9f90e01c.camel@marvell.com/
                                                                           
Alex Belits (3):                                                           
  lib: Restrict cpumask_local_spread to houskeeping CPUs                   
  PCI: Restrict probe functions to housekeeping CPUs                       
  net: Restrict receive packets queuing to housekeeping CPUs               
                                                                           
 drivers/pci/pci-driver.c |  5 ++++-                                       
 lib/cpumask.c            | 43 +++++++++++++++++++++++-----------------    
 net/core/net-sysfs.c     | 10 +++++++++-                                  
 3 files changed, 38 insertions(+), 20 deletions(-)                        
                                                                           
--

From: Alex Belits <redacted>

The current implementation of cpumask_local_spread() does not respect the
isolated CPUs, i.e., even if a CPU has been isolated for Real-Time task,
it will return it to the caller for pinning of its IRQ threads. Having
these unwanted IRQ threads on an isolated CPU adds up to a latency
overhead.

Restrict the CPUs that are returned for spreading IRQs only to the
available housekeeping CPUs.

Signed-off-by: Alex Belits <redacted>
Signed-off-by: Nitesh Narayan Lal <redacted>
---
 lib/cpumask.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/lib/cpumask.c b/lib/cpumask.c
index fb22fb266f93..cc4311a8c079 100644
--- a/lib/cpumask.c
+++ b/lib/cpumask.c

@@ -6,6 +6,7 @@
 #include <linux/export.h>
 #include <linux/memblock.h>
 #include <linux/numa.h>
+#include <linux/sched/isolation.h>
 
 /**
  * cpumask_next - get the next cpu in a cpumask

@@ -205,28 +206,34 @@ void __init free_bootmem_cpumask_var(cpumask_var_t mask)
  */
 unsigned int cpumask_local_spread(unsigned int i, int node)
 {
-	int cpu;
+	int cpu, m, n, hk_flags;
+	const struct cpumask *mask;
 
+	hk_flags = HK_FLAG_DOMAIN | HK_FLAG_WQ;
+	mask = housekeeping_cpumask(hk_flags);
+	m = cpumask_weight(mask);
 	/* Wrap: we always want a cpu. */
-	i %= num_online_cpus();
+	n = i % m;
+	while (m-- > 0) {
+		if (node == NUMA_NO_NODE) {
+			for_each_cpu(cpu, mask)
+				if (n-- == 0)
+					return cpu;
+		} else {
+			/* NUMA first. */
+			for_each_cpu_and(cpu, cpumask_of_node(node), mask)
+				if (n-- == 0)
+					return cpu;
 
-	if (node == NUMA_NO_NODE) {
-		for_each_cpu(cpu, cpu_online_mask)
-			if (i-- == 0)
-				return cpu;
-	} else {
-		/* NUMA first. */
-		for_each_cpu_and(cpu, cpumask_of_node(node), cpu_online_mask)
-			if (i-- == 0)
-				return cpu;
+			for_each_cpu(cpu, mask) {
+				/* Skip NUMA nodes, done above. */
+				if (cpumask_test_cpu(cpu,
+						     cpumask_of_node(node)))
+					continue;
 
-		for_each_cpu(cpu, cpu_online_mask) {
-			/* Skip NUMA nodes, done above. */
-			if (cpumask_test_cpu(cpu, cpumask_of_node(node)))
-				continue;
-
-			if (i-- == 0)
-				return cpu;
+				if (n-- == 0)
+					return cpu;
+			}
 		}
 	}
 	BUG();

-- 
2.18.4

From: Alex Belits <redacted>

pci_call_probe() prevents the nesting of work_on_cpu() for a scenario
where a VF device is probed from work_on_cpu() of the PF.

Replace the cpumask used in pci_call_probe() from all online CPUs to only
housekeeping CPUs. This is to ensure that there are no additional latency
overheads caused due to the pinning of jobs on isolated CPUs.

Signed-off-by: Alex Belits <redacted>
Signed-off-by: Nitesh Narayan Lal <redacted>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
---
 drivers/pci/pci-driver.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index da6510af1221..449466f71040 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c

@@ -12,6 +12,7 @@
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
+#include <linux/sched/isolation.h>
 #include <linux/cpu.h>
 #include <linux/pm_runtime.h>
 #include <linux/suspend.h>

@@ -333,6 +334,7 @@ static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev,
 			  const struct pci_device_id *id)
 {
 	int error, node, cpu;
+	int hk_flags = HK_FLAG_DOMAIN | HK_FLAG_WQ;
 	struct drv_dev_and_id ddi = { drv, dev, id };
 
 	/*

@@ -353,7 +355,8 @@ static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev,
 	    pci_physfn_is_probed(dev))
 		cpu = nr_cpu_ids;
 	else
-		cpu = cpumask_any_and(cpumask_of_node(node), cpu_online_mask);
+		cpu = cpumask_any_and(cpumask_of_node(node),
+				      housekeeping_cpumask(hk_flags));
 
 	if (cpu < nr_cpu_ids)
 		error = work_on_cpu(cpu, local_pci_probe, &ddi);

-- 
2.18.4

From: Alex Belits <redacted>

With the existing implementation of store_rps_map(), packets are queued
in the receive path on the backlog queues of other CPUs irrespective of
whether they are isolated or not. This could add a latency overhead to
any RT workload that is running on the same CPU.

Ensure that store_rps_map() only uses available housekeeping CPUs for
storing the rps_map.

Signed-off-by: Alex Belits <redacted>
Signed-off-by: Nitesh Narayan Lal <redacted>
---
 net/core/net-sysfs.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index e353b822bb15..16e433287191 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c

@@ -11,6 +11,7 @@
 #include <linux/if_arp.h>
 #include <linux/slab.h>
 #include <linux/sched/signal.h>
+#include <linux/sched/isolation.h>
 #include <linux/nsproxy.h>
 #include <net/sock.h>
 #include <net/net_namespace.h>

@@ -741,7 +742,7 @@ static ssize_t store_rps_map(struct netdev_rx_queue *queue,
 {
 	struct rps_map *old_map, *map;
 	cpumask_var_t mask;
-	int err, cpu, i;
+	int err, cpu, i, hk_flags;
 	static DEFINE_MUTEX(rps_map_mutex);
 
 	if (!capable(CAP_NET_ADMIN))

@@ -756,6 +757,13 @@ static ssize_t store_rps_map(struct netdev_rx_queue *queue,
 		return err;
 	}
 
+	hk_flags = HK_FLAG_DOMAIN | HK_FLAG_WQ;
+	cpumask_and(mask, mask, housekeeping_cpumask(hk_flags));
+	if (cpumask_weight(mask) == 0) {
+		free_cpumask_var(mask);
+		return -EINVAL;
+	}
+
 	map = kzalloc(max_t(unsigned int,
 			    RPS_MAP_SIZE(cpumask_weight(mask)), L1_CACHE_BYTES),
 		      GFP_KERNEL);

-- 
2.18.4

On 6/22/20 7:45 PM, Nitesh Narayan Lal wrote:

                                                                           
Testing                                                                    
=======                                                                    
* Patch 1:                                                                 
  Fix for cpumask_local_spread() is tested by creating VFs, loading        
  iavf module and by adding a tracepoint to confirm that only housekeeping 
  CPUs are picked when an appropriate profile is set up and all remaining  
  CPUs when no CPU isolation is configured.                                
                                                                           
* Patch 2:                                                                 
  To test the PCI fix, I hotplugged a virtio-net-pci from qemu console     
  and forced its addition to a specific node to trigger the code path that 
  includes the proposed fix and verified that only housekeeping CPUs       
  are included via tracepoint.                                             
                                                                           
* Patch 3:                                                                 
  To test the fix in store_rps_map(), I tried configuring an isolated      
  CPU by writing to /sys/class/net/en*/queues/rx*/rps_cpus which           
  resulted in 'write error: Invalid argument' error. For the case          
  where a non-isolated CPU is writing in rps_cpus the above operation      
  succeeded without any error.                                             
                                                                           
                                                                           
Changes from v1:                                                           
===============                                                            
- Included the suggestions made by Bjorn Helgaas in the commit messages.    
- Included the 'Reviewed-by' and 'Acked-by' received for Patch-2.          
                                                                           
[1] https://patchwork.ozlabs.org/project/netdev/patch/51102eebe62336c6a4e584c7a503553b9f90e01c.camel@marvell.com/
                                                                           
Alex Belits (3):                                                           
  lib: Restrict cpumask_local_spread to houskeeping CPUs                   
  PCI: Restrict probe functions to housekeeping CPUs                       
  net: Restrict receive packets queuing to housekeeping CPUs               
                                                                           
 drivers/pci/pci-driver.c |  5 ++++-                                       
 lib/cpumask.c            | 43 +++++++++++++++++++++++-----------------    
 net/core/net-sysfs.c     | 10 +++++++++-                                  
 3 files changed, 38 insertions(+), 20 deletions(-)                        
                                                                           
--

Hi,

It seems that the cover email got messed up while I was sending the patches.
I am putting my intended cover-email below for now. I can send a v3 with proper
cover-email if needed. The reason, I am not sending it right now, is that if I
get some comments in my patches I will prefer including them as well in my
v3 posting.


"
This patch-set is originated from one of the patches that have been
posted earlier as a part of "Task_isolation" mode [1] patch series
by Alex Belits [off-list ref]. There are only a couple of
changes that I am proposing in this patch-set compared to what Alex
has posted earlier.


Context
=======
On a broad level, all three patches that are included in this patch
set are meant to improve the driver/library to respect isolated
CPUs by not pinning any job on it. Not doing so could impact
the latency values in RT use-cases.


Patches
=======
* Patch1:
  The first patch is meant to make cpumask_local_spread()
  aware of the isolated CPUs. It ensures that the CPUs that
  are returned by this API only includes housekeeping CPUs.

* Patch2:
  This patch ensures that a probe function that is called
  using work_on_cpu() doesn't run any task on an isolated CPU.

* Patch3:
  This patch makes store_rps_map() aware of the isolated
  CPUs so that rps don't queue any jobs on an isolated CPU.


Proposed Changes
================
To fix the above-mentioned issues Alex has used housekeeping_cpumask().
The only changes that I am proposing here are:
- Removing the dependency on CONFIG_TASK_ISOLATION that was proposed by
  Alex. As it should be safe to rely on housekeeping_cpumask()
  even when we don't have any isolated CPUs and we want
  to fall back to using all available CPUs in any of the above scenarios.
- Using both HK_FLAG_DOMAIN and HK_FLAG_WQ in all three patches, this is
  because we would want the above fixes not only when we have isolcpus but
  also with something like systemd's CPU affinity.


Testing
=======
* Patch 1:
  Fix for cpumask_local_spread() is tested by creating VFs, loading
  iavf module and by adding a tracepoint to confirm that only housekeeping
  CPUs are picked when an appropriate profile is set up and all remaining
  CPUs when no CPU isolation is configured.

* Patch 2:
  To test the PCI fix, I hotplugged a virtio-net-pci from qemu console
  and forced its addition to a specific node to trigger the code path that
  includes the proposed fix and verified that only housekeeping CPUs
  are included via tracepoint.

* Patch 3:
  To test the fix in store_rps_map(), I tried configuring an isolated
  CPU by writing to /sys/class/net/en*/queues/rx*/rps_cpus which
  resulted in 'write error: Invalid argument' error. For the case
  where a non-isolated CPU is writing in rps_cpus the above operation
  succeeded without any error.


Changes from v1: [2]
===============
- Included the suggestions made by Bjorn Helgaas in the commit message.
- Included the 'Reviewed-by' and 'Acked-by' received for Patch-2.

[1]
https://patchwork.ozlabs.org/project/netdev/patch/51102eebe62336c6a4e584c7a503553b9f90e01c.camel@marvell.com/
[2]
https://patchwork.ozlabs.org/project/linux-pci/cover/20200610161226.424337-1-nitesh@redhat.com/

Alex Belits (3):
  lib: Restrict cpumask_local_spread to houskeeping CPUs
  PCI: Restrict probe functions to housekeeping CPUs
  net: Restrict receive packets queuing to housekeeping CPUs

 drivers/pci/pci-driver.c |  5 ++++-
 lib/cpumask.c            | 43 +++++++++++++++++++++++-----------------
 net/core/net-sysfs.c     | 10 +++++++++-
 3 files changed, 38 insertions(+), 20 deletions(-)

-- 
"

-- 
Thanks
Nitesh

On Mon, Jun 22, 2020 at 07:45:08PM -0400, Nitesh Narayan Lal wrote:

I are confuzled. What do we need this outer loop for?

Why isn't something like:

	i %= cpumask_weight(mask);

good enough? That voids having to touch the test.
Still when you're there, at the very least you can fix the horrible
style:

+		if (node == NUMA_NO_NODE) {
+			for_each_cpu(cpu, mask)
+				if (n-- == 0)
+					return cpu;

{ }

+		} else {
+			/* NUMA first. */
+			for_each_cpu_and(cpu, cpumask_of_node(node), mask)
+				if (n-- == 0)
+					return cpu;

{ }

 
+			for_each_cpu(cpu, mask) {
+				/* Skip NUMA nodes, done above. */
+				if (cpumask_test_cpu(cpu,
+						     cpumask_of_node(node)))
+					continue;

No linebreak please.

 
+				if (n-- == 0)
+					return cpu;
+			}
 		}
 	}
 	BUG();
-- 
2.18.4

On Mon, Jun 22, 2020 at 07:45:10PM -0400, Nitesh Narayan Lal wrote:

We have cpumask_empty() for that, which is a much more efficient way of
testing the same.

+		free_cpumask_var(mask);
+		return -EINVAL;
+	}
+
 	map = kzalloc(max_t(unsigned int,
 			    RPS_MAP_SIZE(cpumask_weight(mask)), L1_CACHE_BYTES),
 		      GFP_KERNEL);
-- 
2.18.4

On 6/23/20 5:23 AM, Peter Zijlstra wrote:

On Mon, Jun 22, 2020 at 07:45:10PM -0400, Nitesh Narayan Lal wrote:

quoted

@@ -756,6 +757,13 @@ static ssize_t store_rps_map(struct netdev_rx_queue *queue,
 		return err;
 	}
 
+	hk_flags = HK_FLAG_DOMAIN | HK_FLAG_WQ;
+	cpumask_and(mask, mask, housekeeping_cpumask(hk_flags));
+	if (cpumask_weight(mask) == 0) {

We have cpumask_empty() for that, which is a much more efficient way of
testing the same.

Yes, right.
I will make this change.

quoted

+		free_cpumask_var(mask);
+		return -EINVAL;
+	}
+
 	map = kzalloc(max_t(unsigned int,
 			    RPS_MAP_SIZE(cpumask_weight(mask)), L1_CACHE_BYTES),
 		      GFP_KERNEL);
-- 
2.18.4

-- 
Thanks
Nitesh

On 6/23/20 5:21 AM, Peter Zijlstra wrote:

On Mon, Jun 22, 2020 at 07:45:08PM -0400, Nitesh Narayan Lal wrote:

quoted

From: Alex Belits <redacted>

The current implementation of cpumask_local_spread() does not respect the
isolated CPUs, i.e., even if a CPU has been isolated for Real-Time task,
it will return it to the caller for pinning of its IRQ threads. Having
these unwanted IRQ threads on an isolated CPU adds up to a latency
overhead.

Restrict the CPUs that are returned for spreading IRQs only to the
available housekeeping CPUs.

Signed-off-by: Alex Belits <redacted>
Signed-off-by: Nitesh Narayan Lal <redacted>
---
 lib/cpumask.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/lib/cpumask.c b/lib/cpumask.c
index fb22fb266f93..cc4311a8c079 100644
--- a/lib/cpumask.c
+++ b/lib/cpumask.c

@@ -6,6 +6,7 @@
 #include <linux/export.h>
 #include <linux/memblock.h>
 #include <linux/numa.h>
+#include <linux/sched/isolation.h>
 
 /**
  * cpumask_next - get the next cpu in a cpumask

@@ -205,28 +206,34 @@ void __init free_bootmem_cpumask_var(cpumask_var_t mask)
  */
 unsigned int cpumask_local_spread(unsigned int i, int node)
 {
-	int cpu;
+	int cpu, m, n, hk_flags;
+	const struct cpumask *mask;
 
+	hk_flags = HK_FLAG_DOMAIN | HK_FLAG_WQ;
+	mask = housekeeping_cpumask(hk_flags);
+	m = cpumask_weight(mask);
 	/* Wrap: we always want a cpu. */
-	i %= num_online_cpus();
+	n = i % m;
+	while (m-- > 0) {

I are confuzled. What do we need this outer loop for?

Why isn't something like:

	i %= cpumask_weight(mask);

good enough? That voids having to touch the test.

Makes sense.
Thanks

Still when you're there, at the very least you can fix the horrible
style:

Sure.

quoted

+		if (node == NUMA_NO_NODE) {
+			for_each_cpu(cpu, mask)
+				if (n-- == 0)
+					return cpu;

{ }

quoted

+		} else {
+			/* NUMA first. */
+			for_each_cpu_and(cpu, cpumask_of_node(node), mask)
+				if (n-- == 0)
+					return cpu;

{ }

quoted

 
+			for_each_cpu(cpu, mask) {
+				/* Skip NUMA nodes, done above. */
+				if (cpumask_test_cpu(cpu,
+						     cpumask_of_node(node)))
+					continue;

No linebreak please.

quoted

 
+				if (n-- == 0)
+					return cpu;
+			}
 		}
 	}
 	BUG();
-- 
2.18.4

-- 
Nitesh