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

[Shubhang <sh@gentwo.org>]

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@amd.com>

Thanks, I'll add it.

Regards,
Shubhang Kaushik