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

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

On 09.11.20 12:58, Jan Beulich wrote:
> On 09.11.2020 07:41, Juergen Gross wrote:
> > Currently the lock for a single event channel needs to be taken with
> > interrupts off, which causes deadlocks in some cases.
> >
> > Rework the per event channel lock to be non-blocking for the case of
> > sending an event and removing the need for disabling interrupts for
> > taking the lock.
> >
> > The lock is needed for avoiding races between event channel state
> > changes (creation, closing, binding) against normal operations (set
> > pending, [un]masking, priority changes).
> >
> > Use a rwlock, but with some restrictions:
> >
> > - normal operations use read_trylock(), in case of not obtaining the
> >    lock the operation is omitted or a default state is returned
> >
> > - closing an event channel is using write_lock(), with ASSERT()ing that
> >    the lock is taken as writer only when the state of the event channel
> >    is either before or after the locked region appropriate (either free
> >    or unbound).
> This has become stale, and may have been incomplete already before:
> - Normal operations now may use two diffeent approaches. Which one
> is to be used when would want writing down here.
> - write_lock() use goes beyond closing.
Yes, you are right.

> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -2495,14 +2495,12 @@ static void dump_irqs(unsigned char key)
> >                   pirq = domain_irq_to_pirq(d, irq);
> >                   info = pirq_info(d, pirq);
> >                   evtchn = evtchn_from_port(d, info->evtchn);
> > -                local_irq_disable();
> > -                if ( spin_trylock(&evtchn->lock) )
> > +                if ( evtchn_read_trylock(evtchn) )
> >                   {
> >                       pending = evtchn_is_pending(d, evtchn);
> >                       masked = evtchn_is_masked(d, evtchn);
> > -                    spin_unlock(&evtchn->lock);
> > +                    evtchn_read_unlock(evtchn);
> >                   }
> > -                local_irq_enable();
> >                   printk("d%d:%3d(%c%c%c)%c",
> >                          d->domain_id, pirq, "-P?"[pending],
> >                          "-M?"[masked], info->masked ? 'M' : '-',
> Using trylock here has a reason different from that in sending
> functions, aiui. Please say so in the description, to justify
> exposure of evtchn_read_lock().
Okay.

> > --- 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 need trylock?
It is called directly from the event upcall, so interrupts should be
off here. Without trylock this would result in check_lock() triggering.

> > @@ -1068,15 +1088,16 @@ int evtchn_unmask(unsigned int port)
> >   {
> >       struct domain *d = current->domain;
> >       struct evtchn *evtchn;
> > -    unsigned long flags;
> >   
> >       if ( unlikely(!port_is_valid(d, port)) )
> >           return -EINVAL;
> >   
> >       evtchn = evtchn_from_port(d, port);
> > -    spin_lock_irqsave(&evtchn->lock, flags);
> > -    evtchn_port_unmask(d, evtchn);
> > -    spin_unlock_irqrestore(&evtchn->lock, flags);
> > +    if ( evtchn_read_trylock(evtchn) )
> > +    {
> > +        evtchn_port_unmask(d, evtchn);
> > +        evtchn_read_unlock(evtchn);
> > +    }
> I think this one could as well use plain read_lock().
Oh, indeed.

> > @@ -234,12 +244,13 @@ static inline bool evtchn_is_masked(const struct domain 
> > *d,
> >   static inline bool evtchn_port_is_masked(struct domain *d, evtchn_port_t port)
> >   {
> >       struct evtchn *evtchn = evtchn_from_port(d, port);
> > -    bool rc;
> > -    unsigned long flags;
> > +    bool rc = true;
> >   
> > -    spin_lock_irqsave(&evtchn->lock, flags);
> > -    rc = evtchn_is_masked(d, evtchn);
> > -    spin_unlock_irqrestore(&evtchn->lock, flags);
> > +    if ( evtchn_read_trylock(evtchn) )
> > +    {
> > +        rc = evtchn_is_masked(d, evtchn);
> > +        evtchn_read_unlock(evtchn);
> > +    }
> >   
> >       return rc;
> >   }
> > @@ -252,12 +263,13 @@ static inline int evtchn_port_poll(struct domain *d, 
> > evtchn_port_t port)
> >       if ( port_is_valid(d, port) )
> >       {
> >           struct evtchn *evtchn = evtchn_from_port(d, port);
> > -        unsigned long flags;
> >   
> > -        spin_lock_irqsave(&evtchn->lock, flags);
> > -        if ( evtchn_usable(evtchn) )
> > -            rc = evtchn_is_pending(d, evtchn);
> > -        spin_unlock_irqrestore(&evtchn->lock, flags);
> > +        if ( evtchn_read_trylock(evtchn) )
> > +        {
> > +            if ( evtchn_usable(evtchn) )
> > +                rc = evtchn_is_pending(d, evtchn);
> > +            evtchn_read_unlock(evtchn);
> > +        }
> >       }
> >   
> >       return rc;
> At least for the latter I suppose it should also be plain read_lock().
> We ought to keep the exceptions to where they're actually needed, I
> think.
I think both instances can be switched.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature