[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v13.2 01/14] vpci: use per-domain PCI lock to protect vpci structure
On 19.02.2024 12:47, Stewart Hildebrand wrote: > @@ -895,6 +891,15 @@ int vpci_msix_arch_print(const struct vpci_msix *msix) > { > unsigned int i; > > + /* > + * Assert that d->pdev_list doesn't change. > ASSERT_PDEV_LIST_IS_READ_LOCKED > + * is not suitable here because it may allow either pcidevs_lock() or > + * d->pci_lock to be held, but here we rely on d->pci_lock being held, > not > + * pcidevs_lock(). > + */ > + ASSERT(rw_is_locked(&msix->pdev->domain->pci_lock)); > + ASSERT(spin_is_locked(&msix->pdev->vpci->lock)); There's no "d" in sight here, so it's a little odd that "d" is being talked about. But I guess people can infer what's meant without too much trouble. > @@ -313,17 +316,36 @@ void vpci_dump_msi(void) > { > /* > * On error vpci_msix_arch_print will always return > without > - * holding the lock. > + * holding the locks. > */ > printk("unable to print all MSI-X entries: %d\n", rc); > - process_pending_softirqs(); > - continue; > + goto pdev_done; > } > } > > + /* > + * Unlock locks to process pending softirqs. This is > + * potentially unsafe, as d->pdev_list can be changed in > + * meantime. > + */ > spin_unlock(&pdev->vpci->lock); > + read_unlock(&d->pci_lock); > + pdev_done: > process_pending_softirqs(); > + if ( !read_trylock(&d->pci_lock) ) > + { > + printk("unable to access other devices for the domain\n"); > + goto domain_done; > + } > } > + read_unlock(&d->pci_lock); > + domain_done: > + /* > + * We need this label at the end of the loop, but some > + * compilers might not be happy about label at the end of the > + * compound statement so we adding an empty statement here. > + */ > + ; As to "some compilers": Are there any which accept a label not followed by a statement? Depending on the answer, this comment may be viewed as superfluous. Or else I'd ask about wording: Besides a grammar issue I also don't view it as appropriate that a comment talks about "adding" something when its adjacent code that is meant. That something is there when the comment is there, hence respective wording should imo be used. > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -171,6 +171,19 @@ void pcidevs_lock(void); > void pcidevs_unlock(void); > bool __must_check pcidevs_locked(void); > > +#ifndef NDEBUG > +/* > + * Check to ensure there will be no changes to the entries in d->pdev_list > (but > + * not the contents of each entry). > + * This check is not suitable for protecting other state or critical regions. > + */ > +#define ASSERT_PDEV_LIST_IS_READ_LOCKED(d) \ > + /* NB: d may be evaluated multiple times, or not at all */ \ > + ASSERT(pcidevs_locked() || ((d) && rw_is_locked(&(d)->pci_lock))) Is there actually any case where d can be NULL here? > +#else > +#define ASSERT_PDEV_LIST_IS_READ_LOCKED(d) ({ (void)(d); }) Evaluating d here isn't very useful when the assertion expression doesn't guarantee single evaluation. Plus even if it needed evaluating, there would be no need to use a compiler extension here: #define ASSERT_PDEV_LIST_IS_READ_LOCKED(d) ((void)(d)) Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |