Re: [RFC PATCH] kernel/cpu: in freeze_secondary_cpus() ensure primary cpu is of domain type

[Shashank Balaji <shashank.mahadasyam@sony.com>]

Hi Peter,

On Mon, Jun 30, 2025 at 10:48:08AM +0200, Peter Zijlstra wrote:
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index a59e009e0be4..d9167b0559a5 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -1902,12 +1902,28 @@ int freeze_secondary_cpus(int primary)
> >  
> >  	cpu_maps_update_begin();
> >  	if (primary == -1) {
> > -		primary = cpumask_first(cpu_online_mask);
> > -		if (!housekeeping_cpu(primary, HK_TYPE_TIMER))
> > -			primary = housekeeping_any_cpu(HK_TYPE_TIMER);
> > +		primary = cpumask_first_and_and(cpu_online_mask,
> > +								housekeeping_cpumask(HK_TYPE_TIMER),
> > +								housekeeping_cpumask(HK_TYPE_DOMAIN));
> 
> That's terrible indenting, please align after the opening bracket like:
> 
> 		primary = cpumask_first_and_and(cpu_online_mask,
> 						housekeeping_cpumask(HK_TYPE_TIMER),
> 						housekeeping_cpumask(HK_TYPE_DOMAIN));
> 
> Also, IIRC HK_TYPE_HRTIMER is deprecated and should be something like
> HK_TYPE_NOISE or somesuch. Frederic?
>     <snip>
> > +			primary = cpumask_first_and(cpu_online_mask,
> > +								housekeeping_cpumask(HK_TYPE_DOMAIN));
> 
> Indenting again.

Sorry about the indentation. I've now set my editor to use 8-space tabs.
I'll have it fixed in the next version of the patch.

HK_TYPE_KERNEL_NOISE was added by commit 6010d245ddc9f463bbf0
(sched/isolation: Consolidate housekeeping cpumasks that are always identical).
I'll replace HK_TYPE_TIMER with HK_TYPE_KERNEL_NOISE in the next
version.

> 
> > +			if (primary >= nr_cpu_ids) {
> > +				error = -ENODEV;
> > +				pr_err("No suitable primary CPU found. Ensure at least one non-isolated CPU is online\n");
> > +				goto abort;
> > +			}
> > +		} else if (!housekeeping_cpu(primary, HK_TYPE_DOMAIN)) {
> > +			error = -ENODEV;
> > +			pr_err("Primary CPU %d should not be isolated\n", primary);
> > +			goto abort;
> > +		}
> >  	}
> >  
> >  	/*
> > @@ -1943,6 +1959,7 @@ int freeze_secondary_cpus(int primary)
> >  	else
> >  		pr_err("Non-boot CPUs are not disabled\n");
> >  
> > +abort:
> >  	/*
> >  	 * Make sure the CPUs won't be enabled by someone else. We need to do
> >  	 * this even in case of failure as all freeze_secondary_cpus() users are
> 
> 
> Also; doesn't the above boil down to something like:
> 
> 	if (primary == -1) {
> 		primary = cpumask_first_and_and(cpu_online_mask,
> 						housekeeping_cpumask(HK_TYPE_TIMER),
> 						housekeeping_cpumask(HK_TYPE_DOMAIN));
> 	} if (!cpu_online(primary)) {
> 		primary = cpumask_first_and(cpu_online_mask,
> 					    housekeeping_cpumask(HK_TYPE_DOMAIN));
> 	} 
> 
> 	if (primary >= nr_cpu_ids || !housekeeping_cpu(primary, HK_TYPE_DOMAIN)) {
> 		error = -ENODEV;
> 		pr_err("Primary CPU %d should not be isolated\n", primary);
> 		goto abort;
> 	}
> 
> Yes, this has less error string variation, but the code is simpler.

If primary >= nr_cpu_ids, primary should not be used in the error
string. But I do think it can be simplified as so, at the cost of the
error string not being completely accurate:

	if (primary == -1) {
		primary = cpumask_first_and_and(cpu_online_mask,
						housekeeping_cpumask(HK_TYPE_TIMER),
						housekeeping_cpumask(HK_TYPE_DOMAIN));
	} else if (!cpu_online(primary)) {
		primary = cpumask_first_and(cpu_online_mask,
					    housekeeping_cpumask(HK_TYPE_DOMAIN));
	}
	if (primary >= nr_cpu_ids) {
		error = -ENODEV;
		pr_err("No suitable primary CPU found. Ensure at least one non-isolated CPU is online\n");
		goto abort;
	} else if (!housekeeping_cpu(HK_TYPE_DOMAIN)) {
		error = -ENODEV;
		pr_err("Primary CPU %d should not be isolated\n", primary);
		goto abort;
	}

The "Ensure at lest one non-isolated CPU is online" is only partially
true in the case primary = -1 was passed and we couldn't find a suitable
cpu because we were looking for an online cpu that's non-isolated _and_
non-nohz_full. But the "non-nohz_full" condition isn't taken care of in
the !cpu_online(primary) branch. Although, I do think it should be
(question 2 below): in both the branches, we should look for a cpu that's online,
non-isolated, and of domain type. That would further simplify the above snippet to,

	if (primary == -1 || !cpu_online(primary)) {
		primary = cpumask_first_and_and(cpu_online_mask,
						housekeeping_cpumask(HK_TYPE_TIMER),
						housekeeping_cpumask(HK_TYPE_DOMAIN));
	}
	if (primary >= nr_cpu_ids) {
		error = -ENODEV;
		pr_err("No suitable primary CPU found. Ensure at least one non-isolated, non-nohz_full CPU is online\n");
		goto abort;
	} else if (!housekeeping_cpu(HK_TYPE_DOMAIN) || !housekeeping_cpu(HK_TYPE_TIMER)) {
		error = -ENODEV;
		pr_err("Primary CPU %d should not be isolated or nohz_full\n", primary);
		goto abort;
	}

Now the error string is accurate.

>2. This concerns the following snippet of freeze_secondary_cpus():
>
>        if (primary == -1) {
>            primary = cpumask_first(cpu_online_mask);
>            if (!housekeeping_cpu(primary, HK_TYPE_TIMER))
>                primary = housekeeping_any_cpu(HK_TYPE_TIMER);
>        } else {
>            if (!cpu_online(primary))
>                primary = cpumask_first(cpu_online_mask);
>        }
>
>    suspend_disable_secondary_cpus() calls freeze_secondary_cpus() with primary = -1,
>    if CONFIG_PM_SLEEP_SMP_NONZERO_CPU, and primary = 0 otherwise. On x86 and arm64,
>    for example, it's called with primary = 0. In the primary != -1 branch, why
>    isn't primary checked for HK_TYPE_TIMER as is done in the primary == 1 branch?
>    On x86 this is fine, since it doesn't advertise ARCH_SUSPEND_NONZERO_CPU,
>    cpu 0 will be removed out of the nohz_full mask even if it's specified. And it
>    also cannot be offlined. So on x86, the primary cpu will always be online and
>    non-nohz_full. But on arm64, for example, cpu 0 _can_ be offlined. So
>    cpumask_first(cpu_online_mask) will find the next online cpu, which may not
>    be non-nohz_full.
>
>    Also, why the requirement for the primary cpu to be of HK_TYPE_TIMER?

The HK_TYPE_TIMER check was added by Nicholas in
commit 9ca12ac04bb7d7cfb28aa5 (kernel/cpu: Allow non-zero CPU to be primary
for suspend / kexec freeze) [1], but it was added only to the primary = -1 branch.
I'm not sure why we don't check for HK_TYPE_TIMER when primary is specified.

[1] https://lore.kernel.org/all/20190411033448.20842-4-npiggin@gmail.com/

Thanks,
Shashank