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

[Jürgen Groß <jgross@xxxxxxxx>]

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:
> > > > --- a/xen/arch/x86/pv/shim.c
> > > > +++ b/xen/arch/x86/pv/shim.c
> > > > @@ -660,11 +660,12 @@ void pv_shim_inject_evtchn(unsigned int port)
> > > >        if ( port_is_valid(guest, port) )
> > > >        {
> > > >            struct evtchn *chn = evtchn_from_port(guest, port);
> > > > -        unsigned long flags;
> > > >    
> > > > -        spin_lock_irqsave(&chn->lock, flags);
> > > > -        evtchn_port_set_pending(guest, chn->notify_vcpu_id, chn);
> > > > -        spin_unlock_irqrestore(&chn->lock, flags);
> > > > +        if ( evtchn_read_trylock(chn) )
> > > > +        {
> > > > +            evtchn_port_set_pending(guest, chn->notify_vcpu_id, chn);
> > > > +            evtchn_read_unlock(chn);
> > > > +        }
> > > Does this want some form of else, e.g. at least a printk_once()?
> > No, I don't think so.
> >
> > This is just a race with the port_is_valid() test above where the
> > port is just being switched to invalid.
> This may be such a race yes, but why do you think it _will_ be?
According to the outlined lock discipline there is no other
possibility (assuming that the lock discipline is honored).

I'll have a look whether I can add some ASSERT()s to catch any
lock discipline violation.

> > > > @@ -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.


Juergen