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

Re: [PATCH v6 03/13] vpci: move lock outside of struct vpci


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Fri, 4 Feb 2022 12:13:53 +0100
  • 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=4xdNYezEZ3ZYqc8knLkXt2lQHsUTsGpM53PxmvAtwck=; b=DsJGATfSJDUpUsishGfcGljMsoXkFVPLrwLVjvoD4EAF4OR30xPgIP7AB1AOcAq6uZT9Ad9DRWHklFy9SMTfsPd/xOQqz+pl/y61i9oa5KSOlvWy+640OGmkOKqXgoQuqhMMOVPacEp/pDpS2+/TDVQ2+r6KGT0LgbX1dozIw8uXnnTsaah5XHkxkGLBrWDvx3FKItEAwcSeRNWD/SrSVpBwMVgxS4Ik57QR/sVp8eqQG/4Pa3l1HaAlkIHKOb7A2FpY/ZifMqezOcysEDwgW8MHwTdKksx66/UN5dkTEPOYW0DgMtWwy8qn98bAQMEygNYs5s5pYgp5BoWrBN9gjA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=kBxjofkC8OmiDahhuVp/OFYJDZVa0k6iGDmkgqJeMT1cKVhDCue1q8cDOo5d3BTVbyEzJZngXLuwAfrFJwj3PzFBwPwbIHHIHjhRCv55eATmRj2xWEVFc1LxT54iajHpMUNYvw9exCX4IyFoC/ZZ9OLDiNPdbnpI6JYU08EBcXflVN6e6wgnjiwtbQVKOWkByeX870WF+0Apu5qUMGjWnhIQgNL4ffeoiQGKsFguUenPxnKddQcSihjFQ31KAMyojReZKjZdIQN3HXauBURdmToVRWKTt9PJIMMqlMBHnL6QVCSABbii77AbX4tIHhCfSc6134yBxF5IDhpiS2jsqA==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>, "julien@xxxxxxx" <julien@xxxxxxx>, "sstabellini@xxxxxxxxxx" <sstabellini@xxxxxxxxxx>, Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Artem Mygaiev <Artem_Mygaiev@xxxxxxxx>, "andrew.cooper3@xxxxxxxxxx" <andrew.cooper3@xxxxxxxxxx>, "george.dunlap@xxxxxxxxxx" <george.dunlap@xxxxxxxxxx>, "paul@xxxxxxx" <paul@xxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Rahul Singh <rahul.singh@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 04 Feb 2022 11:14:33 +0000
  • Ironport-data: A9a23:AMsLhqBN+g0MIRVW/wvlw5YqxClBgxIJ4kV8jS/XYbTApD0qhjdTn TBOWD+POfmDYGbxe41ybI2w8UgE75SEn99rQQY4rX1jcSlH+JHPbTi7wuYcHM8wwunrFh8PA xA2M4GYRCwMo/u1Si6FatANl1ElvU2zbue6WL6s1hxZH1c+En970U47wYbVv6Yz6TSHK1LV0 T/Ni5W31G+Ng1aY5UpNtspvADs21BjDkGtwUm4WPJinj3eH/5UhN7oNJLnZEpfNatI88thW5 Qr05OrREmvxp3/BAz4++1rxWhVirrX6ZWBihpfKMkQLb9crSiEai84G2PQghUh/iCTUx897+ Pp0kLu9RikvMKDKu+EBTEwNe81+FfUuFL7vJHG+tYqYzlHccmuqyPJrZK00FdRGoKAtWzgIr KFGbmBWBvyAr7veLLaTUO5ji95lNMD2FIgepmth3XfSCvNOrZXrHf6WtIQAg2tYasZmGfzbY /QfdDRWTDvOXRhINQcpFpgcg7L97pX4W2IB8w/EzUYt2EDLxRF1+KjgNpzSYNPibexPgkudk UfX8G34Dw8yOcSWzHyO9XfEruXChz/hUYQeUrix7Od3gUa7z3YWThYRUDOTsfS/z0KzRd9bA 0gV4TY167g/8lSxSdvwVAH+p2SL1jYGUtpNF6sh6QeCyoLd+QPfDW8BJhZ/b9ghuN4zVCYd/ FaDlNP0BhRiqLSQD3ma89+8ije/OjMcK2MYUgYCQREY+NnooIw1jRXnQ85qFei+ididMTP6z i2OrSM+r64OlsNN3KK+lXjFnjatq57hXgMzoALNUQqNyQd0Z5WsYYCy3mTK9vZLLIufTV6ps WANno6V6+VmJYqWiCWHTeEJHbeoz/WIKjvRhRhoBZZJyti20yf9J8YKumg4fRo3dJZfEdP0X KPNkQBAucdMNkuFVIpQbo2fDcony4vlCcuwA5g4ceFySpR2cQaG+gRnakiRw33hnSAQrE0vB XuIWZ3yVChHUMyL2BLzHr5AiuFzmkjS0EuOHciT8vix7VaJiJd5o58hOUDGUO025bjsTO79o 4cGbJviJ/myvYTDjsjrHWw7cAhiwZsTX8meRylrmgireFEO9IYJUKe5/F/ZU9Y595m5b8+Rl p1HZmdWyUDkmVrMIhiQZ3ZoZdvHBMgj9iJgYHJxbA33gBDPhLpDCo9FLPPbmpF8rIReIQNcF aFZK61s/NwTItg4x9jtRcak99EzHPharQmPIzCkcFACk21IHGT0FivfVlK3rkEmV3Pv3eNn+ uHI/l6LEPIrGlU5ZO6LOa3H8r9ElSVE8A6EdxCTeYc7lYSF2NUCFhEdeddscpxVdEWemmDHv +tUaD9BzdTwT0YO2IChrYiPrpuzEvs4GUxfHmLB6q2xOzWc9W2mqbKsms7RFdwEfG+rqqike 8tPyPTwbK8OkFpQ6tIuGLd316MuodDoouYCnAhjGXzKaXWtC69hfSbajZUe6PUVy+8LoxayV 2KO5sJeZeeDNvT6HQNDPwEidOmCi60Zw2GA8fQvLUzmzyZr577bA15KNhyBhXUFfrt4OY8o2 8k7v8sS51DtgxYmKI/e3CtV636NPjoLVKB+7sMWB4riiwwKzFBeYMODVn+qsc/XM9gVaxskO D6ZgqbGlo9w/EuafiphD2XJ0MpcmY8K5EJAwmgdKgnbgdHCnPI2gkFcqGxlUgRPwxxb+OtvI Ww3ZVZtLKCD8jo01shOW2egR1NICBGDoxGjzlIIkCvSTlWyV3yLJ2o4YL7f8Ecc+mNaXz5a4 LDHlzq1DWe0JJn8jnkoREpoi/3/VtggpATNlfeuE9mBA5RnMyHuhbWjZDZQphbqaS/raJYre QW+ED5MVJDG
  • Ironport-hdrordr: A9a23:q0JI5KhF9u5klKg9OfaxHCqQE3BQXi8ji2hC6mlwRA09TyXBrb HIoB1p726TtN9xYgBbpTnuAtjifZqxz/FICMwqTNOftWrdyRaVxeNZnOnfKlTbckWUnIMw6U 4jSdkaNDSaNykFsS+O2mmF+qEboeVvnprHuQ6U9QYVcegjUdAZ0y5JTj+BFEt4XQ9HAod8Oq a9y6N81kWdUEVSV9+8AHYdWejFupnsr7LJJTA7JzNP0njzsduPgISKYiRw8C1uKA9y/Q==
  • Ironport-sdr: UyWjaO6sFXOeqdAduZltg+W+Brk5AEfeRDBSgD7mW6PCF/Po197fc/HIgFSUFWr5TDAbmbmPtU 4A4LVOvhdyD8FwErgRhROFG5AkZmH2KYHU6u7aFAKYLJOJpqAlWkl0rzk9oDIAmT1OyXn7+c7G qT/Vpcav6RQA6HSVSSa7prU6xu0yV21Ja5pVYEQfJcdWpWiKxVFocDWkOTkrvHCQU8oNvH7CFU B6npd/Jo8pV2Lc2A4+Qc7voR+0hGclw1n8FYkQjUBzDpjxzkEb0lEn9Pkh9XpreG9r2ApwJYES 7najkuVDGSoguvxyZ6X6+cKG
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Fri, Feb 04, 2022 at 11:49:18AM +0100, Jan Beulich wrote:
> On 04.02.2022 11:12, Oleksandr Andrushchenko wrote:
> > On 04.02.22 11:15, Jan Beulich wrote:
> >> On 04.02.2022 09:58, Oleksandr Andrushchenko wrote:
> >>> On 04.02.22 09:52, Jan Beulich wrote:
> >>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
> >>>>> @@ -285,6 +286,12 @@ static int modify_bars(const struct pci_dev *pdev, 
> >>>>> uint16_t cmd, bool rom_only)
> >>>>>                    continue;
> >>>>>            }
> >>>>>    
> >>>>> +        spin_lock(&tmp->vpci_lock);
> >>>>> +        if ( !tmp->vpci )
> >>>>> +        {
> >>>>> +            spin_unlock(&tmp->vpci_lock);
> >>>>> +            continue;
> >>>>> +        }
> >>>>>            for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
> >>>>>            {
> >>>>>                const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
> >>>>> @@ -303,12 +310,14 @@ static int modify_bars(const struct pci_dev 
> >>>>> *pdev, uint16_t cmd, bool rom_only)
> >>>>>                rc = rangeset_remove_range(mem, start, end);
> >>>>>                if ( rc )
> >>>>>                {
> >>>>> +                spin_unlock(&tmp->vpci_lock);
> >>>>>                    printk(XENLOG_G_WARNING "Failed to remove [%lx, 
> >>>>> %lx]: %d\n",
> >>>>>                           start, end, rc);
> >>>>>                    rangeset_destroy(mem);
> >>>>>                    return rc;
> >>>>>                }
> >>>>>            }
> >>>>> +        spin_unlock(&tmp->vpci_lock);
> >>>>>        }
> >>>> At the first glance this simply looks like another unjustified (in the
> >>>> description) change, as you're not converting anything here but you
> >>>> actually add locking (and I realize this was there before, so I'm sorry
> >>>> for not pointing this out earlier).
> >>> Well, I thought that the description already has "...the lock can be
> >>> used (and in a few cases is used right away) to check whether vpci
> >>> is present" and this is enough for such uses as here.
> >>>>    But then I wonder whether you
> >>>> actually tested this, since I can't help getting the impression that
> >>>> you're introducing a live-lock: The function is called from cmd_write()
> >>>> and rom_write(), which in turn are called out of vpci_write(). Yet that
> >>>> function already holds the lock, and the lock is not (currently)
> >>>> recursive. (For the 3rd caller of the function - init_bars() - otoh
> >>>> the locking looks to be entirely unnecessary.)
> >>> Well, you are correct: if tmp != pdev then it is correct to acquire
> >>> the lock. But if tmp == pdev and rom_only == true
> >>> then we'll deadlock.
> >>>
> >>> It seems we need to have the locking conditional, e.g. only lock
> >>> if tmp != pdev
> >> Which will address the live-lock, but introduce ABBA deadlock potential
> >> between the two locks.
> > I am not sure I can suggest a better solution here
> > @Roger, @Jan, could you please help here?
> 
> Well, first of all I'd like to mention that while it may have been okay to
> not hold pcidevs_lock here for Dom0, it surely needs acquiring when dealing
> with DomU-s' lists of PCI devices. The requirement really applies to the
> other use of for_each_pdev() as well (in vpci_dump_msi()), except that
> there it probably wants to be a try-lock.
> 
> Next I'd like to point out that here we have the still pending issue of
> how to deal with hidden devices, which Dom0 can access. See my RFC patch
> "vPCI: account for hidden devices in modify_bars()". Whatever the solution
> here, I think it wants to at least account for the extra need there.

