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

[Tejun Heo]

(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?

> 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) {
> 
> 
> On 01/29, Leonardo Bras wrote:
> >
> > When __queue_delayed_work() is called, it chooses a cpu for handling the
> > timer interrupt. As of today, it will pick either the cpu passed as
> > parameter or the last cpu used for this.
> >
> > This is not good if a system does use CPU isolation, because it can take
> > away some valuable cpu time to:
> > 1 - deal with the timer interrupt,
> > 2 - schedule-out the desired task,
> > 3 - queue work on a random workqueue, and
> > 4 - schedule the desired task back to the cpu.
> >
> > So to fix this, during __queue_delayed_work(), if cpu isolation is in
> > place, pick a random non-isolated cpu to handle the timer interrupt.
> >
> > As an optimization, if the current cpu is not isolated, use it instead
> > of looking for another candidate.
> >
> > Signed-off-by: Leonardo Bras <leobras@xxxxxxxxxx>
> > ---
> > Changes since v1:
> > - Make sure the CPU is isolated for any value of "cpu"
> >
> > Changes since RFC:
> > - Do not use the same cpu from the timer for queueing the work.
> > - If the current cpu is not isolated, use it's timer instead of
> >   looking for another candidate.
> >
> >  kernel/workqueue.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 76e60faed8923..8dd7c01b326a4 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -1958,10 +1958,18 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
> >  	dwork->cpu = cpu;
> >  	timer->expires = jiffies + delay;
> >
> > -	if (unlikely(cpu != WORK_CPU_UNBOUND))
> > +	if (housekeeping_enabled(HK_TYPE_TIMER)) {
> > +		/* If the current cpu is a housekeeping cpu, use it. */
> > +		cpu = smp_processor_id();
> > +		if (!housekeeping_test_cpu(cpu, HK_TYPE_TIMER))
> > +			cpu = housekeeping_any_cpu(HK_TYPE_TIMER);
> >  		add_timer_on(timer, cpu);
> > -	else
> > -		add_timer(timer);
> > +	} else {
> > +		if (likely(cpu == WORK_CPU_UNBOUND))
> > +			add_timer(timer);
> > +		else
> > +			add_timer_on(timer, cpu);
> > +	}
> >  }
> >
> >  /**
> > --
> > 2.43.0
> >
> 

-- 
tejun