Re: [RFC PATCH 01/10] xen: pci: add per-domain pci list lock

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.01.2023 00:18, Stefano Stabellini wrote:
> On Wed, 31 Aug 2022, Volodymyr Babchuk wrote:
>> @@ -1571,6 +1595,7 @@ static int iommu_get_device_group(
>>          if ( sdev_id < 0 )
>>          {
>>              pcidevs_unlock();
>> +            spin_unlock(&d->pdevs_lock);
> 
> lock inversion
> 
> 
>>              return sdev_id;
>>          }
>>  
>> @@ -1581,6 +1606,7 @@ static int iommu_get_device_group(
>>              if ( unlikely(copy_to_guest_offset(buf, i, &bdf, 1)) )
>>              {
>>                  pcidevs_unlock();
>> +                spin_unlock(&d->pdevs_lock);
> 
> lock inversion
> 
> 
>>                  return -EFAULT;
>>              }
>>              i++;
>> @@ -1588,6 +1614,7 @@ static int iommu_get_device_group(
>>      }
>>  
>>      pcidevs_unlock();
>> +    spin_unlock(&d->pdevs_lock);
> 
> lock inversion

While from a cosmetic perspective I of course agree that releasing locks
would better be done the opposite order of acquiring whenever possible,
I'd like to point out that lock release alone is never subject to "lock
order" issues. We do have a couple of cases (I think) where we actually
do so because otherwise respective code would end up uglier, or because
we want to limit locking regions as much as possible (I'm sorry, I don't
have an example to hand).

>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -457,6 +457,7 @@ struct domain
>>  
>>  #ifdef CONFIG_HAS_PCI
>>      struct list_head pdev_list;
>> +    spinlock_t pdevs_lock;
> 
> I think it would be better called "pdev_lock" but OK either way

I'm of two minds here: On one hand the lock is to guard all devices, so
plural looks applicable. Otoh the list head field uses singular as well.

Jan