Yes, sorry, I should take care of that.

> Now it is quite clear that pcidevs_lock isn't going to help with avoiding
> the deadlock, as it's imo not an option at all to acquire that lock
> everywhere else you access ->vpci (or else the vpci lock itself would be
> pointless). But a per-domain auxiliary r/w lock may help: Other paths
> would acquire it in read mode, and here you'd acquire it in write mode (in
> the former case around the vpci lock, while in the latter case there may
> then not be any need to acquire the individual vpci locks at all). FTAOD:
> I haven't fully thought through all implications (and hence whether this is
> viable in the first place); I expect you will, documenting what you've
> found in the resulting patch description. Of course the double lock
> acquire/release would then likely want hiding in helper functions.

I've been also thinking about this, and whether it's really worth to
have a per-device lock rather than a per-domain one that protects all
vpci regions of the devices assigned to the domain.

The OS is likely to serialize accesses to the PCI config space anyway,
and the only place I could see a benefit of having per-device locks is
in the handling of MSI-X tables, as the handling of the mask bit is
likely very performance sensitive, so adding a per-domain lock there
could be a bottleneck.

We could alternatively do a per-domain rwlock for vpci and special case
the MSI-X area to also have a per-device specific lock. At which point
it becomes fairly similar to what you propose.

Thanks, Roger.



 


Rackspace

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