> -----Original Message----- > From: Jan Beulich <jbeul...@suse.com> > Sent: 30 September 2020 07:42 > To: p...@xen.org > Cc: xen-devel@lists.xenproject.org; 'Andrew Cooper' > <andrew.coop...@citrix.com>; 'George Dunlap' > <george.dun...@eu.citrix.com>; 'Ian Jackson' <i...@xenproject.org>; 'Julien > Grall' <jul...@xen.org>; > 'Wei Liu' <w...@xen.org>; 'Stefano Stabellini' <sstabell...@kernel.org> > 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-boun...@lists.xenproject.org> On Behalf Of Jan > >> Beulich > >> Sent: 28 September 2020 11:58 > >> To: xen-devel@lists.xenproject.org > >> Cc: Andrew Cooper <andrew.coop...@citrix.com>; George Dunlap > >> <george.dun...@eu.citrix.com>; Ian > >> Jackson <i...@xenproject.org>; Julien Grall <jul...@xen.org>; Wei Liu > >> <w...@xen.org>; Stefano > Stabellini > >> <sstabell...@kernel.org> > >> 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
