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

[bot+bpf-ci@kernel.org]

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

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

TL;DR: Commit message only: claims the new is_idle_task() gate is "strictly
wider", but it also now covers idle-injection kthreads (PF_IDLE) and not some
early-boot cases; the changelog should say so.

>     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. A cgroup holding one such task was charged twice the CPU time it
>     could possibly have used; numbers are in the cover letter.
>
>     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 new gate is strictly wider than the old one, and deliberately so.
>     idle_cpu() is idle_rq(), rq->curr == rq->idle && !rq->nr_running &&
>     !rq->ttwu_pending, so besides the two cases above it also skipped a CPU
>     idling with a wakeup queued by TTWU_QUEUE whose IPI has not run yet.
>     That case is now covered too, on any kernel and not just under
>     sched_ext. It is the same latch failure for a shorter window, so
>     covering it looks right rather than merely harmless, but it was reached
>     by reading the code and not measured on its own. What keeps the wider
>     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.
>
>     Tooling: the bug was found and this patch and changelog were written by
>     Claude Opus 5 (Claude Code), in a session measuring cgroup CPU limit
>     enforcement under sched_ext. All of the patch and all of the changelog
>     are tool-generated; the cover letter says what was and was not measured.
>     By the threat model in Documentation/process/threat-model.rst this is a
>     regular bug and not a vulnerability: it misreports CPU time under a
>     legitimate configuration and crosses no privilege or isolation boundary.
>
>     Fixes: f0e1a0643a59 ("sched_ext: Implement BPF extensible scheduler class")
>     Signed-off-by: Radu Toader <radu.m.toader@gmail.com>
>

The commit message says "The new gate is strictly wider than the old one,
and deliberately so", and lists the newly covered cases as nr_running > 0
while the idle task runs, and a queued TTWU_QUEUE wakeup. Is that accurate?

is_idle_task() tests PF_IDLE on current, which is a different predicate
from rq->curr == rq->idle.  play_idle_precise() sets PF_IDLE on a
SCHED_FIFO kthread and then loops on do_idle():

kernel/sched/idle.c:play_idle_precise() {
    ...
	current->flags |= PF_IDLE;
    ...
	while (!READ_ONCE(it.done))
		do_idle();
    ...
}

During idle injection rq->curr is that kthread, so the old gate was false
(except on nohz_full CPUs), while the new gate is true whenever
need_resched() is clear.  Interrupts during injected idle will now run
tick_nohz_irq_exit() with TS_FLAG_INIDLE set, which does
tick_nohz_clock_sleep(), updates ts->idle_entrytime, and may start kcpustat
idle accounting.  This affects users such as intel_powerclamp,
cpuidle_cooling and idle_inject, and is not limited to sched_ext.

Peter Zijlstra raised the same point on the earlier posting: "So
is_idle_task() is also true for idle-injection tasks. I *think* that is
also right in this case, but it wasn't mentioned."  No reply or changelog
update addressing it is visible.

Conversely, PF_IDLE is set by cpu_startup_entry(), not by init_idle(), so
a CPU running its idle task before that point (rq->curr == rq->idle,
nr_running == 0) satisfied idle_cpu() but does not satisfy
is_idle_task(current).  That looks harmless, since TS_FLAG_INIDLE is only
set inside do_idle() and nohz_full CPUs are still covered by
tick_nohz_full_cpu(cpu), but it means the gate is not strictly wider.

Could the commit message name the idle injection case among the newly
covered states, and replace "strictly wider" with wording that matches the
PF_IDLE semantics?


---
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/42280840441