RE: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the per-channel lock

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 30 September 2020 07:42
> To: paul@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; 'Andrew Cooper' 
> <andrew.cooper3@xxxxxxxxxx>; 'George Dunlap'
> <George.Dunlap@xxxxxxxxxxxxx>; 'Ian Jackson' <iwj@xxxxxxxxxxxxxx>; 'Julien 
> Grall' <julien@xxxxxxx>;
> 'Wei Liu' <wl@xxxxxxx>; 'Stefano Stabellini' <sstabellini@xxxxxxxxxx>
> Subject: Re: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the 
> per-channel lock
> 
> On 29.09.2020 18:31, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan 
> >> Beulich
> >> Sent: 28 September 2020 11:58
> >> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap 
> >> <George.Dunlap@xxxxxxxxxxxxx>; Ian
> >> Jackson <iwj@xxxxxxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Wei Liu 
> >> <wl@xxxxxxx>; Stefano
> Stabellini
> >> <sstabellini@xxxxxxxxxx>
> >> Subject: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the 
> >> per-channel lock
> >>
> >> evtchn_fifo_set_pending() (invoked with the per-channel lock held) has
> >> two uses of the channel's priority field.
> >
> > AFAICT it is invoked with only the sending end's lock held...
> >
> >> The field gets updated by
> >> evtchn_fifo_set_priority() with only the per-domain event_lock held,
> >> i.e. the two reads may observe two different values. While the 2nd use
> >> could - afaict - in principle be replaced by q->priority, I think
> >> evtchn_set_priority() should acquire the per-channel lock in any event.
> >>
> >
> > ... so how is this going to help?
> 
> I guess the reasoning needs to change here - it should focus solely
> on using the finer grained lock here (as holding the per-domain one
> doesn't help anyway). It would then be patch 10 which addresses the
> (FIFO-specific) concern of possibly reading inconsistent values.
> 

Yes, it looks like patch #10 should ensure consistency.

Prior to ad34d0656fc at least the first layer of calls done in evtchn_send() 
didn't take the evtchn itself as an arg. Of course, evtchn_set_pending() then 
looked up the evtchn and passed it to evtchn_port_set_pending() without any 
locking in the interdomain case. I wonder whether, to make reasoning easier, 
there ought to be a rule that ABI entry points are always called with the 
evtchn lock held?

  Paul

> Jan