Re: [PATCH] vpci: introduce per-domain lock to protect vpci structure

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 14.02.22 16:31, Jan Beulich wrote:
> On 14.02.2022 15:26, Oleksandr Andrushchenko wrote:
>>
>> On 14.02.22 16:19, Jan Beulich wrote:
>>> On 09.02.2022 14:36, Oleksandr Andrushchenko wrote:
>>>> @@ -410,14 +428,37 @@ static void vpci_write_helper(const struct pci_dev 
>>>> *pdev,
>>>>                 r->private);
>>>>    }
>>>>    
>>>> +static bool vpci_header_write_lock(const struct pci_dev *pdev,
>>>> +                                   unsigned int start, unsigned int size)
>>>> +{
>>>> +    /*
>>>> +     * Writing the command register and ROM BAR register may trigger
>>>> +     * modify_bars to run which in turn may access multiple pdevs while
>>>> +     * checking for the existing BAR's overlap. The overlapping check, if 
>>>> done
>>>> +     * under the read lock, requires vpci->lock to be acquired on both 
>>>> devices
>>>> +     * being compared, which may produce a deadlock. It is not possible to
>>>> +     * upgrade read lock to write lock in such a case. So, in order to 
>>>> prevent
>>>> +     * the deadlock, check which registers are going to be written and 
>>>> acquire
>>>> +     * the lock in the appropriate mode from the beginning.
>>>> +     */
>>>> +    if ( !vpci_offset_cmp(start, size, PCI_COMMAND, 2) )
>>>> +        return true;
>>>> +
>>>> +    if ( !vpci_offset_cmp(start, size, pdev->vpci->header.rom_reg, 4) )
>>>> +        return true;
>>>> +
>>>> +    return false;
>>>> +}
>>> A function of this name gives (especially at the call site(s)) the
>>> impression of acquiring a lock. Considering that of the prefixes
>>> neither "vpci" nor "header" are really relevant here, may I suggest
>>> to use need_write_lock()?
>>>
>>> May I further suggest that you either split the comment or combine
>>> the two if()-s (perhaps even straight into single return statement)?
>>> Personally I'd prefer the single return statement approach here ...
>> That was already questioned by Roger and now it looks like:
>>
>> static bool overlap(unsigned int r1_offset, unsigned int r1_size,
>>                       unsigned int r2_offset, unsigned int r2_size)
>> {
>>       /* Return true if there is an overlap. */
>>       return r1_offset < r2_offset + r2_size && r2_offset < r1_offset + 
>> r1_size;
>> }
>>
>> bool vpci_header_write_lock(const struct pci_dev *pdev,
>>                               unsigned int start, unsigned int size)
>> {
>>       /*
>>        * Writing the command register and ROM BAR register may trigger
>>        * modify_bars to run which in turn may access multiple pdevs while
>>        * checking for the existing BAR's overlap. The overlapping check, if 
>> done
>>        * under the read lock, requires vpci->lock to be acquired on both 
>> devices
>>        * being compared, which may produce a deadlock. It is not possible to
>>        * upgrade read lock to write lock in such a case. So, in order to 
>> prevent
>>        * the deadlock, check which registers are going to be written and 
>> acquire
>>        * the lock in the appropriate mode from the beginning.
>>        */
>>       if ( overlap(start, size, PCI_COMMAND, 2) ||
>>            (pdev->vpci->header.rom_reg &&
>>             overlap(start, size, pdev->vpci->header.rom_reg, 4)) )
>>           return true;
>>
>>       return false;
>> }
>>
>> vpci_header_write_lock moved to header.c and is not static anymore.
>> So, sitting in header.c, the name seems to be appropriate now
> The prefix of the name - yes. But as said, a function of this name looks
> as if it would acquire a lock. Imo you want to insert "need" or some
> such.
Agree. Then vpci_header_need_write_lock.
I will also update the comment because it makes an impression that
the function acquires the lock
>
> Jan
>
Thank you,
Oleksandr