[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! 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |