[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240402105847.GA24832@redhat.com>
Date: Tue, 2 Apr 2024 12:58:47 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Leonardo Bras <leobras@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>
Cc: Tejun Heo <tj@...nel.org>, Lai Jiangshan <jiangshanlai@...il.com>,
linux-kernel@...r.kernel.org, Junyao Zhao <junzhao@...hat.com>,
Chris von Recklinghausen <crecklin@...hat.com>
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@...hat.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
>
Powered by blists - more mailing lists