Re: [PATCH v2 3/4] vpci: use pcidevs locking to protect MMIO handlers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2022 22:33, Volodymyr Babchuk wrote:
> Jan Beulich <jbeulich@xxxxxxxx> writes:
>> On 18.07.2022 23:15, Volodymyr Babchuk wrote:
>>> -            if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
>>> +
>>> +            if ( !pcidevs_read_trylock() )
>>> +                return -EBUSY;
>>> +            pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, 
>>> sbdf.devfn);
>>> +            /*
>>> +             * FIXME: we may find a re-allocated pdev's copy here.
>>> +             * Even occupying the same address as before. Do our best.
>>> +             */
>>> +            if ( !pdev || (pdev != msix->pdev) || !pdev->vpci ||
>>
>> Despite the comment: What guarantees that msix isn't a dangling pointer
>> at this point? At the very least I think you need to check !pdev->vpci
>> first. And I'm afraid I don't view "do our best" as good enough here
>> (considering the patch doesn't carry an RFC tag). And no, I don't have
>> any good suggestion other than "our PCI device locking needs a complete
>> overhaul". Quite likely what we need is a refcounter per device, which
>> - as long as non-zero - prevents removal.
> 
> Refcounter itself is a good idea, but I'm not liking where all this
> goes. We already are reworking locking by adding rw-locks with counters,
> adding refcounter on top of this will complicate things even further.

I'm of quite the opposite opinion: A lot of the places will no longer
need to hold the pcidevs lock when instead they hold a reference; the
lock will only be needed to acquire a reference. Therefore refcounting
is likely to simplify things, presumably to the point where at least
recursive locking (and probably also converting to some r/w locking
scheme) won't be necessary. The main complicating factor is that all
places where a reference is needed will have to be located, and (quite
obviously I'm inclined to say) in particular all involved error paths
will need to be covered when it comes to dropping references no longer
needed.

> I'm starting to think that complete PCI device locking rework may be
> simpler solution, actually. By any chance, were there any prior
> discussion on how proper locking should look like? 

Well, there were prior discussions (you'd need to search the list, as
I have no pointers to hand), but I'm not sure a clear picture had
surfaced how "proper locking" would look like. I guess that's part of
the reason why the currently proposed locking model actually makes
things quite a bit more complicated.

Jan