wq: Avoid using isolated cpus' timers on queue_delayed_work

PATCH Applied Leonardo Bras 8 participants 40 messages Jan 30, 2024 – Oct 5, 2024

Subsystems (4)

Documentation Scheduler High-resolution timers, timer wheel, clockevents Workqueue

lore

Share & tools

lore

Leonardo Bras

author

2024-01-30 01:01 UTC #0 lore raw

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

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 <[email protected]>

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

kernel/workqueue.c +11 -3

index 76e60faed8923..8dd7c01b326a4 100644
@@ -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);
+	}
 }
 
 /**

Signature · 1 line

2.43.0

Tejun Heo

maintainer Applied

2024-01-30 01:22 UTC #1 lore raw

Quoted 11 lines · Leonardo Bras

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.

Quoted 7 lines

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 <[email protected]>

Applied to wq/for-6.9.

Thanks.

Signature · 1 line

tejun

Leonardo Bras

author

2024-01-30 02:58 UTC #2 lore raw

Quoted 15 lines · Tejun Heo

Quoted 14 lines · Leonardo Bras
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.

Quoted 12 lines

Quoted 4 lines
As an optimization, if the current cpu is not isolated, use it instead of looking for another candidate.
Signed-off-by Leonardo Bras <[email protected]>
Applied to wq/for-6.9.
Thanks.
Signature · 1 line
tejun

Thank you!
Leo

Oleg Nesterov

2024-04-02 11:00 UTC #3 lore raw

In reply to Leonardo Bras

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.

kernel/sched/isolation.c +1 -1

@@ -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) {

Quoted 60 lines · Leonardo Bras

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 <[email protected]>
---
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(-)
kernel/workqueue.c +11 -4
index 76e60faed8923..8dd7c01b326a4 100644
@@ -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 Heo

maintainer

2024-04-03 19:12 UTC #4 lore raw

(cc'ing Frederic and quoting whole body)

Hello, Oleg.

Quoted 9 lines · Oleg Nesterov

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.

Quoted 6 lines

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.

Quoted 21 lines

(quoted diff) +3 -1
-			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
(quoted diff) +5 -1
-			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

Quoted 6 lines

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?

Quoted 81 lines

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.
kernel/sched/isolation.c +1 -1
@@ -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) {
Quoted 60 lines · Leonardo Bras
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 <[email protected]>
---
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(-)
kernel/workqueue.c +11 -4
index 76e60faed8923..8dd7c01b326a4 100644
@@ -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

Signature · 1 line

tejun

Oleg Nesterov

2024-04-03 20:39 UTC #5 lore raw

Hi Tejun,

Quoted 51 lines · Tejun Heo

(cc'ing Frederic and quoting whole body)
Hello, Oleg.
Quoted 9 lines · Oleg Nesterov
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.
Quoted 6 lines
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.
Quoted 26 lines
(quoted diff) +3 -1
-			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
(quoted diff) +5 -1
-			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.

Quoted 4 lines

Quoted 1 line
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?

I do not know. But I thought about this too.

In the context of this discussion we do not care if the boot CPU goes down. But we need at least one housekeeping CPU after cpu_down(). The comment in cpu_down_maps_locked() says

Also keep at least one housekeeping cpu onlined

but it checks HK_TYPE_DOMAIN, and I do not know (and it is too late for me to try to read the code ;) if housekeeping.cpumasks[HK_TYPE_TIMER] can get empty or not.

Oleg.

Quoted 18 lines

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.

kernel/sched/isolation.c +1 -1

@@ -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) {

Oleg Nesterov

2024-04-05 14:06 UTC #6 lore raw

Quoted 19 lines · Oleg Nesterov

Quoted 8 lines
Quoted 8 lines
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.
Quoted 4 lines
Quoted 1 line
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?
I do not know. But I thought about this too.
In the context of this discussion we do not care if the boot CPU goes down. But we need at least one housekeeping CPU after cpu_down(). The comment in cpu_down_maps_locked() says

Quoted 5 lines

Also keep at least one housekeeping cpu onlined
but it checks HK_TYPE_DOMAIN, and I do not know (and it is too late for me to try to read the code ;) if housekeeping.cpumasks[HK_TYPE_TIMER] can get empty or not.

This nearly killed me, but I managed to convince myself we shouldn't worry about cpu_down().

HK_FLAG_TIMER implies HK_FLAG_TICK.

HK_FLAG_TICK implies tick_nohz_full_setup() which sets tick_nohz_full_mask = non_housekeeping_mask.

When tick_setup_device() is called on a housekeeping CPU it does

	else if (tick_do_timer_boot_cpu != -1 &&
					!tick_nohz_full_cpu(cpu)) {
		tick_take_do_timer_from_boot();
		tick_do_timer_boot_cpu = -1;


	and this sets tick_do_timer_cpu = first-housekeeping-cpu.

cpu_down(tick_do_timer_cpu) will fail, tick_nohz_cpu_down() will nack it.

So cpu_down() can't make housekeeping.cpumasks[HK_FLAG_TIMER] empty and I still think that the change below is the right approach.

But probably WARN_ON() in housekeeping_any_cpu() makes sense anyway.

What do you think?

Oleg.

Quoted 18 lines

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.

kernel/sched/isolation.c +1 -1

@@ -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) {

Tejun Heo

maintainer

2024-04-05 15:38 UTC #7 lore raw

Hello, Oleg.

On Fri, Apr 05, 2024 at 04:04:49PM +0200, Oleg Nesterov wrote:
...

Quoted 9 lines

Quoted 9 lines
Quoted 2 lines
Don't some archs allow the boot CPU to go down too tho? If so, this doesn't really solve the problem, right?
I do not know. But I thought about this too.
In the context of this discussion we do not care if the boot CPU goes down. But we need at least one housekeeping CPU after cpu_down(). The comment in cpu_down_maps_locked() says

Quoted 7 lines

Quoted 5 lines
Also keep at least one housekeeping cpu onlined
but it checks HK_TYPE_DOMAIN, and I do not know (and it is too late for me to try to read the code ;) if housekeeping.cpumasks[HK_TYPE_TIMER] can get empty or not.
This nearly killed me, but I managed to convince myself we shouldn't worry

Oh no, don't die. :)

Quoted 17 lines

about cpu_down().
HK_FLAG_TIMER implies HK_FLAG_TICK.
HK_FLAG_TICK implies tick_nohz_full_setup() which sets tick_nohz_full_mask = non_housekeeping_mask.
When tick_setup_device() is called on a housekeeping CPU it does
	else if (tick_do_timer_boot_cpu != -1 &&
					!tick_nohz_full_cpu(cpu)) {
		tick_take_do_timer_from_boot();
		tick_do_timer_boot_cpu = -1;


	and this sets tick_do_timer_cpu = first-housekeeping-cpu.

Quoted 6 lines

cpu_down(tick_do_timer_cpu) will fail, tick_nohz_cpu_down() will nack it.
So cpu_down() can't make housekeeping.cpumasks[HK_FLAG_TIMER] empty and I still think that the change below is the right approach.
But probably WARN_ON() in housekeeping_any_cpu() makes sense anyway.

This would be great.

Quoted 16 lines

What do you think?
Oleg.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
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.
kernel/sched/isolation.c +0 -0
@@ -129,7 +154,7 @@ static int __init housekeeping_setup(char *str, unsigned long flags)
 	cpumask_andnot(housekeeping_staging,
 		       cpu_possible_mask, non_housekeeping_mask);
 

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
kernel/sched/isolation.c static int __init housekeeping_setup(char *str, unsigned long flags) · line 160
Quoted 5 lines
kernel/sched/isolation.c +1 -1
-	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) {

Ensuring the boot CPU always be a housekeeping CPU makes sense to me but I'm not very familiar with the housekeeping code. Frederic, what do you think?

Thanks.

Signature · 1 line

tejun

Frederic Weisbecker

maintainer

2024-04-05 22:03 UTC #8 lore raw

Quoted 12 lines · Tejun Heo

Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
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.
kernel/sched/isolation.c +0 -0
@@ -129,7 +154,7 @@ static int __init housekeeping_setup(char *str, unsigned long flags)
 	cpumask_andnot(housekeeping_staging,
 		       cpu_possible_mask, non_housekeeping_mask);
 

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
kernel/sched/isolation.c static int __init housekeeping_setup(char *str, unsigned long flags) · line 160
Quoted 5 lines
kernel/sched/isolation.c +1 -1
-	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) {
Ensuring the boot CPU always be a housekeeping CPU makes sense to me but I'm not very familiar with the housekeeping code. Frederic, what do you think?

I vote for reverting:

08ae95f4fd3b (nohz_full: Allow the boot CPU to be nohz_full)

And if that's not possible because there are actual users of it, then the best we can do is to make housekeeping_any_cpu() return smp_processor_id() if the housekeeping_mask() is empty. Or rather if the housekeeping_mask is empty, return cpumask_of(smp_processor_id()).

Of course that's merely just a bandaid.

Thanks.

Frederic Weisbecker

maintainer

2024-04-05 21:52 UTC #9 lore raw

Nohz_full on boot CPU is broken (was: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work)

In reply to Oleg Nesterov

+Cc Nick

Quoted 44 lines · Oleg Nesterov

Quoted 19 lines · Oleg Nesterov
Quoted 8 lines
Quoted 8 lines
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.
Quoted 4 lines
Quoted 1 line
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?
I do not know. But I thought about this too.
In the context of this discussion we do not care if the boot CPU goes down. But we need at least one housekeeping CPU after cpu_down(). The comment in cpu_down_maps_locked() says
Quoted 5 lines
Also keep at least one housekeeping cpu onlined
but it checks HK_TYPE_DOMAIN, and I do not know (and it is too late for me to try to read the code ;) if housekeeping.cpumasks[HK_TYPE_TIMER] can get empty or not.
This nearly killed me, but I managed to convince myself we shouldn't worry about cpu_down().
HK_FLAG_TIMER implies HK_FLAG_TICK.
HK_FLAG_TICK implies tick_nohz_full_setup() which sets tick_nohz_full_mask = non_housekeeping_mask.
When tick_setup_device() is called on a housekeeping CPU it does
	else if (tick_do_timer_boot_cpu != -1 &&
					!tick_nohz_full_cpu(cpu)) {
		tick_take_do_timer_from_boot();
		tick_do_timer_boot_cpu = -1;


	and this sets tick_do_timer_cpu = first-housekeeping-cpu.

Quoted 8 lines

cpu_down(tick_do_timer_cpu) will fail, tick_nohz_cpu_down() will nack it.
So cpu_down() can't make housekeeping.cpumasks[HK_FLAG_TIMER] empty and I still think that the change below is the right approach.
But probably WARN_ON() in housekeeping_any_cpu() makes sense anyway.
What do you think?

Good analysis on this nasty housekeeping VS tick code. I promised so many times to cleanup this mess but things keep piling up.

It is indeed possible for the boot CPU to be a nohz_full CPU and as you can see, it's only half-working. This is so ever since:

08ae95f4fd3b (nohz_full: Allow the boot CPU to be nohz_full)

I wish I had nacked it before it got merged, especially as the changelog mentions that the user could have solved this with modifying its setup... I would love to revert that now but I don't know if anyone uses this and have it working by chance somewhere... Should we continue to support a broken feature? Can we break user ABI if it's already half-broken?

Anyway so during boot it's possible to have an empty housekeeping_mask(HK_TYPE_TIMER) & cpu_online_mask. After boot though (provided any CPU from the housekeeping_mask(HK_TYPE_TIMER) has actually booted, which isn't even guaranteed if maxcpus= is passed...) the first online housekeeping can't go down like you spotted.

Thanks.

Oleg Nesterov

2024-04-07 13:10 UTC #10 lore raw

Re: Nohz_full on boot CPU is broken (was: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work)

Quoted 57 lines · Frederic Weisbecker

+Cc Nick
Quoted 44 lines · Oleg Nesterov
Quoted 19 lines · Oleg Nesterov
Quoted 8 lines
Quoted 8 lines
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.
Quoted 4 lines
Quoted 1 line
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?
I do not know. But I thought about this too.
In the context of this discussion we do not care if the boot CPU goes down. But we need at least one housekeeping CPU after cpu_down(). The comment in cpu_down_maps_locked() says
Quoted 5 lines
Also keep at least one housekeeping cpu onlined
but it checks HK_TYPE_DOMAIN, and I do not know (and it is too late for me to try to read the code ;) if housekeeping.cpumasks[HK_TYPE_TIMER] can get empty or not.
This nearly killed me, but I managed to convince myself we shouldn't worry about cpu_down().
HK_FLAG_TIMER implies HK_FLAG_TICK.
HK_FLAG_TICK implies tick_nohz_full_setup() which sets tick_nohz_full_mask = non_housekeeping_mask.
When tick_setup_device() is called on a housekeeping CPU it does
	else if (tick_do_timer_boot_cpu != -1 &&
					!tick_nohz_full_cpu(cpu)) {
		tick_take_do_timer_from_boot();
		tick_do_timer_boot_cpu = -1;


	and this sets tick_do_timer_cpu = first-housekeeping-cpu.
Quoted 8 lines
cpu_down(tick_do_timer_cpu) will fail, tick_nohz_cpu_down() will nack it.
So cpu_down() can't make housekeeping.cpumasks[HK_FLAG_TIMER] empty and I still think that the change below is the right approach.
But probably WARN_ON() in housekeeping_any_cpu() makes sense anyway.
What do you think?

Quoted 7 lines

Good analysis on this nasty housekeeping VS tick code. I promised so many times to cleanup this mess but things keep piling up.
It is indeed possible for the boot CPU to be a nohz_full CPU and as you can see, it's only half-working. This is so ever since:
08ae95f4fd3b (nohz_full: Allow the boot CPU to be nohz_full)

Thanks... So this is intentional. I was confused by

from Documentation/timers/no_hz.rst

Quoted 4 lines

I would love
to revert that now but I don't know if anyone uses this and have it working by chance somewhere... Should we continue to support a broken feature? Can we break user ABI if it's already half-broken?

Well, the changelog says

nohz_full has been trialed at a large supercomputer site and found to significantly reduce jitter. In order to deploy it in production, they need CPU0 to be nohz_full

so I guess this feature has users.

But after the commit aae17ebb53cd3da ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time if the boot CPU is nohz_full.

So we need a workaround at least. I am starting to think I will send a trivial patch which changes __queue_delayed_work() to validate the cpu returned by housekeeping_any_cpu(HK_TYPE_TIMER).

But perhaps something like below makes more sense as a (stupid) workaround?

Oleg.

kernel/sched/isolation.c +9 -1

@@ -46,7 +46,15 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return 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;
+
+			cpu = READ_ONCE(tick_do_timer_boot_cpu);
+			if (cpu >= 0)
+				return cpu;
+
+			WARN_ON_ONCE(1);
 		}
 	}
 	return smp_processor_id();

Oleg Nesterov

2024-04-07 13:54 UTC #11 lore raw

Re: Nohz_full on boot CPU is broken (was: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work)

Quoted 93 lines · Oleg Nesterov

Quoted 57 lines · Frederic Weisbecker
+Cc Nick
Quoted 44 lines · Oleg Nesterov
Quoted 19 lines · Oleg Nesterov
Quoted 8 lines
Quoted 8 lines
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.
Quoted 4 lines
Quoted 1 line
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?
I do not know. But I thought about this too.
In the context of this discussion we do not care if the boot CPU goes down. But we need at least one housekeeping CPU after cpu_down(). The comment in cpu_down_maps_locked() says
Quoted 5 lines
Also keep at least one housekeeping cpu onlined
but it checks HK_TYPE_DOMAIN, and I do not know (and it is too late for me to try to read the code ;) if housekeeping.cpumasks[HK_TYPE_TIMER] can get empty or not.
This nearly killed me, but I managed to convince myself we shouldn't worry about cpu_down().
HK_FLAG_TIMER implies HK_FLAG_TICK.
HK_FLAG_TICK implies tick_nohz_full_setup() which sets tick_nohz_full_mask = non_housekeeping_mask.
When tick_setup_device() is called on a housekeeping CPU it does
	else if (tick_do_timer_boot_cpu != -1 &&
					!tick_nohz_full_cpu(cpu)) {
		tick_take_do_timer_from_boot();
		tick_do_timer_boot_cpu = -1;


	and this sets tick_do_timer_cpu = first-housekeeping-cpu.
Quoted 8 lines
cpu_down(tick_do_timer_cpu) will fail, tick_nohz_cpu_down() will nack it.
So cpu_down() can't make housekeeping.cpumasks[HK_FLAG_TIMER] empty and I still think that the change below is the right approach.
But probably WARN_ON() in housekeeping_any_cpu() makes sense anyway.
What do you think?
Quoted 7 lines
Good analysis on this nasty housekeeping VS tick code. I promised so many times to cleanup this mess but things keep piling up.
It is indeed possible for the boot CPU to be a nohz_full CPU and as you can see, it's only half-working. This is so ever since:
08ae95f4fd3b (nohz_full: Allow the boot CPU to be nohz_full)
Thanks... So this is intentional. I was confused by
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.
from Documentation/timers/no_hz.rst
Quoted 4 lines
I would love
to revert that now but I don't know if anyone uses this and have it working by chance somewhere... Should we continue to support a broken feature? Can we break user ABI if it's already half-broken?
Well, the changelog says
nohz_full has been trialed at a large supercomputer site and found to significantly reduce jitter. In order to deploy it in production, they need CPU0 to be nohz_full
so I guess this feature has users.
But after the commit aae17ebb53cd3da ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time if the boot CPU is nohz_full.

Quoted 5 lines

So we need a workaround at least. I am starting to think I will send a trivial patch which changes __queue_delayed_work() to validate the cpu returned by housekeeping_any_cpu(HK_TYPE_TIMER).
But perhaps something like below makes more sense as a (stupid) workaround?

Or simply

kernel/sched/isolation.c +5 -1

index 373d42c707bc..e912555c6fc8 100644
@@ -46,7 +46,11 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return 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;
+
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING);
 		}
 	}
 	return smp_processor_id();

Frederic Weisbecker

maintainer

2024-04-09 12:05 UTC #12 lore raw

Re: Nohz_full on boot CPU is broken (was: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work)

Quoted 110 lines · Oleg Nesterov

Quoted 93 lines · Oleg Nesterov
Quoted 57 lines · Frederic Weisbecker
+Cc Nick
Quoted 44 lines · Oleg Nesterov
Quoted 19 lines · Oleg Nesterov
Quoted 8 lines
Quoted 8 lines
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.
Quoted 4 lines
Quoted 1 line
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?
I do not know. But I thought about this too.
In the context of this discussion we do not care if the boot CPU goes down. But we need at least one housekeeping CPU after cpu_down(). The comment in cpu_down_maps_locked() says
Quoted 5 lines
Also keep at least one housekeeping cpu onlined
but it checks HK_TYPE_DOMAIN, and I do not know (and it is too late for me to try to read the code ;) if housekeeping.cpumasks[HK_TYPE_TIMER] can get empty or not.
This nearly killed me, but I managed to convince myself we shouldn't worry about cpu_down().
HK_FLAG_TIMER implies HK_FLAG_TICK.
HK_FLAG_TICK implies tick_nohz_full_setup() which sets tick_nohz_full_mask = non_housekeeping_mask.
When tick_setup_device() is called on a housekeeping CPU it does
	else if (tick_do_timer_boot_cpu != -1 &&
					!tick_nohz_full_cpu(cpu)) {
		tick_take_do_timer_from_boot();
		tick_do_timer_boot_cpu = -1;


	and this sets tick_do_timer_cpu = first-housekeeping-cpu.
Quoted 8 lines
cpu_down(tick_do_timer_cpu) will fail, tick_nohz_cpu_down() will nack it.
So cpu_down() can't make housekeeping.cpumasks[HK_FLAG_TIMER] empty and I still think that the change below is the right approach.
But probably WARN_ON() in housekeeping_any_cpu() makes sense anyway.
What do you think?
Quoted 7 lines
Good analysis on this nasty housekeeping VS tick code. I promised so many times to cleanup this mess but things keep piling up.
It is indeed possible for the boot CPU to be a nohz_full CPU and as you can see, it's only half-working. This is so ever since:
08ae95f4fd3b (nohz_full: Allow the boot CPU to be nohz_full)
Thanks... So this is intentional. I was confused by
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.
from Documentation/timers/no_hz.rst
Quoted 4 lines
I would love
to revert that now but I don't know if anyone uses this and have it working by chance somewhere... Should we continue to support a broken feature? Can we break user ABI if it's already half-broken?
Well, the changelog says
nohz_full has been trialed at a large supercomputer site and found to significantly reduce jitter. In order to deploy it in production, they need CPU0 to be nohz_full
so I guess this feature has users.
But after the commit aae17ebb53cd3da ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time if the boot CPU is nohz_full.
Quoted 5 lines
So we need a workaround at least. I am starting to think I will send a trivial patch which changes __queue_delayed_work() to validate the cpu returned by housekeeping_any_cpu(HK_TYPE_TIMER).
But perhaps something like below makes more sense as a (stupid) workaround?
Or simply
kernel/sched/isolation.c +0 -0
index 373d42c707bc..e912555c6fc8 100644
@@ -46,7 +46,11 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return cpu;

kernel/sched/isolation.c int housekeeping_any_cpu(enum hk_type type) · line 56

Quoted 10 lines

kernel/sched/isolation.c +5 -1

 
-			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_ONCE(system_state == SYSTEM_RUNNING);
 		}
 	}
 	return smp_processor_id();

Yep, looks good!

Thanks.

Frederic Weisbecker

maintainer

2024-04-09 12:04 UTC #13 lore raw

Re: Nohz_full on boot CPU is broken (was: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work)

In reply to Oleg Nesterov

Quoted 86 lines · Oleg Nesterov

Quoted 57 lines · Frederic Weisbecker
+Cc Nick
Quoted 44 lines · Oleg Nesterov
Quoted 19 lines · Oleg Nesterov
Quoted 8 lines
Quoted 8 lines
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.
Quoted 4 lines
Quoted 1 line
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?
I do not know. But I thought about this too.
In the context of this discussion we do not care if the boot CPU goes down. But we need at least one housekeeping CPU after cpu_down(). The comment in cpu_down_maps_locked() says
Quoted 5 lines
Also keep at least one housekeeping cpu onlined
but it checks HK_TYPE_DOMAIN, and I do not know (and it is too late for me to try to read the code ;) if housekeeping.cpumasks[HK_TYPE_TIMER] can get empty or not.
This nearly killed me, but I managed to convince myself we shouldn't worry about cpu_down().
HK_FLAG_TIMER implies HK_FLAG_TICK.
HK_FLAG_TICK implies tick_nohz_full_setup() which sets tick_nohz_full_mask = non_housekeeping_mask.
When tick_setup_device() is called on a housekeeping CPU it does
	else if (tick_do_timer_boot_cpu != -1 &&
					!tick_nohz_full_cpu(cpu)) {
		tick_take_do_timer_from_boot();
		tick_do_timer_boot_cpu = -1;


	and this sets tick_do_timer_cpu = first-housekeeping-cpu.
Quoted 8 lines
cpu_down(tick_do_timer_cpu) will fail, tick_nohz_cpu_down() will nack it.
So cpu_down() can't make housekeeping.cpumasks[HK_FLAG_TIMER] empty and I still think that the change below is the right approach.
But probably WARN_ON() in housekeeping_any_cpu() makes sense anyway.
What do you think?
Quoted 7 lines
Good analysis on this nasty housekeeping VS tick code. I promised so many times to cleanup this mess but things keep piling up.
It is indeed possible for the boot CPU to be a nohz_full CPU and as you can see, it's only half-working. This is so ever since:
08ae95f4fd3b (nohz_full: Allow the boot CPU to be nohz_full)
Thanks... So this is intentional. I was confused by
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.
from Documentation/timers/no_hz.rst
Quoted 4 lines
I would love
to revert that now but I don't know if anyone uses this and have it working by chance somewhere... Should we continue to support a broken feature? Can we break user ABI if it's already half-broken?
Well, the changelog says
nohz_full has been trialed at a large supercomputer site and found to significantly reduce jitter. In order to deploy it in production, they need CPU0 to be nohz_full

Quoted 5 lines

so I guess this feature has users.
But after the commit aae17ebb53cd3da ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time if the boot CPU is nohz_full.

Right but there are many possible calls to housekeeping on boot before the first housekeeper becomes online.

Thanks.

Oleg Nesterov

2024-04-09 13:09 UTC #14 lore raw

Re: Nohz_full on boot CPU is broken (was: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work)

Quoted 8 lines · Frederic Weisbecker

Quoted 6 lines · Oleg Nesterov
Well, the changelog says
nohz_full has been trialed at a large supercomputer site and found to significantly reduce jitter. In order to deploy it in production, they need CPU0 to be nohz_full

Quoted 8 lines

Quoted 5 lines
so I guess this feature has users.
But after the commit aae17ebb53cd3da ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time if the boot CPU is nohz_full.
Right but there are many possible calls to housekeeping on boot before the first housekeeper becomes online.

Well, it seems that other callers are more or less fine in this respect... At least the kernel boots fine with that commit reverted.

But a) I didn't try to really check, and b) this doesn't matter.

I agree, and that is why I never blamed this change in queue_delayed_work().

OK, you seem to agree with the patch below, I'll write the changelog/comment and send it "officially".

Or did I misunderstand you?

Oleg.

kernel/sched/isolation.c +5 -1

index 373d42c707bc..e912555c6fc8 100644
@@ -46,7 +46,11 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return 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;
+
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING);
 		}
 	}
 	return smp_processor_id();

Frederic Weisbecker

maintainer

2024-04-09 14:00 UTC #15 lore raw

Re: Nohz_full on boot CPU is broken (was: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work)

Quoted 21 lines · Oleg Nesterov

Quoted 8 lines · Frederic Weisbecker
Quoted 6 lines · Oleg Nesterov
Well, the changelog says
nohz_full has been trialed at a large supercomputer site and found to significantly reduce jitter. In order to deploy it in production, they need CPU0 to be nohz_full
Quoted 8 lines
Quoted 5 lines
so I guess this feature has users.
But after the commit aae17ebb53cd3da ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time if the boot CPU is nohz_full.
Right but there are many possible calls to housekeeping on boot before the first housekeeper becomes online.
Well, it seems that other callers are more or less fine in this respect... At least the kernel boots fine with that commit reverted.

Quoted 8 lines

But a) I didn't try to really check, and b) this doesn't matter.
I agree, and that is why I never blamed this change in queue_delayed_work().
OK, you seem to agree with the patch below, I'll write the changelog/comment and send it "officially".
Or did I misunderstand you?

Works for me!

Thanks.

Quoted 22 lines

Oleg.

kernel/sched/isolation.c +5 -1

index 373d42c707bc..e912555c6fc8 100644
@@ -46,7 +46,11 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return 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;
+
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING);
 		}
 	}
 	return smp_processor_id();

Nicholas Piggin

2024-04-10 04:27 UTC #16 lore raw

Re: Nohz_full on boot CPU is broken (was: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work)

In reply to Oleg Nesterov

Quoted 10 lines · Oleg Nesterov

Quoted 9 lines · Frederic Weisbecker
Quoted 7 lines · Oleg Nesterov
Well, the changelog says
nohz_full has been trialed at a large supercomputer site and found to significantly reduce jitter. In order to deploy it in production, they need CPU0 to be nohz_full
so I guess this feature has users.

It was the Summit/Sierra supercomputers which I suppose are still using it. IIRC it was an existing job scheduler system they had which ran housekeeping work on the highest numbered core in a socket and allocated jobs from lowest number. We certainly asked if they could change that, but apparently that was difficult. I'm surprised nobody ran into it on x86 though. Maybe the system had more jitter (SMT4 doesn't help), so maybe it wasn't needed to use isolcpus= in other cases.

The other thing is powerpc can boot on arbitrary CPU number. So if boot CPU must be in the mask then it could randomly break your boot config if boot CPU must be in HK mask.

Quoted 11 lines

Quoted 7 lines
Quoted 4 lines
But after the commit aae17ebb53cd3da ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time if the boot CPU is nohz_full.
Right but there are many possible calls to housekeeping on boot before the first housekeeper becomes online.
Well, it seems that other callers are more or less fine in this respect... At least the kernel boots fine with that commit reverted.

Quoted 8 lines

But a) I didn't try to really check, and b) this doesn't matter.
I agree, and that is why I never blamed this change in queue_delayed_work().
OK, you seem to agree with the patch below, I'll write the changelog/comment and send it "officially".
Or did I misunderstand you?

Thanks for this. Taking a while to page this back in, the intention is for housekeeping to be done by boot CPU until house keeper is awake, so returning smp_processor_id() seems like the right thing to do here for ephemeral jobs like timers and work, provided that CPU / mask is not stored somewhere long term by the caller.

For things that set an affinity like kthread, sched, maybe managed irqs, and such.

There are not many callers of housekeeping_any_cpu() so that's easy enough to verify. But similar like housekeeping_cpumask() and others could be an issue or at least a foot-gun, I'm not sure how well I convinced myself of those.

Could you test like this?

  WARN_ON_ONCE(system_state == SYSTEM_RUNNING ||
               type != HK_TYPE_TIMER);

With a comment to say other ephemeral mask types could be exempted if needed.

It would also be nice to warn for cases that would be bugs if the boot CPU was not in the HK mask. Could that be done by having a housekeepers_online() call after smp_init() (maybe at the start of sched_init_smp()) that could verify there is at least one online, and set a flag that could be used to create warnings.

Thanks,
Nick

Quoted 21 lines

Oleg.

kernel/sched/isolation.c +5 -1

index 373d42c707bc..e912555c6fc8 100644
@@ -46,7 +46,11 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return 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;
+
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING);
 		}
 	}
 	return smp_processor_id();

Oleg Nesterov

2024-04-10 13:56 UTC #17 lore raw

Re: Nohz_full on boot CPU is broken (was: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work)

Hi Nicholas,

Quoted 15 lines · Nicholas Piggin

Thanks for this. Taking a while to page this back in, the intention is for housekeeping to be done by boot CPU until house keeper is awake, so returning smp_processor_id() seems like the right thing to do here for ephemeral jobs like timers and work, provided that CPU / mask is not stored somewhere long term by the caller.
For things that set an affinity like kthread, sched, maybe managed irqs, and such.
There are not many callers of housekeeping_any_cpu() so that's easy enough to verify. But similar like housekeeping_cpumask() and others could be an issue or at least a foot-gun, I'm not sure how well I convinced myself of those.

Quoted 7 lines

Could you test like this?
  WARN_ON_ONCE(system_state == SYSTEM_RUNNING ||
               type != HK_TYPE_TIMER);
With a comment to say other ephemeral mask types could be exempted if needed.

Sorry, I don't understand... Let me repeat, I know absolutely nothing about nonhz/etc. I didn't even try to really fix the problem(s), I am only trying to find a minimal/simple workaround to fix the problem we hit in Red Hat.

This is what I was going to send:

	--- a/kernel/sched/isolation.c
	+++ b/kernel/sched/isolation.c
	@@ -46,7 +46,15 @@ int housekeeping_any_cpu(enum hk_type type)
				if (cpu < nr_cpu_ids)
					return cpu;
	 
	-			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
	+			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
	+			if (likely(cpu < nr_cpu_ids))
	+				return cpu;
	+			/*
	+			 * Unless we have another problem this can only happen
	+			 * at boot time before start_secondary() brings the 1st
	+			 * housekeeping CPU up.
	+			 */
	+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING);
			}
		}
		return smp_processor_id();

