On Thu, Sep 21, 2023 at 09:42:08AM +0200, Jan Beulich wrote: > On 20.09.2023 15:56, Stewart Hildebrand wrote: > > On 9/20/23 04:09, Roger Pau Monné wrote: > >> On Tue, Sep 19, 2023 at 12:20:39PM -0400, Stewart Hildebrand wrote: > >>> On 9/19/23 11:39, Roger Pau Monné wrote: > >>>> On Tue, Aug 29, 2023 at 11:19:42PM +0000, Volodymyr Babchuk wrote: > >>>>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c > >>>>> index 8f2b59e61a..a0733bb2cb 100644 > >>>>> --- a/xen/drivers/vpci/msi.c > >>>>> +++ b/xen/drivers/vpci/msi.c > >>>>> @@ -318,15 +321,28 @@ void vpci_dump_msi(void) > >>>>> * holding the lock. > >>>>> */ > >>>>> printk("unable to print all MSI-X entries: %d\n", > >>>>> rc); > >>>>> - process_pending_softirqs(); > >>>>> - continue; > >>>>> + goto pdev_done; > >>>>> } > >>>>> } > >>>>> > >>>>> spin_unlock(&pdev->vpci->lock); > >>>>> + pdev_done: > >>>>> + /* > >>>>> + * Unlock lock to process pending softirqs. This is > >>>>> + * potentially unsafe, as d->pdev_list can be changed in > >>>>> + * meantime. > >>>>> + */ > >>>>> + read_unlock(&d->pci_lock); > >>>>> process_pending_softirqs(); > >>>>> + if ( !read_trylock(&d->pci_lock) ) > >>>>> + { > >>>>> + printk("unable to access other devices for the > >>>>> domain\n"); > >>>>> + goto domain_done; > >>>> > >>>> Shouldn't the domain_done label be after the read_unlock(), so that we > >>>> can proceed to try to dump the devices for the next domain? With the > >>>> proposed code a failure to acquire one of the domains pci_lock > >>>> terminates the dump. > >>>> > >>>>> + } > >>>>> } > >>>>> + read_unlock(&d->pci_lock); > >>>>> } > >>>>> + domain_done: > >>>>> rcu_read_unlock(&domlist_read_lock); > >>>>> } > >>>>> > >>> > >>> With the label moved, a no-op expression after the label is needed to > >>> make the compiler happy: > >>> > >>> } > >>> } > >>> read_unlock(&d->pci_lock); > >>> domain_done: > >>> (void)0; > >>> } > >>> rcu_read_unlock(&domlist_read_lock); > >>> } > >>> > >>> > >>> If the no-op is omitted, the compiler may complain (gcc 9.4.0): > >>> > >>> drivers/vpci/msi.c: In function ‘vpci_dump_msi’: > >>> drivers/vpci/msi.c:351:2: error: label at end of compound statement > >>> 351 | domain_done: > >>> | ^~~~~~~~~~~ > >> > >> > >> Might be better to place the label at the start of the loop, and > >> likely rename to next_domain. > > > > That would bypass the loop condition and increment statements. > > Right, such a label would be bogus even without that; instead of "goto" > the use site then simply should use "continue".
IIRC continue is not suitable because the code would reach the read_unlock() without having the lock held. Anyway, I would leave to the submitter to find a suitable way to continue the domain iteration. Thanks, Roger.