Re: [PATCH v2 05/12] sched/core: Abstract/fix rq_clock_start_loop_update()

[K Prateek Nayak <kprateek.nayak@amd.com>]

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