On 17/11/20 16:13, Peter Zijlstra wrote:
> On Tue, Nov 17, 2020 at 03:37:24PM +0000, Valentin Schneider wrote:
>
>> >> + /*
>> >> + * This field must not be in the scheduler word above due to wakelist
>> >> + * queueing no longer being serialized by p->on_cpu. However:
>> >> + *
>> >> + * p->XXX = X; ttwu()
>> >> + * schedule() if (p->on_rq && ..) // false
>> >> + * smp_mb__after_spinlock(); if (smp_load_acquire(&p->on_cpu) &&
>> >> //true
>> >> + * deactivate_task() ttwu_queue_wakelist())
>> >> + * p->on_rq = 0; p->sched_remote_wakeup = Y;
>> >> + *
>> >> + * guarantees all stores of 'current' are visible before
>> >> + * ->sched_remote_wakeup gets used, so it can be in this word.
>> >> + */
>> >
>> > Isn't the control dep between that ttwu() p->on_rq read and
>> > p->sched_remote_wakeup write "sufficient"?
>>
>> smp_acquire__after_ctrl_dep() that is, since we need
>> ->on_rq load => 'current' bits load + store
>
> I don't think we need that extra barrier; after all, there will be a
> complete schedule() between waking the task and it actually becoming
> current.
Apologies for the messy train of thought; what I was trying to say is that
we have already the following, which AIUI is sufficient:
* p->XXX = X; ttwu()
* schedule() if (p->on_rq && ..) // false
* smp_mb__after_spinlock(); smp_acquire__after_ctrl_dep();
* deactivate_task() ttwu_queue_wakelist()
* p->on_rq = 0; p->sched_remote_wakeup = Y;
