Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Jan!

On 12.01.22 16:57, Jan Beulich wrote:
> On 25.11.2021 12:02, Oleksandr Andrushchenko wrote:
>> @@ -68,12 +84,13 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>       /* We should not get here twice for the same device. */
>>       ASSERT(!pdev->vpci);
>>   
>> -    pdev->vpci = xzalloc(struct vpci);
>> -    if ( !pdev->vpci )
>> +    vpci = xzalloc(struct vpci);
>> +    if ( !vpci )
>>           return -ENOMEM;
>>   
>> +    spin_lock(&pdev->vpci_lock);
>> +    pdev->vpci = vpci;
>>       INIT_LIST_HEAD(&pdev->vpci->handlers);
>> -    spin_lock_init(&pdev->vpci->lock);
> INIT_LIST_HEAD() can occur ahead of taking the lock, and can also act
> on &vpci->handlers rather than &pdev->vpci->handlers.
Yes, I will move it, good catch
>>       for ( i = 0; i < NUM_VPCI_INIT; i++ )
>>       {
>> @@ -83,7 +100,8 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>       }
> This loop wants to live in the locked region because you need to install
> vpci into pdev->vpci up front, afaict. I wonder whether that couldn't
> be relaxed, but perhaps that's an improvement that can be thought about
> later.
Ok, so I'll leave it as is
>
> The main reason I'm looking at this closely is because from the patch
> title I didn't expect new locking regions to be introduced right here;
> instead I did expect strictly a mechanical conversion.
>
>> @@ -152,8 +170,6 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
>> *read_handler,
>>       r->offset = offset;
>>       r->private = data;
>>   
>> -    spin_lock(&vpci->lock);
>  From the description I can't deduce why this lock is fine to go away
> now, i.e. that all call sites have the lock now acquire earlier.
> Therefore I'd expect at least an assertion to be left here ...
>
>> @@ -183,7 +197,6 @@ int vpci_remove_register(struct vpci *vpci, unsigned int 
>> offset,
>>       const struct vpci_register r = { .offset = offset, .size = size };
>>       struct vpci_register *rm;
>>   
>> -    spin_lock(&vpci->lock);
> ... and here.
Previously the lock lived in struct vpci and now it lives in struct pci_dev 
which
is not visible here, so:
1. we cannot take that lock here and do expect for it to be acquired outside
2. we cannot add an ASSERT here as we would need
ASSERT(spin_is_locked(&pdev->vpci_lock));
and pdev is not here
All the callers of the vpci_{add|remove}_register are REGISTER_VPCI_INIT
functions which are called with &pdev->vpci_lock held.

So, while I agree that it would be indeed a good check with ASSERT here,
but adding an additional argument to the respective functions just for that
might not be a good idea IMO

I will describe this lock removal in the commit message

Thank you,
Oleksandr