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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.09.2020 15:03, Julien Grall wrote:
> On 28/09/2020 12:02, Jan Beulich wrote:
>> 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().
> 
> Looking at the code below, all the spin_lock() have been replaced by a 
> read_lock_*(). This is a bit surprising,

It is, I agree, but that's what it ends up being. It is my understanding
that the lock really only exists for the purpose of the barrier in
evtchn_close().

>> --- 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>
> 
> I would prefer if keep including <xen/spinlock.h> as the fact 
> <xen/rwlock.h> include it is merely an implementation details.

Can do, sure.

>> @@ -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) );
>> +}
> 
> Why do you need to call smp_mb() at each loop? Would not it be 
> sufficient to write something similar to spin_barrier(). I.e:
> 
> smp_mb();
> while ( _rw_is_locked(lock) )
>    cpu_relax();
> smp_mb();

Yes, this looks to be possible. Both for this and the question
below it may be relevant to know that this patch pre-dates the
conversion to ticket locks by quite a bit. Hence the construct
above resembles the _spin_barrier() back at the time.

> But I wonder if there is a risk with either implementation for 
> _rw_is_locked() to always return true and therefore never end.
> 
> Let say we receive an interrupt, by the time it is handled, the 
> read/lock may have been taken again.

With non-ticket locks I would say there was the same issue. But
yes, ...

> spin_barrier() seems to handle this situation fine because it just wait 
> for the head to change. I don't think we can do the same here...
> 
> I am thinking that it may be easier to hold the write lock when doing 
> the update.

... perhaps this is indeed better. I have to admit that I never
fully understood the benefit of using spin_barrier() in this code
(as opposed to the use in evtchn_destroy()).

Jan