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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
  • Date: Mon, 14 Feb 2022 14:34:00 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=gTCJecP4i3IZaqOPE/2k9vTMhudYuakj3LcaND1YHb0=; b=hl9FY6ayft8ET6Q4BW2+OOmPdxrHwRm4sUpJEPC3lDf4EHHMOCnFYWRxgxL8LF6tKIGAQ5D22+sAwb3AX+S6jDNPekBwreJsXFvGF2Vf3ncWCJZORnPKAAJwkflI9z2VeXJ1J4c31ZFqYgJogV+Im++9OxCkdeWMdJfNA+qIJAD3W23n6hN5H/Zdh+feWxo9n9wXsuKeuZIcPOSEe9cMzCJKZFctOXlVQp6Hq6QaPhcmodxxB9ln7XruQW0B7j6LRGxR0GYYP7sfoyaqhtIScNRID/z2Azzot/ZGccvVy7nSwcldJhTHrCMylLqdAKB3MgyStgHcopLIDK7Nxl4kxA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jOPGNlz6dSyam11sigDBRakRIVCkK7ICkRYPqpKHKhJS6p2Q7g+eBodtHdx3e6hjlWMhwsg/5whyln7Zp6PV1nOq8qz9Ubhm47EqqzWNdolFMIdjNZmsC/5ImHTTaQ+AgiJImpZL+taNDSAzRdz3pmniBPkE5QCT8CyLhyn1IsgBMIrTsTFBgWNBIpwtbIZJghM1Y8ddpKhUSLZBzVPA72vIWS0pRIfQhTLDYmM/toK0YOyTVJ6K/RuxvzBj7xsNMJB85QnB4hX88r/Mvop3xP02UyZvjYxLbDiYSNVltQOmvzPZy58395iQ/HvXQlVsQ0Zgmh0MJpPxlsmquUS9Tg==
  • Cc: "roger.pau@xxxxxxxxxx" <roger.pau@xxxxxxxxxx>, "julien@xxxxxxx" <julien@xxxxxxx>, "sstabellini@xxxxxxxxxx" <sstabellini@xxxxxxxxxx>, Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Artem Mygaiev <Artem_Mygaiev@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Rahul Singh <rahul.singh@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
  • Delivery-date: Mon, 14 Feb 2022 14:34:15 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYHboQT3cBWYI1/EGunE7hOwop6ayTIGYAgAABuQCAAAF7AIAAAMCA
  • Thread-topic: [PATCH] vpci: introduce per-domain lock to protect vpci structure


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

 


Rackspace

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