Yes, this fixes the symptom, not the problem. And yes, "another problem" mentioned in the comment is very possible, say "maxcpus" kernel-parameter can be less than the first housekeeping cpu. But in this case the user should blame himself (and I am not sure the kernel will boot).

I don't understand why do you suggest to add "|| type != HK_TYPE_TIMER", currently all the callers of housekeeping_any_cpu() use type == HK_TIMER. But OK, I can add this check. I guess for the case it finds another user with type != HK_TYPE_TIMER which can't use smp_processor_id() even at boot time or stores the returned CPU for the long term.

Will you agree with the change above or what do you suggest instead as a simple workaround?

Quoted 5 lines

It would also be nice to warn for cases that would be bugs if the boot CPU was not in the HK mask. Could that be done by having a housekeepers_online() call after smp_init() (maybe at the start of sched_init_smp()) that could verify there is at least one online, and set a flag that could be used to create warnings.

Again, I am not sure I understand, but I too thought that something like

	housekeeping_check(void)
	{
		for_each_set_bit(type, &housekeeping.flags, HK_TYPE_MAX) {
			if (!cpumask_intersects(cpu_online, housekeeping.cpumasks[type]))
				panic();
		}

after bringup_nonboot_cpus(setup_max_cpus).

But I am not sure this is correct and this is another (although related) issue.

Oleg.

Oleg Nesterov

2024-04-11 13:43 UTC #18 lore raw

Re: Nohz_full on boot CPU is broken (was: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work)

Quoted 3 lines · Oleg Nesterov

Will you agree with the change above or what do you suggest instead as a simple workaround?

OK, let me send the patch.

Nicholas, Frederic, please review.

Quoted 10 lines

Again, I am not sure I understand, but I too thought that something like
	housekeeping_check(void)
	{
		for_each_set_bit(type, &housekeeping.flags, HK_TYPE_MAX) {
			if (!cpumask_intersects(cpu_online, housekeeping.cpumasks[type]))
				panic();
		}
after bringup_nonboot_cpus(setup_max_cpus).

It seems we can make a simpler patch for this. It will (should) fix another problem when maxcpus < first-housekeeping.

I'll recheck and try to do this later, lets solve the reported problem first.

Oleg.

Oleg Nesterov

2024-04-11 14:40 UTC #19 lore raw

PATCH 1/1 sched/isolation: fix boot crash when the boot CPU is nohz_full

In reply to Oleg Nesterov

Documentation/timers/no_hz.rst states that the "nohz_full=" mask must not include the boot CPU, this is no longer true after the commit 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full").

However after another commit aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time in this case; housekeeping_any_cpu() returns an invalid cpu nr until smp_init() paths bring the 1st housekeeping CPU up.

Change housekeeping_any_cpu() to check the result of cpumask_any_and() and return smp_processor_id() in this case. Yes, this is just the simple and backportable workaround which fixes the symptom, but smp_processor_id() at boot time should be safe at least for type == HK_TYPE_TIMER, this more or less matches the tick_do_timer_boot_cpu logic.

We should not worry about cpu_down(); tick_nohz_cpu_down() will not allow to offline tick_do_timer_cpu (the 1st online housekeeping CPU).

Fixes aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work")

Reported-by Chris von Recklinghausen <[email protected]>

Closes https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Oleg Nesterov <[email protected]>

---
 Documentation/timers/no_hz.rst |  7 ++-----
 kernel/sched/isolation.c       | 11 ++++++++++-
 2 files changed, 12 insertions(+), 6 deletions(-)

Documentation/timers/no_hz.rst +2 -5

index f8786be15183..7fe8ef9718d8 100644
@@ -129,11 +129,8 @@ adaptive-tick CPUs:  At least one non-adaptive-tick CPU must remain
 online to handle timekeeping tasks in order to ensure that system
 calls like gettimeofday() returns accurate values on adaptive-tick CPUs.
 (This is not an issue for CONFIG_NO_HZ_IDLE=y because there are no running
-user processes to observe slight drifts in clock rate.)  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.  Note that
-this means that your system must have at least two CPUs in order for
+user processes to observe slight drifts in clock rate.) Note that this
+means that your system must have at least two CPUs in order for
 CONFIG_NO_HZ_FULL=y to do anything for you.
 
 Finally, adaptive-ticks CPUs must have their RCU callbacks offloaded.

kernel/sched/isolation.c +10 -1

index 373d42c707bc..2a262d3ecb3d 100644
@@ -46,7 +46,16 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return cpu;
 
-			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			if (likely(cpu < nr_cpu_ids))
+				return cpu;
+			/*
+			 * Unless we have another problem this can only happen
+			 * at boot time before start_secondary() brings the 1st
+			 * housekeeping CPU up.
+			 */
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING ||
+				     type != HK_TYPE_TIMER);
 		}
 	}
 	return smp_processor_id();

