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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Thanks, Roger.