[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |