Re: [PATCH v13.2 01/14] vpci: use per-domain PCI lock to protect vpci structure

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 2/19/24 08:12, Jan Beulich wrote:
> On 19.02.2024 13:47, Stewart Hildebrand wrote:
>> On 2/19/24 07:10, Jan Beulich wrote:
>>> 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.
>>
>> I can s/d->pci_lock/msix->pdev->domain->pci_lock/ for the next rev.
> 
> Or simply drop the d-s? That would be better for readability's sake,
> I think.

OK

>>>> @@ -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.
>>
>> It seems like hit or miss whether gcc would accept it or not (prior
>> discussion at [1]). I agree the comment is rather lengthy for what it's
>> trying to convey. I'd be happy to either remove the comment or reduce
>> it to:
>>
>>     domain_done:
>>         ; /* Empty statement to make some compilers happy */
>>
>> [1] 
>> https://lore.kernel.org/xen-devel/98b8c131-b0b9-f46c-5f46-c2136f2e3b4e@xxxxxxx/
> 
> This earlier discussion only proves that there is at least one compiler
> objecting. There's no proof there that any compiler exists which, as a
> language extension, actually permits such syntax. Yet if the comment
> was purely about normal language syntax, then imo it should be zapped
> altogether, not just be shrunk.

I'll zap it