Re: [PATCH v3 2/2] xen/evtchn: rework per event channel lock

[Julien Grall <julien@xxxxxxx>]

On 04/11/2020 09:56, Jürgen Groß wrote:
> On 04.11.20 10:50, Julien Grall wrote:
> > Hi Juergen,
> >
> > On 02/11/2020 15:26, Jürgen Groß wrote:
> > > On 02.11.20 16:18, Jan Beulich wrote:
> > > > On 02.11.2020 14:59, Jürgen Groß wrote:
> > > > > On 02.11.20 14:52, Jan Beulich wrote:
> > > > > > On 02.11.2020 14:41, Jürgen Groß wrote:
> > > > > > > On 20.10.20 11:28, Jan Beulich wrote:
> > > > > > > > On 16.10.2020 12:58, Juergen Gross wrote:
> > > > > > > > > @@ -360,7 +352,7 @@ static long 
> > > > > > > > > evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
> > > > > > > > >         if ( rc )
> > > > > > > > >             goto out;
> > > > > > > > > -    flags = double_evtchn_lock(lchn, rchn);
> > > > > > > > > +    double_evtchn_lock(lchn, rchn);
> > > > > > > > This introduces an unfortunate conflict with my conversion of
> > > > > > > > the per-domain event lock to an rw one: It acquires rd's lock
> > > > > > > > in read mode only, while the requirements here would not allow
> > > > > > > > doing so. (Same in evtchn_close() then.)
> > > > > > > Is it a problem to use write mode for those cases?
> > > > > > "Problem" can have a wide range of meanings - it's not going to
> > > > > > be the end of the world, but I view any use of a write lock as
> > > > > > a problem when a read lock would suffice. This can still harm
> > > > > > parallelism.
> > > > > Both cases are very rare ones in the life time of an event channel. I
> > > > > don't think you'll ever be able to measure any performance impact from
> > > > > switching these case to a write lock for any well behaved guest.
> > > > I agree as far as the lifetime of an individual port goes, but
> > > > we're talking about the per-domain lock here. (Perhaps my
> > > > choice of context in your patch wasn't the best one, as there
> > > > it is the per-channel lock of which two instances get acquired.
> > > > I'm sorry if this has lead to any confusion.)
> > > Hmm, with the switch to an ordinary rwlock it should be fine to drop
> > > the requirement to hold the domain's event channel lock exclusively
> > > for taking the per-channel lock as a writer.
> > I don't think you can drop d->event_lock. It protects us against 
> > allocating new ports while evtchn_reset() is called.
> I wrote "exclusively", as in case of a switch to a rwlock it should be
> fine to hold it as a reader in case the reset coding takes it as a
> writer.
Oh I misread your comment. Sorry for the noise.

Cheers,

--
Julien Grall