> -----Original Message----- > From: Jan Beulich <[email protected]> > Sent: 30 September 2020 07:42 > To: [email protected] > Cc: [email protected]; 'Andrew Cooper' > <[email protected]>; 'George Dunlap' > <[email protected]>; 'Ian Jackson' <[email protected]>; 'Julien > Grall' <[email protected]>; > 'Wei Liu' <[email protected]>; 'Stefano Stabellini' <[email protected]> > 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 <[email protected]> On Behalf Of Jan > >> Beulich > >> Sent: 28 September 2020 11:58 > >> To: [email protected] > >> Cc: Andrew Cooper <[email protected]>; George Dunlap > >> <[email protected]>; Ian > >> Jackson <[email protected]>; Julien Grall <[email protected]>; Wei Liu > >> <[email protected]>; Stefano > Stabellini > >> <[email protected]> > >> 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
