Re: [PATCH v2 6/8] evtchn: convert vIRQ lock to an r/w one

[Julien Grall <julien@xxxxxxx>]

On 30/10/2020 12:25, Jan Beulich wrote:
> On 30.10.2020 13:08, Julien Grall wrote:
> > On 30/10/2020 12:00, Jan Beulich wrote:
> > > On 30.10.2020 11:57, Julien Grall wrote:
> > > > On 20/10/2020 15:10, Jan Beulich wrote:
> > > > > --- a/xen/common/event_channel.c
> > > > > +++ b/xen/common/event_channel.c
> > > > > @@ -449,6 +449,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t
> > > > >     
> > > > >         spin_unlock_irqrestore(&chn->lock, flags);
> > > > >     
> > > > > +    /*
> > > > > +     * If by any, the update of virq_to_evtchn[] would need guarding by
> > > > > +     * virq_lock, but since this is the last action here, there's no strict
> > > > > +     * need to acquire the lock. Hnece holding event_lock isn't helpful
> > > > s/Hnece/Hence/
> > > >
> > > > > +     * anymore at this point, but utilize that its unlocking acts as the
> > > > > +     * otherwise necessary smp_wmb() here.
> > > > > +     */
> > > > >         v->virq_to_evtchn[virq] = bind->port = port;
> > > > I think all access to v->virq_to_evtchn[virq] should use ACCESS_ONCE()
> > > > or {read, write}_atomic() to avoid any store/load tearing.
> > > IOW you're suggesting this to be the subject of a separate patch?
> > > I don't think such a conversion belongs here (nor even in this
> > > series, seeing the much wider applicability of such a change
> > > throughout the code base).
> > > Or are you seeing anything here which
> > > would require such a conversion to be done as a prereq?
> > Yes, your comment implies that it is fine to write to virq_to_evtchn[]
> > without the lock taken. However, this is *only* valid if the compiler
> > doesn't tear down load/store.
> >
> > So this is a pre-req to get your comment valid.
> That's an interesting way to view it. I'm only adding the comment,
> without changing the code here. Iirc it was you who asked me to
> add the comment. 
Yes I asked this comment because it makes easier for reader to figure 
out how your code works. But this doesn't mean I wanted to have a 
misleading comment.
If you don't plan to handle the ACCESS_ONCE(), then it is best to be 
open to say it in the comment.
> And now this is leading to you wanting me to do
> entirely unrelated changes, when the code base is full of similar
> assumptions towards no torn accesses? 
I was expecting you writing that :). Well, small steps are always easier 
in order to achieve a consistent code base.
To me, this is similar to some of the reviewers pushing contributors to 
update the coding style of area touched. The difference here is without 
following a memory model, you are at the mercy of the compiler and XSAs.
> (Yes, I certainly can add
> such a patch, but no, I don't think this should be a requirement
> here.)
That's ok. But please update the comment to avoid misleading the reader.

An alternative would be to use the write_lock() here. This shouldn't 
impact that much the performance and nicely fix the issue.
Cheers,

--
Julien Grall