RE: [PATCH v2] Drivers: hv: vmbus: Don't assign VMbus channel interrupts to isolated CPUs

[Michael Kelley (LINUX)]

From: Stephen Hemminger <sthemmin@xxxxxxxxxxxxx> Sent: Friday, May 27, 2022 8:41 AM
> 
> Would this have impact for DPDK applications using isolated cpus?

I don't have any existing knowledge of DPDK use of isolated CPUs,
so someone with more expertise feel free to correct me.

From what I see in the DPDK documentation (Section 8.3 here:
https://doc.dpdk.org/guides/linux_gsg/enable_func.html), there's
no impact.  The example in that documentation does CPU isolation
only for the purpose of scheduling, not for interrupts.  The
example kernel command line is:

isolcpus=2,4,6

which defaults to "domain" as the "flag" and is equivalent to:

isolcpus=domain,2,4,6.

VMbus channel interrupts are affected only if "managed_irq" is
specified as the flag per the commit message below.

And FWIW, cpusets provide a better way to doing scheduler
isolation than the isolcpus kernel boot option.  Perhaps the
DPDK documentation should be updated. :-)

Michael

> 
> -----Original Message-----
> From: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
> Sent: Friday, May 27, 2022 12:22 AM
> To: KY Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>;
> Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>; wei.liu@xxxxxxxxxx; Dexuan Cui
> <decui@xxxxxxxxxxxxx>; linux-hyperv@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxx>; Michael Kelley (LINUX)
> <mikelley@xxxxxxxxxxxxx>
> Subject: [PATCH v2] Drivers: hv: vmbus: Don't assign VMbus channel interrupts to
> isolated CPUs
> 
> When initially assigning a VMbus channel interrupt to a CPU, don’t choose
> a managed IRQ isolated CPU (as specified on the kernel boot line with
> parameter 'isolcpus=managed_irq,<#cpu>'). Also, when using sysfs to change
> the CPU that a VMbus channel will interrupt, don't allow changing to a
> managed IRQ isolated CPU.
> 
> Signed-off-by: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
> ---
> v2: * better commit message
>     * Added back empty line, removed by mistake
>     * Removed error print for sysfs error
> 
>  drivers/hv/channel_mgmt.c | 18 ++++++++++++------
>  drivers/hv/vmbus_drv.c    |  4 ++++
>  2 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 97d8f56..e1fe029 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -21,6 +21,7 @@
>  #include <linux/cpu.h>
>  #include <linux/hyperv.h>
>  #include <asm/mshyperv.h>
> +#include <linux/sched/isolation.h>
> 
>  #include "hyperv_vmbus.h"
> 
> @@ -728,16 +729,20 @@ static void init_vp_index(struct vmbus_channel *channel)
>  	u32 i, ncpu = num_online_cpus();
>  	cpumask_var_t available_mask;
>  	struct cpumask *allocated_mask;
> +	const struct cpumask *hk_mask =
> housekeeping_cpumask(HK_TYPE_MANAGED_IRQ);
>  	u32 target_cpu;
>  	int numa_node;
> 
>  	if (!perf_chn ||
> -	    !alloc_cpumask_var(&available_mask, GFP_KERNEL)) {
> +	    !alloc_cpumask_var(&available_mask, GFP_KERNEL) ||
> +	    cpumask_empty(hk_mask)) {
>  		/*
>  		 * If the channel is not a performance critical
>  		 * channel, bind it to VMBUS_CONNECT_CPU.
>  		 * In case alloc_cpumask_var() fails, bind it to
>  		 * VMBUS_CONNECT_CPU.
> +		 * If all the cpus are isolated, bind it to
> +		 * VMBUS_CONNECT_CPU.
>  		 */
>  		channel->target_cpu = VMBUS_CONNECT_CPU;
>  		if (perf_chn)
> @@ -758,17 +763,19 @@ static void init_vp_index(struct vmbus_channel *channel)
>  		}
>  		allocated_mask = &hv_context.hv_numa_map[numa_node];
> 
> -		if (cpumask_equal(allocated_mask, cpumask_of_node(numa_node))) {
> +retry:
> +		cpumask_xor(available_mask, allocated_mask,
> cpumask_of_node(numa_node));
> +		cpumask_and(available_mask, available_mask, hk_mask);
> +
> +		if (cpumask_empty(available_mask)) {
>  			/*
>  			 * We have cycled through all the CPUs in the node;
>  			 * reset the allocated map.
>  			 */
>  			cpumask_clear(allocated_mask);
> +			goto retry;
>  		}
> 
> -		cpumask_xor(available_mask, allocated_mask,
> -			    cpumask_of_node(numa_node));
> -
>  		target_cpu = cpumask_first(available_mask);
>  		cpumask_set_cpu(target_cpu, allocated_mask);
> 
> @@ -778,7 +785,6 @@ static void init_vp_index(struct vmbus_channel *channel)
>  	}
> 
>  	channel->target_cpu = target_cpu;
> -
>  	free_cpumask_var(available_mask);
>  }
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 714d549..547ae33 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -21,6 +21,7 @@
>  #include <linux/kernel_stat.h>
>  #include <linux/clockchips.h>
>  #include <linux/cpu.h>
> +#include <linux/sched/isolation.h>
>  #include <linux/sched/task_stack.h>
> 
>  #include <linux/delay.h>
> @@ -1770,6 +1771,9 @@ static ssize_t target_cpu_store(struct vmbus_channel
> *channel,
>  	if (target_cpu >= nr_cpumask_bits)
>  		return -EINVAL;
> 
> +	if (!cpumask_test_cpu(target_cpu,
> housekeeping_cpumask(HK_TYPE_MANAGED_IRQ)))
> +		return -EINVAL;
> +
>  	/* No CPUs should come up or down during this. */
>  	cpus_read_lock();
> 
> --
> 1.8.3.1