RE: [PATCH 11/12] evtchn: convert vIRQ lock to an r/w one

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
> Sent: 28 September 2020 12:02
> To: xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
> Jackson <iwj@xenproject.org>; Julien Grall <julien@xen.org>; Wei Liu <wl@xen.org>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: [PATCH 11/12] evtchn: convert vIRQ lock to an r/w one
> 
> There's no need to serialize all sending of vIRQ-s; all that's needed
> is serialization against the closing of the respective event channels
> (by means of a barrier). To facilitate the conversion, introduce a new
> rw_barrier().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -160,7 +160,7 @@ struct vcpu *vcpu_create(struct domain *
>      v->vcpu_id = vcpu_id;
>      v->dirty_cpu = VCPU_CPU_CLEAN;
> 
> -    spin_lock_init(&v->virq_lock);
> +    rwlock_init(&v->virq_lock);
> 
>      tasklet_init(&v->continue_hypercall_tasklet, NULL, NULL);
> 
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -640,7 +640,7 @@ int evtchn_close(struct domain *d1, int
>              if ( v->virq_to_evtchn[chn1->u.virq] != port1 )
>                  continue;
>              v->virq_to_evtchn[chn1->u.virq] = 0;
> -            spin_barrier(&v->virq_lock);
> +            rw_barrier(&v->virq_lock);
>          }
>          break;
> 
> @@ -794,7 +794,7 @@ void send_guest_vcpu_virq(struct vcpu *v
> 
>      ASSERT(!virq_is_global(virq));
> 
> -    spin_lock_irqsave(&v->virq_lock, flags);
> +    read_lock_irqsave(&v->virq_lock, flags);
> 
>      port = v->virq_to_evtchn[virq];
>      if ( unlikely(port == 0) )
> @@ -807,7 +807,7 @@ void send_guest_vcpu_virq(struct vcpu *v
>      spin_unlock(&chn->lock);
> 
>   out:
> -    spin_unlock_irqrestore(&v->virq_lock, flags);
> +    read_unlock_irqrestore(&v->virq_lock, flags);
>  }
> 
>  void send_guest_global_virq(struct domain *d, uint32_t virq)
> @@ -826,7 +826,7 @@ void send_guest_global_virq(struct domai
>      if ( unlikely(v == NULL) )
>          return;
> 
> -    spin_lock_irqsave(&v->virq_lock, flags);
> +    read_lock_irqsave(&v->virq_lock, flags);
> 
>      port = v->virq_to_evtchn[virq];
>      if ( unlikely(port == 0) )
> @@ -838,7 +838,7 @@ void send_guest_global_virq(struct domai
>      spin_unlock(&chn->lock);
> 
>   out:
> -    spin_unlock_irqrestore(&v->virq_lock, flags);
> +    read_unlock_irqrestore(&v->virq_lock, flags);
>  }
> 
>  void send_guest_pirq(struct domain *d, const struct pirq *pirq)
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -2,7 +2,7 @@
>  #include <xen/irq.h>
>  #include <xen/smp.h>
>  #include <xen/time.h>
> -#include <xen/spinlock.h>
> +#include <xen/rwlock.h>
>  #include <xen/guest_access.h>
>  #include <xen/preempt.h>
>  #include <public/sysctl.h>
> @@ -334,6 +334,12 @@ void _spin_unlock_recursive(spinlock_t *
>      }
>  }
> 
> +void _rw_barrier(rwlock_t *lock)
> +{
> +    check_barrier(&lock->lock.debug);
> +    do { smp_mb(); } while ( _rw_is_locked(lock) );
> +}

Should you not have a cpu_relax() somewhere in here?

TBH though, the fact this lock is never taken as a writer makes me wonder whether there needs to be a lock at all.

  Paul

> +
>  #ifdef CONFIG_DEBUG_LOCK_PROFILE
> 
>  struct lock_profile_anc {
> --- a/xen/include/xen/rwlock.h
> +++ b/xen/include/xen/rwlock.h
> @@ -237,6 +237,8 @@ static inline int _rw_is_write_locked(rw
>      return (atomic_read(&lock->cnts) & _QW_WMASK) == _QW_LOCKED;
>  }
> 
> +void _rw_barrier(rwlock_t *lock);
> +
>  #define read_lock(l)                  _read_lock(l)
>  #define read_lock_irq(l)              _read_lock_irq(l)
>  #define read_lock_irqsave(l, f)                                 \
> @@ -266,6 +268,7 @@ static inline int _rw_is_write_locked(rw
>  #define rw_is_locked(l)               _rw_is_locked(l)
>  #define rw_is_write_locked(l)         _rw_is_write_locked(l)
> 
> +#define rw_barrier(l)                 _rw_barrier(l)
> 
>  typedef struct percpu_rwlock percpu_rwlock_t;
> 
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -235,7 +235,7 @@ struct vcpu
> 
>      /* IRQ-safe virq_lock protects against delivering VIRQ to stale evtchn. */
>      evtchn_port_t    virq_to_evtchn[NR_VIRQS];
> -    spinlock_t       virq_lock;
> +    rwlock_t         virq_lock;
> 
>      /* Tasklet for continue_hypercall_on_cpu(). */
>      struct tasklet   continue_hypercall_tasklet;
>