Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work

[Oleg Nesterov <oleg@redhat.com>]

Hi Tejun,

On 04/03, Tejun Heo wrote:
>
> (cc'ing Frederic and quoting whole body)
>
> Hello, Oleg.
>
> On Tue, Apr 02, 2024 at 12:58:47PM +0200, Oleg Nesterov wrote:
> > This patch was applied as aae17ebb53cd3da but as Chris reports with this
> > commit the kernel can crash at boot time because __queue_delayed_work()
> > doesn't check that housekeeping_any_cpu() returns a valid cpu < nr_cpu_ids.
> >
> > Just boot the kernel with nohz_full=mask which includes the boot cpu, say
> > nohz_full=0-6 on a machine with 8 CPUs. __queue_delayed_work() will use
> > add_timer_on(timer, NR_CPUS /* returned by housekeeping_any_cpu */) until
> > start_secondary() brings CPU 7 up.
> >
> > The problem is simple, but I do not know what should we do, I know nothing
> > about CPU isolation.
> >
> > We can fix __queue_delayed_work(), this is simple, but other callers of
> > housekeeping_any_cpu() seem to assume it must always return a valid CPU
> > too. So perhaps we should change housekeeping_any_cpu()
>
> Yeah, patching this up from wq side is easy but housekeeping_any_cpu()
> always being able to pick a housekeeping CPU would be better.
>
> > -			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
> > +			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
> > +			if (cpu < nr_cpu_ids)
> > +				return cpu;
> >
> > ?
> >
> > But I'm afraid this can hide other problems. May be
> >
> > -			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
> > +			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
> > +			if (cpu < nr_cpu_ids)
> > +				return cpu;
> > +
> > +			WARN_ON(system_state > SYSTEM_BOOTING);
> >
> > ?
> >
> > -------------------------------------------------------------------------------
> > OTOH, Documentation/timers/no_hz.rst says
> >
> > 	Therefore, the
> > 	boot CPU is prohibited from entering adaptive-ticks mode.  Specifying a
> > 	"nohz_full=" mask that includes the boot CPU will result in a boot-time
> > 	error message, and the boot CPU will be removed from the mask.
> >
> > and this doesn't match the reality.
>
> Don't some archs allow the boot CPU to go down too tho? If so, this doesn't
> really solve the problem, right?

I do not know. But I thought about this too.

In the context of this discussion we do not care if the boot CPU goes down.
But we need at least one housekeeping CPU after cpu_down(). The comment in
cpu_down_maps_locked() says

	Also keep at least one housekeeping cpu onlined

but it checks HK_TYPE_DOMAIN, and I do not know (and it is too late for me
to try to read the code ;) if housekeeping.cpumasks[HK_TYPE_TIMER] can get
empty or not.

Oleg.	

> > So it seems that we should fix housekeeping_setup() ? see the patch below.
> >
> > In any case the usage of cpu_present_mask doesn't look right to me.
> >
> > Oleg.
> >
> > --- a/kernel/sched/isolation.c
> > +++ b/kernel/sched/isolation.c
> > @@ -129,7 +154,7 @@ static int __init housekeeping_setup(char *str, unsigned long flags)
> >  	cpumask_andnot(housekeeping_staging,
> >  		       cpu_possible_mask, non_housekeeping_mask);
> >
> > -	if (!cpumask_intersects(cpu_present_mask, housekeeping_staging)) {
> > +	if (!cpumask_test_cpu(smp_processor_id(), housekeeping_staging)) {
> >  		__cpumask_set_cpu(smp_processor_id(), housekeeping_staging);
> >  		__cpumask_clear_cpu(smp_processor_id(), non_housekeeping_mask);
> >  		if (!housekeeping.flags) {
> >