Messages in this thread | | | Date | Wed, 3 Apr 2024 09:12:22 -1000 | From | Tejun Heo <> | Subject | Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work |
| |
(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@redhat.com> > > --- > > 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
| |