Signature · 1 line

2.25.1.362.g51ebf55

Oleg Nesterov

2024-04-11 17:01 UTC #20 lore raw

Re: [PATCH] sched/isolation: fix boot crash when the boot CPU is nohz_full

Just in case...

This reveals another issue, tick_setup_device() can't use smp_call_function() due to irqs_disabled(), but lets discuss this later/separately too.

Quoted 71 lines · Oleg Nesterov

Documentation/timers/no_hz.rst states that the "nohz_full=" mask must not include the boot CPU, this is no longer true after the commit 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full").
However after another commit aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time in this case; housekeeping_any_cpu() returns an invalid cpu nr until smp_init() paths bring the 1st housekeeping CPU up.
Change housekeeping_any_cpu() to check the result of cpumask_any_and() and return smp_processor_id() in this case. Yes, this is just the simple and backportable workaround which fixes the symptom, but smp_processor_id() at boot time should be safe at least for type == HK_TYPE_TIMER, this more or less matches the tick_do_timer_boot_cpu logic.
We should not worry about cpu_down(); tick_nohz_cpu_down() will not allow to offline tick_do_timer_cpu (the 1st online housekeeping CPU).
Fixes aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work")
Reported-by Chris von Recklinghausen <[email protected]>
Closes https://lore.kernel.org/all/[email protected]/ open here
Signed-off-by Oleg Nesterov <[email protected]>
---
 Documentation/timers/no_hz.rst |  7 ++-----
 kernel/sched/isolation.c       | 11 ++++++++++-
 2 files changed, 12 insertions(+), 6 deletions(-)
Documentation/timers/no_hz.rst +2 -5
index f8786be15183..7fe8ef9718d8 100644
@@ -129,11 +129,8 @@ adaptive-tick CPUs:  At least one non-adaptive-tick CPU must remain
 online to handle timekeeping tasks in order to ensure that system
 calls like gettimeofday() returns accurate values on adaptive-tick CPUs.
 (This is not an issue for CONFIG_NO_HZ_IDLE=y because there are no running
-user processes to observe slight drifts in clock rate.)  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.  Note that
-this means that your system must have at least two CPUs in order for
+user processes to observe slight drifts in clock rate.) Note that this
+means that your system must have at least two CPUs in order for
 CONFIG_NO_HZ_FULL=y to do anything for you.
 
 Finally, adaptive-ticks CPUs must have their RCU callbacks offloaded.
kernel/sched/isolation.c +10 -1
index 373d42c707bc..2a262d3ecb3d 100644
@@ -46,7 +46,16 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return cpu;
 
-			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			if (likely(cpu < nr_cpu_ids))
+				return cpu;
+			/*
+			 * Unless we have another problem this can only happen
+			 * at boot time before start_secondary() brings the 1st
+			 * housekeeping CPU up.
+			 */
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING ||
+				     type != HK_TYPE_TIMER);
 		}
 	}
 	return smp_processor_id();
Signature · 1 line
2.25.1.362.g51ebf55

Oleg Nesterov

2024-04-13 14:19 UTC #21 lore raw

PATCH 1/1 sched/isolation: fix boot crash when maxcpus < first-housekeeping-cpu

In reply to Oleg Nesterov

housekeeping_setup() checks cpumask_intersects(present, online) to ensure that the kernel will have at least one housekeeping CPU after smp_init(), but this doesn't work if the maxcpus= kernel parameter limits the number of processors available after bootup.

For example, the kernel with "maxcpus=2 nohz_full=0-2" parameters crashes at boot time on my virtual machine with 4 CPUs.

Change housekeeping_setup() to use cpumask_first_and() and check that the returned cpu number is valid and less than setup_max_cpus.

Another corner case is "nohz_full=0" on a machine with a single CPU or with the maxcpus=1 kernel argument. In this case non_housekeeping_mask is empty and IIUC tick_nohz_full_setup() makes no sense. And indeed, the kernel hits the WARN_ON(tick_nohz_full_running) in tick_sched_do_timer().

And how should the kernel interpret the "nohz_full=" parameter? I think it should be silently ignored, but currently cpulist_parse() happily returns the empty cpumask and this leads to the same problem.

Change housekeeping_setup() to check cpumask_empty(non_housekeeping_mask) and do nothing in this case.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/sched/isolation.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

kernel/sched/isolation.c +6 -1

index 2a262d3ecb3d..5891e715f00d 100644
@@ -118,6 +118,7 @@ static void __init housekeeping_setup_type(enum hk_type type,
 static int __init housekeeping_setup(char *str, unsigned long flags)
 {
 	cpumask_var_t non_housekeeping_mask, housekeeping_staging;
+	unsigned int first_cpu;
 	int err = 0;
 
 	if ((flags & HK_FLAG_TICK) && !(housekeeping.flags & HK_FLAG_TICK)) {

@@ -138,7 +139,8 @@ 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)) {
+	first_cpu = cpumask_first_and(cpu_present_mask, housekeeping_staging);
+	if (first_cpu >= nr_cpu_ids || first_cpu >= setup_max_cpus) {
 		__cpumask_set_cpu(smp_processor_id(), housekeeping_staging);
 		__cpumask_clear_cpu(smp_processor_id(), non_housekeeping_mask);
 		if (!housekeeping.flags) {

@@ -147,6 +149,9 @@ static int __init housekeeping_setup(char *str, unsigned long flags)
 		}
 	}
 
+	if (cpumask_empty(non_housekeeping_mask))
+		goto free_housekeeping_staging;
+
 	if (!housekeeping.flags) {
 		/* First setup call ("nohz_full=" or "isolcpus=") */
 		enum hk_type type;

Signature · 1 line

2.25.1.362.g51ebf55

Phil Auld

Reviewed-by on v1 1/1

2024-04-18 14:54 UTC #22 lore raw

Re: [PATCH] sched/isolation: fix boot crash when maxcpus < first-housekeeping-cpu

Quoted 16 lines · On Sat, Apr 13, 2024 at 04:17:46PM +0200 Oleg Nesterov wrote:

housekeeping_setup() checks cpumask_intersects(present, online) to ensure that the kernel will have at least one housekeeping CPU after smp_init(), but this doesn't work if the maxcpus= kernel parameter limits the number of processors available after bootup.
For example, the kernel with "maxcpus=2 nohz_full=0-2" parameters crashes at boot time on my virtual machine with 4 CPUs.
Change housekeeping_setup() to use cpumask_first_and() and check that the returned cpu number is valid and less than setup_max_cpus.
Another corner case is "nohz_full=0" on a machine with a single CPU or with the maxcpus=1 kernel argument. In this case non_housekeeping_mask is empty and IIUC tick_nohz_full_setup() makes no sense. And indeed, the kernel hits the WARN_ON(tick_nohz_full_running) in tick_sched_do_timer().

Quoted 8 lines

And how should the kernel interpret the "nohz_full=" parameter? I think it should be silently ignored, but currently cpulist_parse() happily returns the empty cpumask and this leads to the same problem.
Change housekeeping_setup() to check cpumask_empty(non_housekeeping_mask) and do nothing in this case.
Signed-off-by Oleg Nesterov <[email protected]>

Looks good to me. One less footgun.

Reviewed-by Phil Auld <[email protected]>

Quoted 41 lines

---
 kernel/sched/isolation.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

kernel/sched/isolation.c +6 -1

index 2a262d3ecb3d..5891e715f00d 100644
@@ -118,6 +118,7 @@ static void __init housekeeping_setup_type(enum hk_type type,
 static int __init housekeeping_setup(char *str, unsigned long flags)
 {
 	cpumask_var_t non_housekeeping_mask, housekeeping_staging;
+	unsigned int first_cpu;
 	int err = 0;
 
 	if ((flags & HK_FLAG_TICK) && !(housekeeping.flags & HK_FLAG_TICK)) {

@@ -138,7 +139,8 @@ 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)) {
+	first_cpu = cpumask_first_and(cpu_present_mask, housekeeping_staging);
+	if (first_cpu >= nr_cpu_ids || first_cpu >= setup_max_cpus) {
 		__cpumask_set_cpu(smp_processor_id(), housekeeping_staging);
 		__cpumask_clear_cpu(smp_processor_id(), non_housekeeping_mask);
 		if (!housekeeping.flags) {

@@ -147,6 +149,9 @@ static int __init housekeeping_setup(char *str, unsigned long flags)
 		}
 	}
 
+	if (cpumask_empty(non_housekeeping_mask))
+		goto free_housekeeping_staging;
+
 	if (!housekeeping.flags) {
 		/* First setup call ("nohz_full=" or "isolcpus=") */
 		enum hk_type type;

Signature · 1 line

2.25.1.362.g51ebf55

Frederic Weisbecker

maintainer Acked-by on v1 1/1

2024-04-18 15:40 UTC #23 lore raw

Re: [PATCH] sched/isolation: fix boot crash when maxcpus < first-housekeeping-cpu

In reply to Oleg Nesterov

Quoted 16 lines · Oleg Nesterov

housekeeping_setup() checks cpumask_intersects(present, online) to ensure that the kernel will have at least one housekeeping CPU after smp_init(), but this doesn't work if the maxcpus= kernel parameter limits the number of processors available after bootup.
For example, the kernel with "maxcpus=2 nohz_full=0-2" parameters crashes at boot time on my virtual machine with 4 CPUs.
Change housekeeping_setup() to use cpumask_first_and() and check that the returned cpu number is valid and less than setup_max_cpus.
Another corner case is "nohz_full=0" on a machine with a single CPU or with the maxcpus=1 kernel argument. In this case non_housekeeping_mask is empty and IIUC tick_nohz_full_setup() makes no sense. And indeed, the kernel hits the WARN_ON(tick_nohz_full_running) in tick_sched_do_timer().

Quoted 8 lines

And how should the kernel interpret the "nohz_full=" parameter? I think it should be silently ignored, but currently cpulist_parse() happily returns the empty cpumask and this leads to the same problem.
Change housekeeping_setup() to check cpumask_empty(non_housekeeping_mask) and do nothing in this case.
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

tip-bot2 for Oleg Nesterov

merge bot Applied

2024-04-24 20:05 UTC #24 lore raw

[tip: sched/urgent] sched/isolation: Fix boot crash when maxcpus < first housekeeping CPU

In reply to Oleg Nesterov

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     b6ad00418eaf376b4f2a68a1696d6368c1381310
Gitweb:        https://git.kernel.org/tip/b6ad00418eaf376b4f2a68a1696d6368c1381310
Author:        Oleg Nesterov <[email protected]>
AuthorDate:    Sat, 13 Apr 2024 16:17:46 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Wed, 24 Apr 2024 21:53:34 +02:00

sched/isolation: Fix boot crash when maxcpus < first housekeeping CPU

For example, a kernel with "maxcpus=2 nohz_full=0-2" parameters crashes at boot time on a virtual machine with 4 CPUs.

Change housekeeping_setup() to use cpumask_first_and() and check that the returned CPU number is valid and less than setup_max_cpus.

Another corner case is "nohz_full=0" on a machine with a single CPU or with the maxcpus=1 kernel argument. In this case non_housekeeping_mask is empty and tick_nohz_full_setup() makes no sense. And indeed, the kernel hits the WARN_ON(tick_nohz_full_running) in tick_sched_do_timer().

And how should the kernel interpret the "nohz_full=" parameter? It should be silently ignored, but currently cpulist_parse() happily returns the empty cpumask and this leads to the same problem.

Change housekeeping_setup() to check cpumask_empty(non_housekeeping_mask) and do nothing in this case.

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Reviewed-by Phil Auld <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 kernel/sched/isolation.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

kernel/sched/isolation.c +6 -1

index 2a262d3..5891e71 100644
@@ -118,6 +118,7 @@ static void __init housekeeping_setup_type(enum hk_type type,
 static int __init housekeeping_setup(char *str, unsigned long flags)
 {
 	cpumask_var_t non_housekeeping_mask, housekeeping_staging;
+	unsigned int first_cpu;
 	int err = 0;
 
 	if ((flags & HK_FLAG_TICK) && !(housekeeping.flags & HK_FLAG_TICK)) {

@@ -138,7 +139,8 @@ 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)) {
+	first_cpu = cpumask_first_and(cpu_present_mask, housekeeping_staging);
+	if (first_cpu >= nr_cpu_ids || first_cpu >= setup_max_cpus) {
 		__cpumask_set_cpu(smp_processor_id(), housekeeping_staging);
 		__cpumask_clear_cpu(smp_processor_id(), non_housekeeping_mask);
 		if (!housekeeping.flags) {

@@ -147,6 +149,9 @@ static int __init housekeeping_setup(char *str, unsigned long flags)
 		}
 	}
 
+	if (cpumask_empty(non_housekeeping_mask))
+		goto free_housekeeping_staging;
+
 	if (!housekeeping.flags) {
 		/* First setup call ("nohz_full=" or "isolcpus=") */
 		enum hk_type type;

Ingo Molnar

2024-04-28 08:13 UTC #25 lore raw

Re: [tip: sched/urgent] sched/isolation: Fix boot crash when maxcpus < first housekeeping CPU

Quoted 11 lines · * tip-bot2 for Oleg Nesterov <[email protected]> wrote:

Another corner case is "nohz_full=0" on a machine with a single CPU or with the maxcpus=1 kernel argument. In this case non_housekeeping_mask is empty and tick_nohz_full_setup() makes no sense. And indeed, the kernel hits the WARN_ON(tick_nohz_full_running) in tick_sched_do_timer().
And how should the kernel interpret the "nohz_full=" parameter? It should be silently ignored, but currently cpulist_parse() happily returns the empty cpumask and this leads to the same problem.
Change housekeeping_setup() to check cpumask_empty(non_housekeeping_mask) and do nothing in this case.

So arguably the user meant NOHZ_FULL to be turned off - but it is de-facto already turned off by the fact that there's only a single CPU available, right?

Thanks,

Ingo

Oleg Nesterov

2024-04-28 13:18 UTC #26 lore raw

Re: [tip: sched/urgent] sched/isolation: Fix boot crash when maxcpus < first housekeeping CPU

Quoted 12 lines · Ingo Molnar

Quoted 9 lines · * tip-bot2 for Oleg Nesterov <[email protected]> wrote:
Another corner case is "nohz_full=0" on a machine with a single CPU or with the maxcpus=1 kernel argument. In this case non_housekeeping_mask is empty and tick_nohz_full_setup() makes no sense. And indeed, the kernel hits the WARN_ON(tick_nohz_full_running) in tick_sched_do_timer().
And how should the kernel interpret the "nohz_full=" parameter? It should be silently ignored, but currently cpulist_parse() happily returns the empty cpumask and this leads to the same problem.

Quoted 6 lines

Quoted 2 lines
Change housekeeping_setup() to check cpumask_empty(non_housekeeping_mask) and do nothing in this case.
So arguably the user meant NOHZ_FULL to be turned off - but it is de-facto already turned off by the fact that there's only a single CPU available, right?

Or the user passes the empty "nohz_full=" mask on a multi-CPU machine.

In both cases (before this patch) housekeeping_setup() calls tick_nohz_full_setup(non_housekeeping_mask) which sets tick_nohz_full_running = true even if tick_nohz_full_mask is empty.

This doesn't look right to me and triggers the "this should not happen" warning in tick_sched_do_timer().

But let me repeat, I know nothing about nohz/etc.

Oleg.

tip-bot2 for Oleg Nesterov

merge bot Applied

2024-04-28 08:24 UTC #27 lore raw

[tip: sched/urgent] sched/isolation: Fix boot crash when maxcpus < first housekeeping CPU

In reply to Oleg Nesterov

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     257bf89d84121280904800acd25cc2c444c717ae
Gitweb:        https://git.kernel.org/tip/257bf89d84121280904800acd25cc2c444c717ae
Author:        Oleg Nesterov <[email protected]>
AuthorDate:    Sat, 13 Apr 2024 16:17:46 +02:00
Committer:     Ingo Molnar <[email protected]>
CommitterDate: Sun, 28 Apr 2024 10:08:21 +02:00

sched/isolation: Fix boot crash when maxcpus < first housekeeping CPU

For example, a kernel with "maxcpus=2 nohz_full=0-2" parameters crashes at boot time on a virtual machine with 4 CPUs.

Change housekeeping_setup() to use cpumask_first_and() and check that the returned CPU number is valid and less than setup_max_cpus.

And how should the kernel interpret the "nohz_full=" parameter? It should be silently ignored, but currently cpulist_parse() happily returns the empty cpumask and this leads to the same problem.

Change housekeeping_setup() to check cpumask_empty(non_housekeeping_mask) and do nothing in this case.

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

Reviewed-by Phil Auld <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 kernel/sched/isolation.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

kernel/sched/isolation.c +6 -1

index 2a262d3..5891e71 100644
@@ -118,6 +118,7 @@ static void __init housekeeping_setup_type(enum hk_type type,
 static int __init housekeeping_setup(char *str, unsigned long flags)
 {
 	cpumask_var_t non_housekeeping_mask, housekeeping_staging;
+	unsigned int first_cpu;
 	int err = 0;
 
 	if ((flags & HK_FLAG_TICK) && !(housekeeping.flags & HK_FLAG_TICK)) {

@@ -138,7 +139,8 @@ 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)) {
+	first_cpu = cpumask_first_and(cpu_present_mask, housekeeping_staging);
+	if (first_cpu >= nr_cpu_ids || first_cpu >= setup_max_cpus) {
 		__cpumask_set_cpu(smp_processor_id(), housekeeping_staging);
 		__cpumask_clear_cpu(smp_processor_id(), non_housekeeping_mask);
 		if (!housekeeping.flags) {

@@ -147,6 +149,9 @@ static int __init housekeeping_setup(char *str, unsigned long flags)
 		}
 	}
 
+	if (cpumask_empty(non_housekeeping_mask))
+		goto free_housekeeping_staging;
+
 	if (!housekeeping.flags) {
 		/* First setup call ("nohz_full=" or "isolcpus=") */
 		enum hk_type type;

Frederic Weisbecker

maintainer Acked-by on v1 1/1

2024-04-15 21:37 UTC #28 lore raw

Re: [PATCH] sched/isolation: fix boot crash when the boot CPU is nohz_full

In reply to Oleg Nesterov

Quoted 15 lines · Oleg Nesterov

Documentation/timers/no_hz.rst states that the "nohz_full=" mask must not include the boot CPU, this is no longer true after the commit 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full").
However after another commit aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time in this case; housekeeping_any_cpu() returns an invalid cpu nr until smp_init() paths bring the 1st housekeeping CPU up.
Change housekeeping_any_cpu() to check the result of cpumask_any_and() and return smp_processor_id() in this case. Yes, this is just the simple and backportable workaround which fixes the symptom, but smp_processor_id() at boot time should be safe at least for type == HK_TYPE_TIMER, this more or less matches the tick_do_timer_boot_cpu logic.

Quoted 7 lines

We should not worry about cpu_down(); tick_nohz_cpu_down() will not allow to offline tick_do_timer_cpu (the 1st online housekeeping CPU).
Fixes aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work")
Reported-by Chris von Recklinghausen <[email protected]>
Closes https://lore.kernel.org/all/[email protected]/ open here
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

Phil Auld

Reviewed-by on v1 1/1

2024-04-18 14:50 UTC #29 lore raw

Re: [PATCH] sched/isolation: fix boot crash when the boot CPU is nohz_full

In reply to Oleg Nesterov

Quoted 15 lines · On Thu, Apr 11, 2024 at 04:39:05PM +0200 Oleg Nesterov wrote:

Documentation/timers/no_hz.rst states that the "nohz_full=" mask must not include the boot CPU, this is no longer true after the commit 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full").
However after another commit aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time in this case; housekeeping_any_cpu() returns an invalid cpu nr until smp_init() paths bring the 1st housekeeping CPU up.
Change housekeeping_any_cpu() to check the result of cpumask_any_and() and return smp_processor_id() in this case. Yes, this is just the simple and backportable workaround which fixes the symptom, but smp_processor_id() at boot time should be safe at least for type == HK_TYPE_TIMER, this more or less matches the tick_do_timer_boot_cpu logic.

Quoted 7 lines

We should not worry about cpu_down(); tick_nohz_cpu_down() will not allow to offline tick_do_timer_cpu (the 1st online housekeeping CPU).
Fixes aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work")
Reported-by Chris von Recklinghausen <[email protected]>
Closes https://lore.kernel.org/all/[email protected]/ open here
Signed-off-by Oleg Nesterov <[email protected]>

Checking the returned value instead of assuming seems safer in any case.

Reviewed-by Phil Auld <[email protected]>

Quoted 50 lines

---
 Documentation/timers/no_hz.rst |  7 ++-----
 kernel/sched/isolation.c       | 11 ++++++++++-
 2 files changed, 12 insertions(+), 6 deletions(-)

Documentation/timers/no_hz.rst +2 -5

index f8786be15183..7fe8ef9718d8 100644
@@ -129,11 +129,8 @@ adaptive-tick CPUs:  At least one non-adaptive-tick CPU must remain
 online to handle timekeeping tasks in order to ensure that system
 calls like gettimeofday() returns accurate values on adaptive-tick CPUs.
 (This is not an issue for CONFIG_NO_HZ_IDLE=y because there are no running
-user processes to observe slight drifts in clock rate.)  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.  Note that
-this means that your system must have at least two CPUs in order for
+user processes to observe slight drifts in clock rate.) Note that this
+means that your system must have at least two CPUs in order for
 CONFIG_NO_HZ_FULL=y to do anything for you.
 
 Finally, adaptive-ticks CPUs must have their RCU callbacks offloaded.

kernel/sched/isolation.c +10 -1

index 373d42c707bc..2a262d3ecb3d 100644
@@ -46,7 +46,16 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return cpu;
 
-			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			if (likely(cpu < nr_cpu_ids))
+				return cpu;
+			/*
+			 * Unless we have another problem this can only happen
+			 * at boot time before start_secondary() brings the 1st
+			 * housekeeping CPU up.
+			 */
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING ||
+				     type != HK_TYPE_TIMER);
 		}
 	}
 	return smp_processor_id();

Signature · 1 line

2.25.1.362.g51ebf55

Oleg Nesterov

2024-04-22 18:51 UTC #30 lore raw

Re: [PATCH] sched/isolation: fix boot crash when the boot CPU is nohz_full

Phil, Frederic,

Thanks for your review! Who do you think can take these patches? At least the 1st one.

To remind, there are more problems with boot CPU in nohz mask, but can we at least fix the kernel crash?

Oleg.

Quoted 82 lines · Phil Auld

Quoted 15 lines · On Thu, Apr 11, 2024 at 04:39:05PM +0200 Oleg Nesterov wrote:
Documentation/timers/no_hz.rst states that the "nohz_full=" mask must not include the boot CPU, this is no longer true after the commit 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full").
However after another commit aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time in this case; housekeeping_any_cpu() returns an invalid cpu nr until smp_init() paths bring the 1st housekeeping CPU up.
Change housekeeping_any_cpu() to check the result of cpumask_any_and() and return smp_processor_id() in this case. Yes, this is just the simple and backportable workaround which fixes the symptom, but smp_processor_id() at boot time should be safe at least for type == HK_TYPE_TIMER, this more or less matches the tick_do_timer_boot_cpu logic.
Quoted 7 lines
We should not worry about cpu_down(); tick_nohz_cpu_down() will not allow to offline tick_do_timer_cpu (the 1st online housekeeping CPU).
Fixes aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work")
Reported-by Chris von Recklinghausen <[email protected]>
Closes https://lore.kernel.org/all/[email protected]/ open here
Signed-off-by Oleg Nesterov <[email protected]>
Checking the returned value instead of assuming seems safer in any case.
Reviewed-by Phil Auld <[email protected]>
Quoted 46 lines
---
 Documentation/timers/no_hz.rst |  7 ++-----
 kernel/sched/isolation.c       | 11 ++++++++++-
 2 files changed, 12 insertions(+), 6 deletions(-)
Documentation/timers/no_hz.rst +2 -5
index f8786be15183..7fe8ef9718d8 100644
@@ -129,11 +129,8 @@ adaptive-tick CPUs:  At least one non-adaptive-tick CPU must remain
 online to handle timekeeping tasks in order to ensure that system
 calls like gettimeofday() returns accurate values on adaptive-tick CPUs.
 (This is not an issue for CONFIG_NO_HZ_IDLE=y because there are no running
-user processes to observe slight drifts in clock rate.)  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.  Note that
-this means that your system must have at least two CPUs in order for
+user processes to observe slight drifts in clock rate.) Note that this
+means that your system must have at least two CPUs in order for
 CONFIG_NO_HZ_FULL=y to do anything for you.
 
 Finally, adaptive-ticks CPUs must have their RCU callbacks offloaded.
kernel/sched/isolation.c +10 -2
index 373d42c707bc..2a262d3ecb3d 100644
@@ -46,7 +46,16 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return cpu;
 
-			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			if (likely(cpu < nr_cpu_ids))
+				return cpu;
+			/*
+			 * Unless we have another problem this can only happen
+			 * at boot time before start_secondary() brings the 1st
+			 * housekeeping CPU up.
+			 */
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING ||
+				     type != HK_TYPE_TIMER);
 		}
 	}
 	return smp_processor_id();
--
Quoted 4 lines
2.25.1.362.g51ebf55
--

Phil Auld

2024-04-24 14:42 UTC #31 lore raw

Re: [PATCH] sched/isolation: fix boot crash when the boot CPU is nohz_full

HI Oleg, Ingo,

Quoted 8 lines · On Mon, Apr 22, 2024 at 08:50:21PM +0200 Oleg Nesterov wrote:

Phil, Frederic,
Thanks for your review! Who do you think can take these patches? At least the 1st one.
To remind, there are more problems with boot CPU in nohz mask, but can we at least fix the kernel crash?

I think that would be good. I don't know if Peter is at full strength.

Ingo could you take look at this, please?

Cheers,
Phil

Quoted 86 lines

Oleg.
Quoted 82 lines · Phil Auld
Quoted 15 lines · On Thu, Apr 11, 2024 at 04:39:05PM +0200 Oleg Nesterov wrote:
Documentation/timers/no_hz.rst states that the "nohz_full=" mask must not include the boot CPU, this is no longer true after the commit 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full").
However after another commit aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time in this case; housekeeping_any_cpu() returns an invalid cpu nr until smp_init() paths bring the 1st housekeeping CPU up.
Change housekeeping_any_cpu() to check the result of cpumask_any_and() and return smp_processor_id() in this case. Yes, this is just the simple and backportable workaround which fixes the symptom, but smp_processor_id() at boot time should be safe at least for type == HK_TYPE_TIMER, this more or less matches the tick_do_timer_boot_cpu logic.
Quoted 7 lines
We should not worry about cpu_down(); tick_nohz_cpu_down() will not allow to offline tick_do_timer_cpu (the 1st online housekeeping CPU).
Fixes aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work")
Reported-by Chris von Recklinghausen <[email protected]>
Closes https://lore.kernel.org/all/[email protected]/ open here
Signed-off-by Oleg Nesterov <[email protected]>
Checking the returned value instead of assuming seems safer in any case.
Reviewed-by Phil Auld <[email protected]>
Quoted 46 lines
---
 Documentation/timers/no_hz.rst |  7 ++-----
 kernel/sched/isolation.c       | 11 ++++++++++-
 2 files changed, 12 insertions(+), 6 deletions(-)
Documentation/timers/no_hz.rst +2 -5
index f8786be15183..7fe8ef9718d8 100644
@@ -129,11 +129,8 @@ adaptive-tick CPUs:  At least one non-adaptive-tick CPU must remain
 online to handle timekeeping tasks in order to ensure that system
 calls like gettimeofday() returns accurate values on adaptive-tick CPUs.
 (This is not an issue for CONFIG_NO_HZ_IDLE=y because there are no running
-user processes to observe slight drifts in clock rate.)  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.  Note that
-this means that your system must have at least two CPUs in order for
+user processes to observe slight drifts in clock rate.) Note that this
+means that your system must have at least two CPUs in order for
 CONFIG_NO_HZ_FULL=y to do anything for you.
 
 Finally, adaptive-ticks CPUs must have their RCU callbacks offloaded.
kernel/sched/isolation.c +10 -2
index 373d42c707bc..2a262d3ecb3d 100644
@@ -46,7 +46,16 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return cpu;
 
-			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			if (likely(cpu < nr_cpu_ids))
+				return cpu;
+			/*
+			 * Unless we have another problem this can only happen
+			 * at boot time before start_secondary() brings the 1st
+			 * housekeeping CPU up.
+			 */
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING ||
+				     type != HK_TYPE_TIMER);
 		}
 	}
 	return smp_processor_id();
--
Quoted 4 lines
2.25.1.362.g51ebf55
--

tip-bot2 for Oleg Nesterov

merge bot Applied

2024-04-24 20:05 UTC #32 lore raw

[tip: sched/urgent] sched/isolation: {revent boot crash when the boot CPU is nohz_full

In reply to Oleg Nesterov

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     8e3101b38dfc20848a23525b1e6e80bd1641d44c
Gitweb:        https://git.kernel.org/tip/8e3101b38dfc20848a23525b1e6e80bd1641d44c
Author:        Oleg Nesterov <[email protected]>
AuthorDate:    Thu, 11 Apr 2024 16:39:05 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Wed, 24 Apr 2024 21:53:34 +02:00

sched/isolation: {revent boot crash when the boot CPU is nohz_full

Documentation/timers/no_hz.rst states that the "nohz_full=" mask must not include the boot CPU, which is no longer true after commit 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full").

However after commit aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time in this case; housekeeping_any_cpu() returns an invalid CPU number until smp_init() brings the first housekeeping CPU up.

Change housekeeping_any_cpu() to check the result of cpumask_any_and() and return smp_processor_id() in this case.

This is just the simple and backportable workaround which fixes the symptom, but smp_processor_id() at boot time should be safe at least for type == HK_TYPE_TIMER, this more or less matches the tick_do_timer_boot_cpu logic.

There is no worry about cpu_down(); tick_nohz_cpu_down() will not allow to offline tick_do_timer_cpu (the 1st online housekeeping CPU).

Fixes aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work")

Reported-by Chris von Recklinghausen <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Reviewed-by Phil Auld <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Closes https://lore.kernel.org/all/[email protected]/ open here

---
 Documentation/timers/no_hz.rst |  7 ++-----
 kernel/sched/isolation.c       | 11 ++++++++++-
 2 files changed, 12 insertions(+), 6 deletions(-)

Documentation/timers/no_hz.rst +2 -5

index f8786be..7fe8ef9 100644
@@ -129,11 +129,8 @@ adaptive-tick CPUs:  At least one non-adaptive-tick CPU must remain
 online to handle timekeeping tasks in order to ensure that system
 calls like gettimeofday() returns accurate values on adaptive-tick CPUs.
 (This is not an issue for CONFIG_NO_HZ_IDLE=y because there are no running
-user processes to observe slight drifts in clock rate.)  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.  Note that
-this means that your system must have at least two CPUs in order for
+user processes to observe slight drifts in clock rate.) Note that this
+means that your system must have at least two CPUs in order for
 CONFIG_NO_HZ_FULL=y to do anything for you.
 
 Finally, adaptive-ticks CPUs must have their RCU callbacks offloaded.

kernel/sched/isolation.c +10 -1

index 373d42c..2a262d3 100644
@@ -46,7 +46,16 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return cpu;
 
-			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			if (likely(cpu < nr_cpu_ids))
+				return cpu;
+			/*
+			 * Unless we have another problem this can only happen
+			 * at boot time before start_secondary() brings the 1st
+			 * housekeeping CPU up.
+			 */
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING ||
+				     type != HK_TYPE_TIMER);
 		}
 	}
 	return smp_processor_id();

Phil Auld

2024-04-24 20:41 UTC #33 lore raw

Re: [tip: sched/urgent] sched/isolation: {revent boot crash when the boot CPU is nohz_full

Quoted 11 lines · On Wed, Apr 24, 2024 at 08:05:02PM -0000 tip-bot2 for Oleg Nesterov wrote:

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     8e3101b38dfc20848a23525b1e6e80bd1641d44c
Gitweb:        https://git.kernel.org/tip/8e3101b38dfc20848a23525b1e6e80bd1641d44c
Author:        Oleg Nesterov <[email protected]>
AuthorDate:    Thu, 11 Apr 2024 16:39:05 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Wed, 24 Apr 2024 21:53:34 +02:00

sched/isolation: {revent boot crash when the boot CPU is nohz_full

Thanks Thomas, Typo in the reworded description :)

Quoted 74 lines

Documentation/timers/no_hz.rst states that the "nohz_full=" mask must not include the boot CPU, which is no longer true after commit 08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full").
However after commit aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work") the kernel will crash at boot time in this case; housekeeping_any_cpu() returns an invalid CPU number until smp_init() brings the first housekeeping CPU up.
Change housekeeping_any_cpu() to check the result of cpumask_any_and() and return smp_processor_id() in this case.
This is just the simple and backportable workaround which fixes the symptom, but smp_processor_id() at boot time should be safe at least for type == HK_TYPE_TIMER, this more or less matches the tick_do_timer_boot_cpu logic.
There is no worry about cpu_down(); tick_nohz_cpu_down() will not allow to offline tick_do_timer_cpu (the 1st online housekeeping CPU).
Fixes aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work")
Reported-by Chris von Recklinghausen <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
Reviewed-by Phil Auld <[email protected]>
Acked-by Frederic Weisbecker <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Closes https://lore.kernel.org/all/[email protected]/ open here
---
 Documentation/timers/no_hz.rst |  7 ++-----
 kernel/sched/isolation.c       | 11 ++++++++++-
 2 files changed, 12 insertions(+), 6 deletions(-)
Documentation/timers/no_hz.rst +2 -5
index f8786be..7fe8ef9 100644
@@ -129,11 +129,8 @@ adaptive-tick CPUs:  At least one non-adaptive-tick CPU must remain
 online to handle timekeeping tasks in order to ensure that system
 calls like gettimeofday() returns accurate values on adaptive-tick CPUs.
 (This is not an issue for CONFIG_NO_HZ_IDLE=y because there are no running
-user processes to observe slight drifts in clock rate.)  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.  Note that
-this means that your system must have at least two CPUs in order for
+user processes to observe slight drifts in clock rate.) Note that this
+means that your system must have at least two CPUs in order for
 CONFIG_NO_HZ_FULL=y to do anything for you.
 
 Finally, adaptive-ticks CPUs must have their RCU callbacks offloaded.
kernel/sched/isolation.c +10 -1
index 373d42c..2a262d3 100644
@@ -46,7 +46,16 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return cpu;
 
-			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			if (likely(cpu < nr_cpu_ids))
+				return cpu;
+			/*
+			 * Unless we have another problem this can only happen
+			 * at boot time before start_secondary() brings the 1st
+			 * housekeeping CPU up.
+			 */
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING ||
+				     type != HK_TYPE_TIMER);
 		}
 	}
 	return smp_processor_id();

Ingo Molnar

2024-04-28 08:14 UTC #34 lore raw

Re: [tip: sched/urgent] sched/isolation: {revent boot crash when the boot CPU is nohz_full

Quoted 10 lines · Phil Auld

Quoted 9 lines · On Wed, Apr 24, 2024 at 08:05:02PM -0000 tip-bot2 for Oleg Nesterov wrote:
The following commit has been merged into the sched/urgent branch of tip:
Commit-ID:     8e3101b38dfc20848a23525b1e6e80bd1641d44c
Gitweb:        https://git.kernel.org/tip/8e3101b38dfc20848a23525b1e6e80bd1641d44c
Author:        Oleg Nesterov <[email protected]>
AuthorDate:    Thu, 11 Apr 2024 16:39:05 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Wed, 24 Apr 2024 21:53:34 +02:00

Quoted 4 lines

Quoted 2 lines
sched/isolation: {revent boot crash when the boot CPU is nohz_full
Thanks Thomas, Typo in the reworded description :)

Ok, so normally we wouldn't rebase just for a typo in a changelog, but that's an annoying typo that will show up in shortlogs - so I fixed it all up in tip:sched/urgent.

Thanks,

Ingo

Phil Auld

2024-04-29 11:50 UTC #35 lore raw

Re: [tip: sched/urgent] sched/isolation: {revent boot crash when the boot CPU is nohz_full

Quoted 13 lines · On Sun, Apr 28, 2024 at 10:14:30AM +0200 Ingo Molnar wrote:

Quoted 10 lines · Phil Auld
Quoted 9 lines · On Wed, Apr 24, 2024 at 08:05:02PM -0000 tip-bot2 for Oleg Nesterov wrote:
The following commit has been merged into the sched/urgent branch of tip:
Commit-ID:     8e3101b38dfc20848a23525b1e6e80bd1641d44c
Gitweb:        https://git.kernel.org/tip/8e3101b38dfc20848a23525b1e6e80bd1641d44c
Author:        Oleg Nesterov <[email protected]>
AuthorDate:    Thu, 11 Apr 2024 16:39:05 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Wed, 24 Apr 2024 21:53:34 +02:00

Quoted 9 lines

Quoted 4 lines
Quoted 2 lines
sched/isolation: {revent boot crash when the boot CPU is nohz_full
Thanks Thomas, Typo in the reworded description :)
Ok, so normally we wouldn't rebase just for a typo in a changelog, but that's an annoying typo that will show up in shortlogs - so I fixed it all up in tip:sched/urgent.

Yeah, I kept seeing "revert"...

Thanks,
Phil

Quoted 4 lines

Thanks,
Ingo

tip-bot2 for Oleg Nesterov

merge bot Applied

2024-04-28 08:24 UTC #36 lore raw

[tip: sched/urgent] sched/isolation: Prevent boot crash when the boot CPU is nohz_full

In reply to Oleg Nesterov

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     5097cbcb38e6e0d2627c9dde1985e91d2c9f880e
Gitweb:        https://git.kernel.org/tip/5097cbcb38e6e0d2627c9dde1985e91d2c9f880e
Author:        Oleg Nesterov <[email protected]>
AuthorDate:    Thu, 11 Apr 2024 16:39:05 +02:00
Committer:     Ingo Molnar <[email protected]>
CommitterDate: Sun, 28 Apr 2024 10:07:12 +02:00

sched/isolation: Prevent boot crash when the boot CPU is nohz_full

Documentation/timers/no_hz.rst states that the "nohz_full=" mask must not include the boot CPU, which is no longer true after:

08ae95f4fd3b ("nohz_full: Allow the boot CPU to be nohz_full").

However after:

aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work")

the kernel will crash at boot time in this case; housekeeping_any_cpu() returns an invalid CPU number until smp_init() brings the first housekeeping CPU up.

Change housekeeping_any_cpu() to check the result of cpumask_any_and() and return smp_processor_id() in this case.

There is no worry about cpu_down(); tick_nohz_cpu_down() will not allow to offline tick_do_timer_cpu (the 1st online housekeeping CPU).

Fixes aae17ebb53cd ("workqueue: Avoid using isolated cpus' timers on queue_delayed_work")

Reported-by Chris von Recklinghausen <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

Reviewed-by Phil Auld <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Closes https://lore.kernel.org/all/[email protected]/ open here

---
 Documentation/timers/no_hz.rst |  7 ++-----
 kernel/sched/isolation.c       | 11 ++++++++++-
 2 files changed, 12 insertions(+), 6 deletions(-)

Documentation/timers/no_hz.rst +2 -5

index f8786be..7fe8ef9 100644
@@ -129,11 +129,8 @@ adaptive-tick CPUs:  At least one non-adaptive-tick CPU must remain
 online to handle timekeeping tasks in order to ensure that system
 calls like gettimeofday() returns accurate values on adaptive-tick CPUs.
 (This is not an issue for CONFIG_NO_HZ_IDLE=y because there are no running
-user processes to observe slight drifts in clock rate.)  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.  Note that
-this means that your system must have at least two CPUs in order for
+user processes to observe slight drifts in clock rate.) Note that this
+means that your system must have at least two CPUs in order for
 CONFIG_NO_HZ_FULL=y to do anything for you.
 
 Finally, adaptive-ticks CPUs must have their RCU callbacks offloaded.

kernel/sched/isolation.c +10 -1

index 373d42c..2a262d3 100644
@@ -46,7 +46,16 @@ int housekeeping_any_cpu(enum hk_type type)
 			if (cpu < nr_cpu_ids)
 				return cpu;
 
-			return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
+			if (likely(cpu < nr_cpu_ids))
+				return cpu;
+			/*
+			 * Unless we have another problem this can only happen
+			 * at boot time before start_secondary() brings the 1st
+			 * housekeeping CPU up.
+			 */
+			WARN_ON_ONCE(system_state == SYSTEM_RUNNING ||
+				     type != HK_TYPE_TIMER);
 		}
 	}
 	return smp_processor_id();

Frederic Weisbecker

maintainer

2024-10-04 12:37 UTC #37 lore raw

In reply to Leonardo Bras

Hi,

Quoted 35 lines · Leonardo Bras

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 <[email protected]>
---
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(-)
kernel/workqueue.c +0 -0
index 76e60faed8923..8dd7c01b326a4 100644

kernel/workqueue.c static void __queue_delayed_work(int cpu, struct workqueue_struct *wq, · line 1965

Quoted 9 lines

kernel/workqueue.c +5 -1

@@ -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() already picks up a housekeeping CPU. So why is it needed?

Thanks.

Quoted 17 lines

kernel/workqueue.c +6 -2

 		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);
+	}
 
 }

/**

Signature · 1 line

2.43.0

Leonardo Bras Soares Passos

author

2024-10-04 22:43 UTC #38 lore raw

Hi,

Quoted 42 lines · Frederic Weisbecker

Hi,
Quoted 38 lines · Leonardo Bras
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 <[email protected]>
---
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(-)
kernel/workqueue.c +0 -0
index 76e60faed8923..8dd7c01b326a4 100644
@@ -1958,10 +1958,18 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
      dwork->cpu = cpu;
      timer->expires = jiffies + delay;
 

Quoted 9 lines

kernel/workqueue.c static void __queue_delayed_work(int cpu, struct workqueue_struct *wq, · line 1965
Quoted 6 lines
kernel/workqueue.c +5 -1
-     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() already picks up a housekeeping CPU. So why is it needed?

Well, back when it was merged, I could see add_timer() using an isolated CPU, and that's why I sent this change.
Did this change recently?

Also, previously add_timer() was only called if (cpu == WORK_CPU_UNBOUND), and now we make sure that for any value of 'cpu' the timer to be put in a housekeeping cpu.

Thanks!
Leo

Quoted 20 lines

Thanks.

Quoted 17 lines

kernel/workqueue.c +6 -2

              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

Frederic Weisbecker

maintainer

2024-10-05 11:15 UTC #39 lore raw

Quoted 3 lines · Leonardo Bras Soares Passos

Well, back when it was merged, I could see add_timer() using an isolated CPU, and that's why I sent this change.
Did this change recently?

That was (and still is) not supposed to happen because get_nohz_timer_target() forbids that. Since then we have merged tmigr that also takes care of it.

So something looks broken...unless you had kernel.timer_migration=0 ?

Quoted 4 lines

Also, previously add_timer() was only called if (cpu == WORK_CPU_UNBOUND), and now we make sure that for any value of 'cpu' the timer to be put in a housekeeping cpu.

I suppose it makes the situation slightly less worse when a work is bound to an isolated CPU. You get the workqueue without the timer. But the context switch/workqueue is the most unwanted part :-)

Thanks.

Quoted 25 lines

Thanks!
Leo

Quoted 20 lines

Thanks.

Quoted 17 lines

              add_timer_on(timer, cpu);

kernel/workqueue.c +6 -3

-     else
-             add_timer(timer);
+     } else {
+             if (likely(cpu == WORK_CPU_UNBOUND))
+                     add_timer(timer);
+             else
+                     add_timer_on(timer, cpu);
+     }
 
 }
 
 /**
--

2.43.0