Re: [PATCH v7 01/12] vpci: introduce per-domain lock to protect vpci structure

[Jan Beulich <jbeulich@xxxxxxxx>]

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