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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Jan, Roger!

On 26.01.22 13:13, Roger Pau Monné wrote:
> On Wed, Jan 26, 2022 at 08:40:09AM +0000, Oleksandr Andrushchenko wrote:
>> Hello, Roger, Jan!
>>
>> On 12.01.22 16:42, Jan Beulich wrote:
>>> On 11.01.2022 16:17, Roger Pau Monné wrote:
>>>> On Thu, Nov 25, 2021 at 01:02:40PM +0200, Oleksandr Andrushchenko wrote:
>>>>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>>>>> index 657697fe3406..ceaac4516ff8 100644
>>>>> --- a/xen/drivers/vpci/vpci.c
>>>>> +++ b/xen/drivers/vpci/vpci.c
>>>>> @@ -35,12 +35,10 @@ extern vpci_register_init_t *const 
>>>>> __start_vpci_array[];
>>>>>    extern vpci_register_init_t *const __end_vpci_array[];
>>>>>    #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>>>>>    
>>>>> -void vpci_remove_device(struct pci_dev *pdev)
>>>>> +static void vpci_remove_device_handlers_locked(struct pci_dev *pdev)
>>>>>    {
>>>>> -    if ( !has_vpci(pdev->domain) )
>>>>> -        return;
>>>>> +    ASSERT(spin_is_locked(&pdev->vpci_lock));
>>>>>    
>>>>> -    spin_lock(&pdev->vpci->lock);
>>>>>        while ( !list_empty(&pdev->vpci->handlers) )
>>>>>        {
>>>>>            struct vpci_register *r = 
>>>>> list_first_entry(&pdev->vpci->handlers,
>>>>> @@ -50,15 +48,33 @@ void vpci_remove_device(struct pci_dev *pdev)
>>>>>            list_del(&r->node);
>>>>>            xfree(r);
>>>>>        }
>>>>> -    spin_unlock(&pdev->vpci->lock);
>>>>> +}
>>>>> +
>>>>> +void vpci_remove_device_locked(struct pci_dev *pdev)
>>>> I think this could be static instead, as it's only used by
>>>> vpci_remove_device and vpci_add_handlers which are local to the
>>>> file.
>> This is going to be used outside later on while processing pending mappings,
>> so I think it is not worth it defining it static here and then removing the 
>> static
>> key word later on: please see [PATCH v5 04/14] vpci: cancel pending 
>> map/unmap on vpci removal [1]
> I have some comments there also, which might change the approach
> you are using.
>
>>> Does the splitting out of vpci_remove_device_handlers_locked() belong in
>>> this patch in the first place? There's no second caller being added, so
>>> this looks to be an orthogonal adjustment.
>> I think of it as a preparation for the upcoming code: although the reason 
>> for the
>> change might not be immediately seen in this patch it is still in line with 
>> what
>> happens next.
> Right - it's generally best if the change is done together as the new
> callers are added. Otherwise it's hard to understand why certain changes
> are made, and you will likely get asked the same question on next
> rounds.
>
> It's also possible that the code that requires this is changed in
> further iterations so there's no longer a need for the splitting.
Ok, sounds reasonable
I will not split the functions without the obvious need
>
> Thanks, Roger.
Thank you,
Oleksandr