[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 4/4] vpci: move lock outside of struct vpci



On 02.02.2022 12:03, Oleksandr Andrushchenko wrote:
> On 02.02.22 11:22, Jan Beulich wrote:
>> On 01.02.2022 17:25, Oleksandr Andrushchenko wrote:
>>> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
>>>
>>> This way the lock can be used to check whether vpci is present, and
>>> removal can be performed while holding the lock, in order to make
>>> sure there are no accesses to the contents of the vpci struct.
>>> Previously removal could race with vpci_read for example, since the
>>> lock was dropped prior to freeing pdev->vpci.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>> ---
>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>> Cc: Julien Grall <julien@xxxxxxx>
>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>> ---
>>> New in v5 of this series: this is an updated version of the patch published 
>>> at
>>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20180717094830.54806-2-roger.pau@xxxxxxxxxx/__;!!GF_29dbcQIUBPA!jmmcewY6y9Ur4rgvOgqscz8gBWaod2JnQOkHvWtYKgnqeU6BoWJTqCN3UDpCw3io8Ynk-wBXhA$
>>>  [lore[.]kernel[.]org]
>>>
>>> Changes since v5:
>> This is a little odd in a series implicitly tagged as v1.
>>
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -142,12 +142,13 @@ bool vpci_process_pending(struct vcpu *v)
>>>           if ( rc == -ERESTART )
>>>               return true;
>>>   
>>> -        spin_lock(&v->vpci.pdev->vpci->lock);
>>> -        /* Disable memory decoding unconditionally on failure. */
>>> -        modify_decoding(v->vpci.pdev,
>>> -                        rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : 
>>> v->vpci.cmd,
>>> -                        !rc && v->vpci.rom_only);
>>> -        spin_unlock(&v->vpci.pdev->vpci->lock);
>>> +        spin_lock(&v->vpci.pdev->vpci_lock);
>>> +        if ( v->vpci.pdev->vpci )
>>> +            /* Disable memory decoding unconditionally on failure. */
>>> +            modify_decoding(v->vpci.pdev,
>>> +                            rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : 
>>> v->vpci.cmd,
>>> +                            !rc && v->vpci.rom_only);
>>> +        spin_unlock(&v->vpci.pdev->vpci_lock);
>> While I certainly see the point, the addition of this if() (and a
>> few more elsewhere) isn't covered by title or description.
> The commit message says:
> "This way the lock can be used to check whether vpci is present, and
> removal can be performed while holding the lock, in order to make
> sure there are no accesses to the contents of the vpci struct."
> So, I think this is enough to describe the fact that after you have locked
> the protected structure may have gone already and we need to
> re-check it is still present.

I'm afraid that to me "can be used" describes future behavior, as
opposed to e.g. "is used". If you want to point out both aspects,
maybe "... can be used (and in a few cases is used right away) ..."?

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.