Re: [Xen-devel] [PATCH 03/60] xen/sched: let sched_switch_sched() return new lock address

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.06.19 at 11:56, <dfaggioli@xxxxxxxx> wrote:
> On Wed, 2019-06-12 at 10:32 +0100, Jan Beulich wrote:
>> > > > On 12.06.19 at 10:19, <jgross@xxxxxxxx> wrote:
>> > On 12.06.19 10:05, Andrew Cooper wrote:
>> > > On 28/05/2019 11:32, Juergen Gross wrote:
>> > > > 
>> > > > +    per_cpu(scheduler, cpu) = new_ops;
>> > > > +    sd->sched_priv = ppriv;
>> > > > +
>> > > > +    /*
>> > > > +     * (Re?)route the lock to the per pCPU lock as /last/
>> > > > thing. In fact,
>> > > > +     * if it is free (and it can be) we want that anyone that
>> > > > manages
>> > > > +     * taking it, finds all the initializations we've done
>> > > > above in place.
>> > > > +     */
>> > > > +    smp_mb();
>> > > 
>> > > A full memory barrier is a massive overhead for what should be
>> > > smp_wmb().  The matching barrier is actually hidden in the
>> > > implicit
>> > > semantics of managing to lock sd->schedule_lock (which is trial
>> > > an error
>> > > anyway), but the only thing that matters here is that all other
>> > > written
>> > > data is in place first.
>> > > 
>> > Not that it would really matter for performance (switching cpus
>> > between
>> > cpupools is a _very_ rare operation), I'm fine transforming the
>> > barrier
>> > into smp_wmb().
>> 
>> This again is a change easy enough to make while committing. I'm
>> recording the above in case I end up being the committer.
>> 
> I'm fine (i.e., my Rev-by: remains valid) with this being turned into a
> wmb(), and it being done upon commit (thanks Jan for the offer to do
> that!).
> 
> If we do it, however, I think the comment needs to be adjusted too, and
> the commit message needs to briefly mention this new change we're
> doing.
> 
> Maybe, for the comment, add something like:
> 
> "The smp_wmb() corresponds to the barrier implicit in successfully
> taking the lock."

I'm not entirely happy with this one: Taking a lock is an implicit full
barrier, so cannot alone be used to reason why a write barrier is
sufficient here.

> And, for the changelog, add:
> 
> "While there, turn the full barrier, which was overkill, into an
> smp_wmb(), matching with the one implicit in managing to take the
> lock."

This one reads fine to me.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel