[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 03/13] vpci: move lock outside of struct vpci
On 04.02.2022 13:53, Oleksandr Andrushchenko wrote: > > > On 04.02.22 14:47, Jan Beulich wrote: >> On 04.02.2022 13:37, Oleksandr Andrushchenko wrote: >>> >>> On 04.02.22 13:37, Jan Beulich wrote: >>>> On 04.02.2022 12:13, Roger Pau Monné wrote: >>>>> 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. >>>> Hmm, with method 1 accesses serializing globally is basically >>>> unavoidable, but with MMCFG I see no reason why OSes may not (move >>>> to) permit(ting) parallel accesses, with serialization perhaps done >>>> only at device level. See our own pci_config_lock, which applies to >>>> only method 1 accesses; we don't look to be serializing MMCFG >>>> accesses at all. >>>> >>>>> 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. >>> @Jan, @Roger >>> >>> 1. d->vpci_lock - rwlock <- this protects vpci >>> 2. pdev->vpci->msix_tbl_lock - rwlock <- this protects MSI-X tables >>> or should it better be pdev->msix_tbl_lock as MSI-X tables don't >>> really depend on vPCI? >> If so, perhaps indeed better the latter. But as said in reply to Roger, >> I'm not convinced (yet) that doing away with the per-device lock is a >> good move. As said there - we're ourselves doing fully parallel MMCFG >> accesses, so OSes ought to be fine to do so, too. > But with pdev->vpci_lock we face ABBA... I didn't say without per-domain r/w lock, did I? I stand by my earlier outline. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |