Re: [PATCH v3 4/5] evtchn: convert domain event lock to an r/w one

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 23/11/2020 13:29, Jan Beulich wrote:
> @@ -620,7 +620,7 @@ int evtchn_close(struct domain *d1, int
>       long           rc = 0;
>   
>    again:
> -    spin_lock(&d1->event_lock);
> +    write_lock(&d1->event_lock);
>   
>       if ( !port_is_valid(d1, port1) )
>       {
> @@ -690,13 +690,11 @@ int evtchn_close(struct domain *d1, int
>                   BUG();
>   
>               if ( d1 < d2 )
> -            {
> -                spin_lock(&d2->event_lock);
> -            }
> +                read_lock(&d2->event_lock);
This change made me realized that I don't quite understand how the 
rwlock is meant to work for event_lock. I was actually expecting this to 
be a write_lock() given there are state changed in the d2 events.
Could you outline how a developper can find out whether he/she should 
use read_lock or write_lock?
[...]

> --- a/xen/common/rwlock.c
> +++ b/xen/common/rwlock.c
> @@ -102,6 +102,14 @@ void queue_write_lock_slowpath(rwlock_t
>       spin_unlock(&lock->lock);
>   }
>   
> +void _rw_barrier(rwlock_t *lock)
> +{
> +    check_barrier(&lock->lock.debug);
> +    smp_mb();
> +    while ( _rw_is_locked(lock) )
> +        arch_lock_relax();
> +    smp_mb();
> +}
As I pointed out when this implementation was first proposed (see [1]), 
there is a risk that the loop will never exit.
I think the following implementation would be better (although it is ugly):

write_lock();
/* do nothing */
write_unlock();

This will act as a barrier between lock held before and after the call.

As an aside, I think the introduction of rw_barrier() deserve to be a in 
separate patch to help the review.
Cheers,

--
Julien Grall