Received-SPF: pass (google.com: domain of linux-kernel+bounces-127811-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 2 Apr 2024 12:58:47 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: Leonardo Bras <leobras@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>
Cc: Tejun Heo <tj@kernel.org>, Lai Jiangshan <jiangshanlai@gmail.com>,
	linux-kernel@vger.kernel.org, Junyao Zhao <junzhao@redhat.com>,
	Chris von Recklinghausen <crecklin@redhat.com>
Subject: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on
 queue_delayed_work
Message-ID: <20240402105847.GA24832@redhat.com>
References: <20240130010046.2730139-2-leobras@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240130010046.2730139-2-leobras@redhat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)

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
>