Re: [PATCH v2] softirq: Treat an interrupted idle task as idle in tick_irq_exit()

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 5638 bytes --]

> softirq: Treat an interrupted idle task as idle in tick_irq_exit()

TL;DR: Commit message only, no code issues: the changelog carries posting
notes (v1 history, dropped reproducer, compile-test matrix) that could move
below the '---' line.

> softirq: Treat an interrupted idle task as idle in tick_irq_exit()
>
> tick_irq_exit() only runs tick_nohz_irq_exit() if sched_core_idle_cpu(),
> which is idle_cpu() unless core scheduling is built in. idle_cpu() is
> false when the run queue has tasks while the idle task runs.
>
> That is the normal state of a CPU under sched_ext whose BPF scheduler
> holds back a runnable task of that CPU in one of its queues, e.g. a task
> of a cgroup at its CPU limit: the task stays queued on the CPU's run
> queue and counts in rq->nr_running, while the CPU sits in the idle loop.
>
> tick_nohz_irq_exit() is what marks an idle CPU as idle again for the
> scheduler clock after an interrupt in the idle loop
> (TS_FLAG_IDLE_ACTIVE, sched_clock_idle_sleep_event()). Without it, the
> next exit from the idle loop doesn't resynchronize the CPU's sched_clock
> with sched_clock_idle_wakeup_event(). With an unstable sched_clock, e.g.
> under kvm-clock without a stable TSC, the clock of a CPU in tick-stopped
> idle is clamped to its last stamp plus a tick, so a task picked after a
> reschedule IPI (which doesn't go through irq_enter()) starts with a
> stale exec_start, and the next tick charges the whole idle stretch to
> it.
>
> Measured with one thread spinning 10 ms in every 100 ms in a cgroup,
> kept off its CPU for a further 7 to 53 ms per period by a sched_ext
> scheduler: the cgroup's cpu.stat usage_usec over the thread's own
> CLOCK_MONOTONIC measurement of its busy time reached 2.00, twice the
> CPU time the thread could possibly have used, and is 0.96 with this
> patch. The same ratio with no BPF scheduler loaded is 1.005.
>
> Commit 548796e2e70b ("sched/core: introduce sched_core_idle_cpu()")
> fixed this same gate for the other case of "the idle task runs while
> nr_running > 0", forced idle under core scheduling, where the symptom
> was too little idle time in /proc/stat. sched_ext made that state
> reachable without CONFIG_SCHED_CORE, and there the symptom is idle time
> charged to a task as runtime.
>
> Ask whether the idle task is running, which covers both cases. CPUs
> whose run queue is really empty are unaffected; a CPU interrupted
> between leaving the idle loop and switching to a task has need_resched()
> set or has already left the idle accounting, see tick_nohz_irq_exit().
> sched_core_idle_cpu() has no users left and goes.
>
> The two gates test different predicates rather than one containing the
> other, and the difference is deliberate. idle_cpu() is idle_rq():
> rq->curr == rq->idle && !rq->nr_running && !rq->ttwu_pending.
> is_idle_task() tests PF_IDLE on current. Newly covered:
>
>  - the two "the idle task runs while nr_running > 0" cases above;
>
>  - a CPU idling with a wakeup queued by TTWU_QUEUE whose IPI has not
>    run yet;
>
>  - injected idle. play_idle_precise() sets PF_IDLE on a SCHED_FIFO
>    kthread which then loops on do_idle(), so rq->curr is not rq->idle
>    there and the old gate was false off nohz_full CPUs. Interrupts
>    taken during injected idle now run tick_nohz_irq_exit() with
>    TS_FLAG_INIDLE set. That CPU is running an idle state and needs the
>    same latch for the same reason, so this is wanted rather than merely
>    harmless, but note it reaches the idle injection users in
>    drivers/powercap/idle_inject.c, intel_powerclamp and
>    cpuidle_cooling, and is not confined to sched_ext.
>
> One state is no longer covered: PF_IDLE is set by cpu_startup_entry()
> and not by init_idle(), so a CPU on its idle task before that point
> satisfied idle_cpu() but does not satisfy is_idle_task(current). Nothing
> is lost, because TS_FLAG_INIDLE is only set inside do_idle(), so
> tick_nohz_irq_exit() had nothing to do there, and nohz_full CPUs are
> covered by tick_nohz_full_cpu(cpu) either way.
>
> What keeps the new gate from giving a CPU idle accounting it should not
> have is that tick_nohz_irq_exit() tests TS_FLAG_INIDLE itself and only
> does the idle bookkeeping inside the idle loop.
>
> Compile-tested in four x86_64 defconfig variants - SCHED_CORE=y with
> NO_HZ_FULL=y, SCHED_CORE=n, NO_HZ_COMMON=n, and SCHED_CLASS_EXT=y with
> SCHED_CORE=y and NO_HZ_FULL=y - with no new warnings at W=1.
>
> The reproducer is a sched_ext scheduler that parks a tagged task on an
> otherwise idle CPU for a set time. It was patch 1 of v1 and is dropped
> from this posting at the maintainer's request; it remains in the list
> archive, see Link.
>
> [ ... ]
>
> Fixes: f0e1a0643a59 ("sched_ext: Implement BPF extensible scheduler class")
> Link: https://lore.kernel.org/lkml/20261106094343.174905-2-radu.m.toader@gmail.com/
> Signed-off-by: Radu Toader <radu.m.toader@gmail.com>

This isn't a bug, but the changelog carries notes about the posting itself:

> The reproducer is a sched_ext scheduler that parks a tagged task on an
> otherwise idle CPU for a set time. It was patch 1 of v1 and is dropped
> from this posting at the maintainer's request; it remains in the list
> archive, see Link.

Could the notes about v1 and this posting (and perhaps the compile-test
matrix) move below the '---' line, leaving the Link: tag to point at the
reproducer?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42982351680