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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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().

> --- 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?

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

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

Jan