| Message ID | 20260116145208.87445-2-frederic@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | tick/sched: Refactor idle cputime accounting | expand |
On Fri, Jan 16, 2026 at 03:51:54PM +0100, Frederic Weisbecker wrote: > kernel/sched/idle.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c > index c174afe1dd17..35d79af3286d 100644 > --- a/kernel/sched/idle.c > +++ b/kernel/sched/idle.c > @@ -260,6 +260,12 @@ static void do_idle(void) > { > int cpu = smp_processor_id(); > > + if (cpu_is_offline(cpu)) { Does it make sense to make that: if (unlikely(cpu_is_offline(cpu))) ? > + local_irq_disable(); Also, do we want something like: WARN_ON_ONCE(need_resched()); ? > + cpuhp_report_idle_dead(); > + arch_cpu_idle_dead(); > + } > + > /* > * Check if we need to update blocked load > */ > @@ -311,11 +317,6 @@ static void do_idle(void) > */ > local_irq_disable(); > > - if (cpu_is_offline(cpu)) { > - cpuhp_report_idle_dead(); > - arch_cpu_idle_dead(); > - } > - > arch_cpu_idle_enter(); > rcu_nocb_flush_deferred_wakeup(); > > -- > 2.51.1 >
Le Mon, Jan 19, 2026 at 01:53:47PM +0100, Peter Zijlstra a écrit : > On Fri, Jan 16, 2026 at 03:51:54PM +0100, Frederic Weisbecker wrote: > > > kernel/sched/idle.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c > > index c174afe1dd17..35d79af3286d 100644 > > --- a/kernel/sched/idle.c > > +++ b/kernel/sched/idle.c > > @@ -260,6 +260,12 @@ static void do_idle(void) > > { > > int cpu = smp_processor_id(); > > > > + if (cpu_is_offline(cpu)) { > > Does it make sense to make that: if (unlikely(cpu_is_offline(cpu))) ? Yes indeed! > > > + local_irq_disable(); > > Also, do we want something like: > > WARN_ON_ONCE(need_resched()); > > ? Definetly. Thanks.
Hello Frederic, Peter, On 1/20/2026 2:34 AM, Frederic Weisbecker wrote: > Le Mon, Jan 19, 2026 at 01:53:47PM +0100, Peter Zijlstra a écrit : >> On Fri, Jan 16, 2026 at 03:51:54PM +0100, Frederic Weisbecker wrote: >> >>> kernel/sched/idle.c | 11 ++++++----- >>> 1 file changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c >>> index c174afe1dd17..35d79af3286d 100644 >>> --- a/kernel/sched/idle.c >>> +++ b/kernel/sched/idle.c >>> @@ -260,6 +260,12 @@ static void do_idle(void) >>> { >>> int cpu = smp_processor_id(); >>> >>> + if (cpu_is_offline(cpu)) { >> >> Does it make sense to make that: if (unlikely(cpu_is_offline(cpu))) ? > > Yes indeed! nit. but don't we inherit it from: #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu)) so it will end up being annotated with unlikely() no?
Le Tue, Jan 20, 2026 at 09:56:12AM +0530, K Prateek Nayak a écrit : > Hello Frederic, Peter, > > On 1/20/2026 2:34 AM, Frederic Weisbecker wrote: > > Le Mon, Jan 19, 2026 at 01:53:47PM +0100, Peter Zijlstra a écrit : > >> On Fri, Jan 16, 2026 at 03:51:54PM +0100, Frederic Weisbecker wrote: > >> > >>> kernel/sched/idle.c | 11 ++++++----- > >>> 1 file changed, 6 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c > >>> index c174afe1dd17..35d79af3286d 100644 > >>> --- a/kernel/sched/idle.c > >>> +++ b/kernel/sched/idle.c > >>> @@ -260,6 +260,12 @@ static void do_idle(void) > >>> { > >>> int cpu = smp_processor_id(); > >>> > >>> + if (cpu_is_offline(cpu)) { > >> > >> Does it make sense to make that: if (unlikely(cpu_is_offline(cpu))) ? > > > > Yes indeed! > > nit. but don't we inherit it from: > > #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu)) > > so it will end up being annotated with unlikely() no? Ah right! > > -- > Thanks and Regards, > Prateek >
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c index c174afe1dd17..35d79af3286d 100644 --- a/kernel/sched/idle.c +++ b/kernel/sched/idle.c @@ -260,6 +260,12 @@ static void do_idle(void) { int cpu = smp_processor_id(); + if (cpu_is_offline(cpu)) { + local_irq_disable(); + cpuhp_report_idle_dead(); + arch_cpu_idle_dead(); + } + /* * Check if we need to update blocked load */ @@ -311,11 +317,6 @@ static void do_idle(void) */ local_irq_disable(); - if (cpu_is_offline(cpu)) { - cpuhp_report_idle_dead(); - arch_cpu_idle_dead(); - } - arch_cpu_idle_enter(); rcu_nocb_flush_deferred_wakeup();
Offline handling happens from within the inner idle loop, after the beginning of dyntick cputime accounting, nohz idle load balancing and TIF_NEED_RESCHED polling. This is not necessary and even buggy because: * There is no dyntick handling to do. And calling tick_nohz_idle_enter() messes up with the struct tick_sched reset that was performed on tick_sched_timer_dying(). * There is no nohz idle balancing to do. * Polling on TIF_RESCHED is irrelevant at this stage, there are no more tasks allowed to run. * No need to check if need_resched() before offline handling since stop_machine is done and all per-cpu kthread should be done with their job. Therefore move the offline handling at the beginning of the idle loop. This will also ease the idle cputime unification later by not elapsing idle time while offline through the call to: tick_nohz_idle_enter() -> tick_nohz_start_idle() Signed-off-by: Frederic Weisbecker <frederic@kernel.org> --- kernel/sched/idle.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)