mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Frederic Weisbecker <frederic@kernel.org>
To: Radu Toader <radu.m.toader@gmail.com>
Cc: peterz@infradead.org, mingo@redhat.com, tglx@kernel.org,
	tglx@linutronix.de, tj@kernel.org, juri.lelli@redhat.com,
	vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
	rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
	vschneid@redhat.com, kprateek.nayak@amd.com, void@manifault.com,
	arighi@nvidia.com, changwoo@igalia.com, anna-maria@linutronix.de,
	bigeasy@linutronix.de, joshdon@google.com, haoluo@google.com,
	brho@google.com, boqun@kernel.org, joelagnelf@nvidia.com,
	enlin.mu@linux.dev, brads@mainlining.org, kmehltretter@gmail.com,
	sched-ext@lists.linux.dev, bpf@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] softirq: Treat an interrupted idle task as idle in tick_irq_exit()
Date: Wed, 11 Nov 2026 17:10:54 +0100	[thread overview]
Message-ID: <avSTjihu0YXfFNUV@localhost.localdomain> (raw)
In-Reply-To: <20261111112944.3868822-1-radu.m.toader@gmail.com>

Le Wed, Nov 11, 2026 at 11:29:44AM +0000, Radu Toader a écrit :
> 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.
> 
> Tooling: the bug was found and this patch and changelog were written by
> Claude Opus 5 (Claude Code)

About the changelog, I could recognize given the overzealous details and
labyrinthic reasoning :-)

Still it looks correct.

> in a session measuring cgroup CPU limit
> enforcement under sched_ext. All of the patch and all of the changelog
> are tool-generated. The measurements above were taken on hosts that no
> longer exist and were not repeated for this posting. 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")
> Link: https://lore.kernel.org/lkml/20261106094343.174905-2-radu.m.toader@gmail.com/
> Signed-off-by: Radu Toader <radu.m.toader@gmail.com>
> ---
> Changes in v3:
>  - No code change; the diff is identical to v2.
>  - Moved the posting notes out of the changelog and below this line, as
>    asked in review of v2. The Link: tag still points at the reproducer.
>  - The Tooling: line stays in the changelog above the --- line, because
>    Documentation/process/generated-content.rst asks for tool use to be
>    noted there.
> 
> 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 the Link: tag above.
> 
> 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.
> 
> v1: https://lore.kernel.org/lkml/20261106094343.174905-1-radu.m.toader@gmail.com/
> v2 and its CI run: https://patchwork.kernel.org/series/1206104/
> 
>  include/linux/sched.h   |  2 --
>  kernel/sched/syscalls.c | 12 ------------
>  kernel/softirq.c        | 12 ++++++++++--
>  3 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 1c93da75226e..0e704ea9416f 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2326,11 +2326,9 @@ extern void sched_core_free(struct task_struct *tsk);
>  extern void sched_core_fork(struct task_struct *p);
>  extern int sched_core_share_pid(unsigned int cmd, pid_t pid, enum pid_type type,
>  				unsigned long uaddr);
> -extern int sched_core_idle_cpu(int cpu);
>  #else
>  static inline void sched_core_free(struct task_struct *tsk) { }
>  static inline void sched_core_fork(struct task_struct *p) { }
> -static inline int sched_core_idle_cpu(int cpu) { return idle_cpu(cpu); }
>  #endif
>  
>  extern void sched_set_stop_task(int cpu, struct task_struct *stop);
> diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
> index 45a127deeadd..22e316680a86 100644
> --- a/kernel/sched/syscalls.c
> +++ b/kernel/sched/syscalls.c
> @@ -194,18 +194,6 @@ struct task_struct *idle_task(int cpu)
>  	return cpu_rq(cpu)->idle;
>  }
>  
> -#ifdef CONFIG_SCHED_CORE
> -int sched_core_idle_cpu(int cpu)
> -{
> -	struct rq *rq = cpu_rq(cpu);
> -
> -	if (sched_core_enabled(rq) && rq->curr == rq->idle)
> -		return 1;
> -
> -	return idle_cpu(cpu);
> -}
> -#endif /* CONFIG_SCHED_CORE */
> -
>  /**
>   * find_process_by_pid - find a process with a matching PID value.
>   * @pid: the pid in question.
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 7fc2200392cd..e7d649c29c0c 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -738,8 +738,16 @@ static inline void tick_irq_exit(void)
>  #ifdef CONFIG_NO_HZ_COMMON
>  	int cpu = smp_processor_id();
>  
> -	/* Make sure that timer wheel updates are propagated */
> -	if ((sched_core_idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) {
> +	/*
> +	 * Make sure that timer wheel updates are propagated. An interrupted
> +	 * idle loop counts as idle even with runnable tasks queued on the
> +	 * CPU: forced idle under core scheduling, and the tasks sched_ext
> +	 * keeps on a CPU's run queue while its BPF scheduler holds them back.
> +	 * idle_cpu() would skip the idle bookkeeping of tick_nohz_irq_exit()
> +	 * for those, and with an unstable sched_clock the CPU's clock then
> +	 * isn't resynchronized when it leaves the idle loop.
> +	 */
> +	if ((is_idle_task(current) && !need_resched()) || tick_nohz_full_cpu(cpu)) {
>  		if (hardirq_count() == HARDIRQ_OFFSET)
>  			tick_nohz_irq_exit();

Works for me, but Peter might find something I haven't.

Acked-by: Frederic Weisbecker <frederic@kernel.org>


>  	}
> 
> base-commit: f66469d9905b39d2c260a15d7d6db450f4db67c1
> -- 
> 2.43.0
> 

-- 
Frederic Weisbecker
SUSE Labs

  reply	other threads:[~2026-11-11 16:10 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-11-11 11:29 Radu Toader
2026-11-11 16:10 ` Frederic Weisbecker [this message]
2026-11-11 16:31 Bradley Morgan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=avSTjihu0YXfFNUV@localhost.localdomain \
    --to=frederic@kernel.org \
    --cc=anna-maria@linutronix.de \
    --cc=arighi@nvidia.com \
    --cc=bigeasy@linutronix.de \
    --cc=boqun@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brads@mainlining.org \
    --cc=brho@google.com \
    --cc=bsegall@google.com \
    --cc=changwoo@igalia.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=enlin.mu@linux.dev \
    --cc=haoluo@google.com \
    --cc=joelagnelf@nvidia.com \
    --cc=joshdon@google.com \
    --cc=juri.lelli@redhat.com \
    --cc=kmehltretter@gmail.com \
    --cc=kprateek.nayak@amd.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mgorman@suse.de \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=radu.m.toader@gmail.com \
    --cc=rostedt@goodmis.org \
    --cc=sched-ext@lists.linux.dev \
    --cc=tglx@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=tj@kernel.org \
    --cc=vincent.guittot@linaro.org \
    --cc=void@manifault.com \
    --cc=vschneid@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®