Re: [PATCH v2 3/3] lib/group_cpus.c: honor housekeeping config when grouping CPUs

Hannes Reinecke
2024-07-01T06:43:53Z

On 7/1/24 04:09, Ming Lei wrote:
> On Thu, Jun 27, 2024 at 04:10:53PM +0200, Daniel Wagner wrote:
>> group_cpus_evenly distributes all present CPUs into groups. This ignores
>> the isolcpus configuration and assigns isolated CPUs into the groups.
>>
>> Make group_cpus_evenly aware of isolcpus configuration and use the
>> housekeeping CPU mask as base for distributing the available CPUs into
>> groups.
>>
>> Fixes: 11ea68f553e2 ("genirq, sched/isolation: Isolate from handling managed interrupts")
>> Signed-off-by: Daniel Wagner <dwagner@suse.de>
>> ---
>> lib/group_cpus.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 73 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/group_cpus.c b/lib/group_cpus.c
>> index ee272c4cefcc..19fb7186f9d4 100644
>> --- a/lib/group_cpus.c
>> +++ b/lib/group_cpus.c
>> @@ -8,6 +8,7 @@
>> #include <linux/cpu.h>
>> #include <linux/sort.h>
>> #include <linux/group_cpus.h>
>> +#include <linux/sched/isolation.h>
>>
>> #ifdef CONFIG_SMP
>>
>> @@ -330,7 +331,7 @@ static int __group_cpus_evenly(unsigned int startgrp, unsigned int numgrps,
>> }
>>
>> /**
>> - * group_cpus_evenly - Group all CPUs evenly per NUMA/CPU locality
>> + * group_possible_cpus_evenly - Group all CPUs evenly per NUMA/CPU locality
>> * @numgrps: number of groups
>> *
>> * Return: cpumask array if successful, NULL otherwise. And each element
>> @@ -344,7 +345,7 @@ static int __group_cpus_evenly(unsigned int startgrp, unsigned int numgrps,
>> * We guarantee in the resulted grouping that all CPUs are covered, and
>> * no same CPU is assigned to multiple groups
>> */
>> -struct cpumask *group_cpus_evenly(unsigned int numgrps)
>> +static struct cpumask *group_possible_cpus_evenly(unsigned int numgrps)
>> {
>> unsigned int curgrp = 0, nr_present = 0, nr_others = 0;
>> cpumask_var_t *node_to_cpumask;
>> @@ -423,6 +424,76 @@ struct cpumask *group_cpus_evenly(unsigned int numgrps)
>> }
>> return masks;
>> }
>> +
>> +/**
>> + * group_mask_cpus_evenly - Group all CPUs evenly per NUMA/CPU locality
>> + * @numgrps: number of groups
>> + * @cpu_mask: CPU to consider for the grouping
>> + *
>> + * Return: cpumask array if successful, NULL otherwise. And each element
>> + * includes CPUs assigned to this group.
>> + *
>> + * Try to put close CPUs from viewpoint of CPU and NUMA locality into
>> + * same group. Allocate present CPUs on these groups evenly.
>> + */
>> +static struct cpumask *group_mask_cpus_evenly(unsigned int numgrps,
>> + const struct cpumask *cpu_mask)
>> +{
>> + cpumask_var_t *node_to_cpumask;
>> + cpumask_var_t nmsk;
>> + int ret = -ENOMEM;
>> + struct cpumask *masks = NULL;
>> +
>> + if (!zalloc_cpumask_var(&nmsk, GFP_KERNEL))
>> + return NULL;
>> +
>> + node_to_cpumask = alloc_node_to_cpumask();
>> + if (!node_to_cpumask)
>> + goto fail_nmsk;
>> +
>> + masks = kcalloc(numgrps, sizeof(*masks), GFP_KERNEL);
>> + if (!masks)
>> + goto fail_node_to_cpumask;
>> +
>> + build_node_to_cpumask(node_to_cpumask);
>> +
>> + ret = __group_cpus_evenly(0, numgrps, node_to_cpumask, cpu_mask, nmsk,
>> + masks);
>> +
>> +fail_node_to_cpumask:
>> + free_node_to_cpumask(node_to_cpumask);
>> +
>> +fail_nmsk:
>> + free_cpumask_var(nmsk);
>> + if (ret < 0) {
>> + kfree(masks);
>> + return NULL;
>> + }
>> + return masks;
>> +}
>> +
>> +/**
>> + * group_cpus_evenly - Group all CPUs evenly per NUMA/CPU locality
>> + * @numgrps: number of groups
>> + *
>> + * Return: cpumask array if successful, NULL otherwise.
>> + *
>> + * group_possible_cpus_evently() is used for distributing the cpus on all
>> + * possible cpus in absence of isolcpus command line argument.
>> + * group_mask_cpu_evenly() is used when the isolcpus command line
>> + * argument is used with managed_irq option. In this case only the
>> + * housekeeping CPUs are considered.
>> + */
>> +struct cpumask *group_cpus_evenly(unsigned int numgrps)
>> +{
>> + const struct cpumask *hk_mask;
>> +
>> + hk_mask = housekeeping_cpumask(HK_TYPE_MANAGED_IRQ);
>> + if (!cpumask_empty(hk_mask))
>> + return group_mask_cpus_evenly(numgrps, hk_mask);
>> +
>> + return group_possible_cpus_evenly(numgrps);
>
> Since this patch, some isolated CPUs may not be covered in
> blk-mq queue mapping.
>
> Meantime people still may submit IO workload from isolated CPUs
> such as by 'taskset -c', blk-mq may not work well for this situation,
> for example, IO hang may be caused during cpu hotplug.
>
> I did see this kind of usage in some RH Openshift workloads.
>
> If blk-mq problem can be solved, I am fine with this kind of
> change.
>
That was kinda the idea of this patchset; when 'isolcpus' is active any
in-kernel driver can only run on the housekeeping CPUs, and I/O from the
isolcpus is impossible.
(Otherwise they won't be isolated anymore, and the whole concepts
becomes ever so shaky.).
Consequently we should not spread blk-mq onto the isolcpus (which is
what this patchset attempts). We do need to check how we could inhibit
I/O from the isolcpus, though; not sure if we do that now.
Something we need to check.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich