Re: [PATCH v2 1/4] pci: add rwlock to pcidevs_lock machinery

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hello Wei,

Wei Chen <Wei.Chen@xxxxxxx> writes:

> Hi Volodymyr,
>
> On 2022/7/19 5:15, Volodymyr Babchuk wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>
>>         if ( !use_msi )
>>           return -EOPNOTSUPP;
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 938821e593..f93922acc8 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -50,21 +50,74 @@ struct pci_seg {
>>       } bus2bridge[MAX_BUSES];
>>   };
>>   -static spinlock_t _pcidevs_lock = SPIN_LOCK_UNLOCKED;
>> +static DEFINE_RWLOCK(_pcidevs_rwlock);
>> +static DEFINE_PER_CPU(unsigned int, pcidevs_read_cnt);
>> +static DEFINE_PER_CPU(unsigned int, pcidevs_write_cnt);
>>     void pcidevs_lock(void)
>>   {
>> -    spin_lock_recursive(&_pcidevs_lock);
>> +    pcidevs_write_lock();
>>   }
>>     void pcidevs_unlock(void)
>>   {
>> -    spin_unlock_recursive(&_pcidevs_lock);
>> +    pcidevs_write_unlock();
>>   }
>>   -bool_t pcidevs_locked(void)
>> +bool pcidevs_locked(void)
>>   {
>> -    return !!spin_is_locked(&_pcidevs_lock);
>> +    return pcidevs_write_locked();
>> +}
>> +
>> +void pcidevs_read_lock(void)
>> +{
>> +    if ( this_cpu(pcidevs_read_cnt)++ == 0 )
>> +        read_lock(&_pcidevs_rwlock);
>> +}
>> +
>
> For my understanding, if pcidevs_read_cnt > 0, pcidevs_read_lock
> will be unblocked.I am not sure if this behavior is consistent with
> the original lock? According to my understanding, the original
> spinlock should be blocked all the time, if the lock is not
> acquired. Maybe

Original spinlock was recursive one. As read-write locks are
non-recursive in Xen, we need to implement some other mechanism to
support recursion. This code ensures that pCPU will not dead-lock itself
if it'll call pcidevs_read_lock() twice. Per-CPU counter ensures that
read_unlock() will be called only when pcidevs_read_unlock() calls is
balanced with pcidevs_read_lock()s.

> I have misunderstanding something, I am not very familiar with PCI
> subsystem.
>
> Cheers,
> Wei Chen

[...]

-- 
Volodymyr Babchuk at EPAM