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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.12.2020 12:54, Julien Grall wrote:
> 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.

Well, the protection needs to be against racing changes, i.e.
parallel invocations of this same function, or evtchn_close().
It is debatable whether evtchn_status() and
domain_dump_evtchn_info() would better also be locked out
(other read_lock() uses aren't applicable to interdomain
channels).

> Could you outline how a developper can find out whether he/she should 
> use read_lock or write_lock?

I could try to, but it would again be a port type dependent
model, just like for the per-channel locks. So I'd like it to
be clarified first whether you aren't instead indirectly
asking for these to become 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.

The [1] reference was missing, but I recall you saying so.

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

Right, and back then I indicated agreement. When getting to
actually carry out the change, I realized though that then the less
restrictive check_barrier() can't be used anymore (or to be precise,
it could be used, but the stronger check_lock() would subsequently
still come into play). This isn't a problem here, but would be for
any IRQ-safe r/w lock that the barrier may want to be used on down
the road.

Thinking about it, a read_lock() / read_unlock() pair would suffice
though. But this would then still have check_lock() involved.

Given all of this, maybe it's better not to introduce the function
at all and instead open-code the read_lock() / read_unlock() pair at
the use site.

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

I'm aware there are differing views on this - to me, putting this in
a separate patch would be introduction of dead code. But as per
above maybe the function now won't get introduced anymore anyway.

Jan