Re: [PATCH v2 05/12] sched/core: Abstract/fix rq_clock_start_loop_update()
From: K Prateek Nayak
Date: Wed Nov 11 2026 - 04:25:18 EST
Hello Kayre, Peter,
On 11/9/2026 3:56 AM, Kayra Cizmeci wrote:
> So to fix both of these I would suggest something like so, not tested:
>
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 339a77864ec8..a91d12640f37 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1911,7 +1911,7 @@ static inline unsigned int rq_clock_freeze(struct rq *rq)
> return flags;
> }
>
> -static inline void rq_clock_thaw(struct rq *rq, bool flags)
> +static inline void rq_clock_thaw(struct rq *rq, unsigned int flags)
> {
> lockdep_assert_rq_held(rq);
> rq->clock_update_flags &= ~RQCF_FREEZE;
> @@ -3357,8 +3357,8 @@ unsigned long arch_scale_freq_capacity(int cpu)
> */
> static inline void double_rq_clock_clear_update(struct rq *rq1, struct rq *rq2)
> {
> - rq1->clock_update_flags &= (RQCF_REQ_SKIP|RQCF_ACT_SKIP);
> - rq2->clock_update_flags &= (RQCF_REQ_SKIP|RQCF_ACT_SKIP);
> + rq1->clock_update_flags &= ~RQCF_UPDATED;
> + rq2->clock_update_flags &= ~RQCF_UPDATED;
> }
>
> #define DEFINE_LOCK_GUARD_2(name, type, _lock, _unlock, ...) \
So with this, I still get a clock_update_flags warning like:
------------[ cut here ]------------
!(rq->clock_update_flags & RQCF_UPDATED)
WARNING: kernel/sched/sched.h:1862 at set_next_task_idle+0xe1/0x100, CPU#262: sched-messaging/8375
Modules linked in: ...
CPU: 262 UID: 0 PID: 8375 Comm: sched-messaging Tainted: G W 7.4.0-rc1-test+ #463 PREEMPT(lazy)
Tainted: [W]=WARN
Hardware name: AMD Corporation Titanite_4G/Titanite_4G, BIOS RTI100CC 03/28/2024
RIP: 0010:set_next_task_idle+0xe1/0x100
Code: ...
RSP: 0018:ff919f922f4e7c80 EFLAGS: 00010046
RAX: 0000000000000000 RBX: ff49b96ece1b33c0 RCX: 0000000000000007
RDX: 000000128e40504c RSI: 0000000000000200 RDI: ff49b8b30f2d8000
RBP: ff49b8b28ed84280 R08: 0000000000000200 R09: ff49b8b28ed84280
R10: 0000000000000008 R11: 0000000000000008 R12: ff49b8b28ed84280
R13: ffffffff86a043c0 R14: ff49b8b28dfca700 R15: 0000000000000200
FS: 00007fddd81237c0(0000) GS:ff49b96f477af000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055ce59162840 CR3: 00000001a1f11004 CR4: 0000000000f71ef0
PKRU: 55555554
Call Trace:
<TASK>
__schedule+0x93b/0x1ca0
schedule+0x28/0xa0
anon_pipe_read+0x302/0x410
? __pfx_autoremove_wake_function+0x10/0x10
vfs_read+0x30b/0x340
ksys_read+0xb8/0xe0
do_syscall_64+0xce/0x550
? srso_alias_return_thunk+0x5/0xfbef5
? srso_alias_return_thunk+0x5/0xfbef5
? do_syscall_64+0x35/0x550
entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x7fddd6514932
Code: ...
RSP: 002b:00007fffd6dcde28 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fddd6514932
RDX: 0000000000000064 RSI: 00007fffd6dcde40 RDI: 0000000000000007
RBP: 00007fffd6dcdee0 R08: 0000000000000000 R09: 00007fddd81237c0
R10: 00007fddd84ed0c8 R11: 0000000000000246 R12: 0000000000000064
R13: 00007fffd6dcde40 R14: 00000000000001c7 R15: 000055ce88d5e840
</TASK>
---[ end trace 0000000000000000 ]---
with:
coresched new -t pid -- perf bench sched messaging -p -l 100000 -g 8
on queue:sched/hackery.
If I do:
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 339a77864ec8..fd900d1419c6 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -3357,8 +3357,8 @@ unsigned long arch_scale_freq_capacity(int cpu)
*/
static inline void double_rq_clock_clear_update(struct rq *rq1, struct rq *rq2)
{
- rq1->clock_update_flags &= (RQCF_REQ_SKIP|RQCF_ACT_SKIP);
- rq2->clock_update_flags &= (RQCF_REQ_SKIP|RQCF_ACT_SKIP);
+ __rq_clock_clear(rq1);
+ __rq_clock_clear(rq2);
}
#define DEFINE_LOCK_GUARD_2(name, type, _lock, _unlock, ...) \
---
I don't see that warning anymore. Must be the "core_pick_in_flight"
check keeping everything sane.
> In a case like this:
>
> CPU1 calls rq_drop_lock(), saves FREEZE = 0, and changes FREEZE to 1,
> and drops the lock.
>
> CPU2 gets the lock, saves FREEZE = 1 and changes FREEZE to 1, and then
> drops the lock.
>
> In this period CPU1 gets the lock again, makes FREEZE 0.
>
> And later CPU2 comes and makes it FREEZE 1.
>
>
> Not sure above could happen tho. Could it happen?
I suppose it is possible.
Generally rq_lock is dropped in middle of __schedule() when NEED_RESCHED is still
set so ->task_woken() would generally bail out early for dl and rt (ext doesn't
seem to drop the rq_lock on that path).
That said, a concurrent wakeup can grab the rq_lock when another ->task_woken()
dropped the rq_lock or a __balance_callbacks() for setprio or sched_move_task()
can interleave with a ->task_woken() and lead to your scenario.
> I tried to fix this assuming it could happen.
>
> I'm not really happy with below diff, but I couldn't find a better way
> neither, so please tell me that this case can't happen or there
> is a better way to fix it :-(. This is on top of the current code BTW.
> Only compile tested.
[..snip..]
> bool cfs_task_bw_constrained(struct task_struct *p)
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 339a77864ec8..d8f1784d0640 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1281,6 +1281,7 @@ struct rq {
> u64 clock;
> unsigned long lost_idle_time;
> unsigned int clock_update_flags;
> + unsigned int clock_update_depth;
I think this can stand-in for the freeze flag too if you do
something like:
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 339a77864ec8..49c732d3f3bb 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1280,7 +1280,18 @@ struct rq {
u64 clock_pelt;
u64 clock;
unsigned long lost_idle_time;
- unsigned int clock_update_flags;
+ union {
+ struct {
+ s16 clock_update_flags;
+ u16 freeze_depth;
+ };
+ /*
+ * Inspect both clock_update_flags and
+ * freeze_depth together in
+ * __update_rq_clock().
+ */
+ u32 __clock_update_flags;
+ };
u64 clock_pelt_idle;
u64 clock_idle;
---
Probably makes it very ugly :-)
> u64 clock_pelt_idle;
> u64 clock_idle;
>
> @@ -1902,31 +1903,28 @@ static inline void rq_clock_cancel_skipupdate(struct rq *rq)
> * update_rq_clock() for several cfs and rt runqueues (Typically when using
> * list_for_each_entry_*)
> */
> -static inline unsigned int rq_clock_freeze(struct rq *rq)
> +static inline void rq_clock_freeze(struct rq *rq)
> {
> - unsigned int flags = rq->clock_update_flags;
> lockdep_assert_rq_held(rq);
> assert_clock_updated(rq);
> + rq->clock_update_depth++;
> rq->clock_update_flags |= RQCF_FREEZE;
> - return flags;
> }
>
> -static inline void rq_clock_thaw(struct rq *rq, bool flags)
> +static inline void rq_clock_thaw(struct rq *rq)
> {
> lockdep_assert_rq_held(rq);
> + WARN_ON_ONCE(!rq->clock_update_depth);
> + rq->clock_update_flags |= RQCF_UPDATED;
> + rq->clock_update_depth--;
> + if (rq->clock_update_depth)
> + return;
> rq->clock_update_flags &= ~RQCF_FREEZE;
> - rq->clock_update_flags |= RQCF_UPDATED | (flags & RQCF_FREEZE);
> }
This seems like a legit problem one can run into and
your solutions works well!
--
Thanks and Regards,
Prateek