Re: [PATCH for-4.15] xen/sched: Add missing memory barrier in vcpu_block()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 23/02/2021 07:00, Jan Beulich wrote:
> On 22.02.2021 21:12, Julien Grall wrote:
> > On 22/02/2021 20:09, Stefano Stabellini wrote:
> > > On Mon, 22 Feb 2021, Jan Beulich wrote:
> > > > On 20.02.2021 20:47, Julien Grall wrote:
> > > > > This is a follow-up of the discussion that started in 2019 (see [1])
> > > > > regarding a possible race between do_poll()/vcpu_unblock() and the wake
> > > > > up path.
> > > > >
> > > > > I haven't yet fully thought about the potential race in do_poll(). If
> > > > > there is, then this would likely want to be fixed in a separate patch.
> > > > >
> > > > > On x86, the current code is safe because set_bit() is fully ordered. SO
> > > > > the problem is Arm (and potentially any new architectures).
> > > > >
> > > > > I couldn't convince myself whether the Arm implementation of
> > > > > local_events_need_delivery() contains enough barrier to prevent the
> > > > > re-ordering. However, I don't think we want to play with devil here as
> > > > > the function may be optimized in the future.
> > > > In fact I think this ...
> > > >
> > > > > --- a/xen/common/sched/core.c
> > > > > +++ b/xen/common/sched/core.c
> > > > > @@ -1418,6 +1418,8 @@ void vcpu_block(void)
> > > > >    
> > > > >        set_bit(_VPF_blocked, &v->pause_flags);
> > > > >    
> > > > > +    smp_mb__after_atomic();
> > > > > +
> > > > ... pattern should be looked for throughout the codebase, and barriers
> > > > be added unless it can be proven none is needed. >
> > > And in that case it would be best to add an in-code comment to explain
> > > why the barrier is not needed
> > .
> > I would rather not add comment for every *_bit() calls. It should be
> > pretty obvious for most of them that the barrier is not necessary.
> >
> > We should only add comments where the barrier is necessary or it is not
> > clear why it is not necessary.
> I guess by "pattern" I didn't necessarily mean _all_ *_bit()
> calls - indeed there are many where it's clear that no barrier
> would be needed. I was rather meaning modifications like this
> of v->pause_flags (I'm sure there are further such fields).
Agree, this work is mostly a side project for now. So I will continue to 
go through the pattern when I find time.
Cheers,

--
Julien Grall