Messages in this thread Patch in this message | | | Date | Tue, 2 Apr 2024 12:58:47 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work |
| |
Hello,
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()
- 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.
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 >
| |