[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v7 01/12] vpci: introduce per-domain lock to protect vpci structure
On 23.06.2023 11:26, Volodymyr Babchuk wrote: > Roger Pau Monné <roger.pau@xxxxxxxxxx> writes: >> On Thu, Jun 22, 2023 at 09:17:36PM +0000, Volodymyr Babchuk wrote: >>> Roger Pau Monné <roger.pau@xxxxxxxxxx> writes: >>>> On Wed, Jun 21, 2023 at 10:07:20PM +0000, Volodymyr Babchuk wrote: >>>>> Roger Pau Monné <roger.pau@xxxxxxxxxx> writes: >>>>>> I've made some comments below. >>>>> >>>>> Thank you for the review. I believe that it is a good idea to have a >>>>> per-domain pdev_list lock. See my answers below. >>>> >>>> I think it's important that the lock that protects domain->pdev_list >>>> must be the same that also protects pdev->vpci, or else you might run >>>> into similar ABBA deadlock situations. >>>> >>>> The problem then could be that in vpci_{read,write} you will take the >>>> per-domain pdev lock in read mode in order to get the pdev, and for >>>> writes to the command register or the ROM BAR you would have to >>>> upgrade such lock to a write lock without dropping it, and we don't >>>> have such functionality for rw locks ATM. >>>> >>>> Maybe just re-starting the function knowing that the lock must be >>>> taken in write mode would be a good solution: writes to the command >>>> register will already be slow since they are likely to involve >>>> modifications to the p2m. >>> >>> Looks like modify_bars() is the only cause for this extended lock. I >>> know that this was discussed earlier, but can we rework modify_bars to >>> not iterate over all the pdevs? We can store copy of all enabled BARs in >>> a domain structure, protected by domain->vpci_lock. Something akin to >>> >>> struct vpci_bar { >>> list_head list; >>> struct vpci *vpci; >>> unsigned long start; >>> unsigned long end; >>> bool is_rom; >>> }; >> >> This IMO makes the logic more complicated, as each time a BAR is >> updated we would have to change the cached address and size in two >> different places. It's also duplicated data that takes up memory, and >> there are system with a non-trivial amount of PCI devices and thus >> BARs to track. >> >> I think it's easier to just make the newly introduced per-domain >> rwlock also protect the domain's pdev_list (unless I'm missing >> something). AFAICT it would also simplify locking, as such rwlock >> protecting the domain->pdev_list will avoid you from having to take >> the pcidevs lock in vpci_{read,write} in order to find the device the >> access belongs to. > > In my opinion, this will make the whole locking logic complex, because > we will have one rwlock that protects: > > 1. pdev->vpci > 2. domain->pdev_list The import thing at this stage is to get the granularity down from the global pcidevs lock. What exactly is grouped together is secondary for the moment; it needs writing down clearly in a code comment, of course. Just to be sure I recall things correctly: 1 above is covering only the pointer, not the contents of the pointed to struct? If so, I would agree putting both under the same lock is a little odd, but if that limits lock nesting issues, I'd say so be it. If not, then this lock could simply be declared as covering everything PCI-ish that a domain has in use. Especially in the latter case ... > This is a two semi-related things. I feel that this will be hard to > understand for anyone who is not deeply intimate with the PCI > code. Anyways, I want this patch series to get going, so I believe your > judgment here. How should I name this lock? Taking into account, that > scope is will be more broad, "domain->vpci_rwlock" does not seems as a > good name. ... d->pci_lock would seem quite appropriate. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |