Messages in this thread Patch in this message | | | Date | Fri, 5 Apr 2024 16:04:49 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on queue_delayed_work |
| |
On 04/03, Oleg Nesterov wrote: > > > > OTOH, Documentation/timers/no_hz.rst says > > > > > > Therefore, the > > > boot CPU is prohibited from entering adaptive-ticks mode. Specifying a > > > "nohz_full=" mask that includes the boot CPU will result in a boot-time > > > error message, and the boot CPU will be removed from the mask. > > > > > > and this doesn't match the reality. > > > > Don't some archs allow the boot CPU to go down too tho? If so, this doesn't > > really solve the problem, right? > > 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.
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.
> > > So it seems that we should fix housekeeping_setup() ? see the patch below. > > > > > > In any case the usage of cpu_present_mask doesn't look right to me. > > > > > > Oleg. > > > > > > --- a/kernel/sched/isolation.c > > > +++ b/kernel/sched/isolation.c > > > @@ -129,7 +154,7 @@ static int __init housekeeping_setup(char *str, unsigned long flags) > > > cpumask_andnot(housekeeping_staging, > > > cpu_possible_mask, non_housekeeping_mask); > > > > > > - if (!cpumask_intersects(cpu_present_mask, housekeeping_staging)) { > > > + if (!cpumask_test_cpu(smp_processor_id(), housekeeping_staging)) { > > > __cpumask_set_cpu(smp_processor_id(), housekeeping_staging); > > > __cpumask_clear_cpu(smp_processor_id(), non_housekeeping_mask); > > > if (!housekeeping.flags) { > > >
| |