Re: [PATCH v3] sched/core: Preserve wake flags across queued wakeups

[Shubhang]

Hi Prateek,

On Wed, 22 Jul 2026, K Prateek Nayak wrote:

> Hello Shubhang,
>
> On 7/22/2026 12:24 PM, Shubhang Kaushik (Ampere) wrote:
> > Changes in v3:
> >   - Use READ_ONCE()/WRITE_ONCE() for sched_remote_wakeup_flags.
>
> Why? Adding to the llist will have release semantics and I don't think
> there is any way for the receiver to see an object on the llist and it
> not having "sched_remote_wakeup_flags" set properly.
>
> Otherwise, a bunch of csd initialization bits would also need
> READ_ONCE() / WRITE_ONCE() but I don't see any.

Thanks, that makes sense.

I added READ_ONCE()/WRITE_ONCE() because the field is written by the
waking CPU and read by the target CPU, so I initially treated it as an
explicit cross-CPU field. But the flags are stored before the task is
published with llist_add(), and the target CPU reads them only after
removing the entry from the llist. So yeah, will drop that.

> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -983,6 +983,16 @@ struct task_struct {
> >
> >         /* Used for emulating ABI behavior of previous Linux versions: */
> >         unsigned int                    personality;
> > +       /*
> > +        * Must not share the scheduler bitfield word because wakelist
> > +        * queueing is not serialized by p->on_cpu.
> > +        *
> > +        * smp_load_acquire(&p->on_cpu) in ttwu_queue_wakelist() pairs
>
> This should be "before" and not "in" here ^^
>
> > +        * with smp_mb__after_spinlock() in schedule(), making current's
>
> What? That load acquire pairs with smp_store_release() in finish_task()
> before interrupts are enabled after context-switch on the destination
> CPU.

Agreed, will fix this comment.

> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 96226707c2f6135341aa779b8262f113e103d8ad..7ddcc6a7f1c06675ce8084cd304475d61f869eb7 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -3902,13 +3902,16 @@ void sched_ttwu_pending(void *arg)
> >         update_rq_clock(rq);
> >
> >         llist_for_each_entry_safe(p, t, llist, wake_entry.llist) {
> > +               int wake_flags;
>
> Do we really need that local variable?

No, once READ_ONCE() goes away this can be passed directly to
ttwu_do_activate(). I'll drop the local variable as well.

> > @@ -3947,11 +3950,14 @@ bool call_function_single_prep_ipi(int cpu)
> >   * via sched_ttwu_wakeup() for activation so the wakee incurs the cost
> >   * of the wakeup instead of the waker.
> >   */
> > +#define WF_TTWU_QUEUE_MASK     (WF_TTWU | WF_SYNC | WF_MIGRATED | \
> > +                                WF_RQ_SELECTED)
>
> I much rather prefer it in single line even though it crosses
> 80 characters. #defines like SCHED_DL_FLAGS run even longer.

Sure, will keep it in one line.

> >  static void __ttwu_queue_wakelist(struct task_struct *p, int cpu, int wake_flags)
> >  {
> >         struct rq *rq = cpu_rq(cpu);
> >
> > -       p->sched_remote_wakeup = !!(wake_flags & WF_MIGRATED);
> > +       WRITE_ONCE(p->sched_remote_wakeup_flags,
> > +                  wake_flags & WF_TTWU_QUEUE_MASK);
> >
> >         WRITE_ONCE(rq->ttwu_pending, 1);
> >  #ifdef CONFIG_SMP
>
> FWIW, feel free to include:
>
> Tested-by: K Prateek Nayak <kprateek.nayak@xxxxxxx>

Thanks, I'll add it.

Regards,
Shubhang Kaushik