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

[Kayra Cizmeci <kayracizmeci@gmail.com>]

Hi!

On Wed, Nov 11, 2026 at 02:40:36PM +0530, K Prateek Nayak wrote:
> 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.
> 

Thanks for testing! Well, I missed that :-(. Core Scheduling...

> > 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 :-)
> 

TBH, I don't know which one is better. And if I were to decide,
I'd be wrong 50% of the time, so it's up to Peter :-).

> >  	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 for having a look at this!

> 
> -- 
> Thanks and Regards,
> Prateek
> 

Cheers,

Kayra