Re: [PATCH v3 2/6] xen: pci: introduce reference counting for pdev

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.03.2023 12:48, Roger Pau Monné wrote:
> On Wed, Mar 29, 2023 at 11:55:26AM +0200, Jan Beulich wrote:
>> On 16.03.2023 17:16, Roger Pau Monné wrote:
>>> On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote:
>>>> Prior to this change, lifetime of pci_dev objects was protected by global
>>>> pcidevs_lock(). Long-term plan is to remove this log, so we need some
>>>                                                    ^ lock
>>>
>>> I wouldn't say remove, as one way or another we need a lock to protect
>>> concurrent accesses.
>>>
>>>> other mechanism to ensure that those objects will not disappear under
>>>> feet of code that access them. Reference counting is a good choice as
>>>> it provides easy to comprehend way to control object lifetime.
>>>>
>>>> This patch adds two new helper functions: pcidev_get() and
>>>> pcidev_put(). pcidev_get() will increase reference counter, while
>>>> pcidev_put() will decrease it, destroying object when counter reaches
>>>> zero.
>>>>
>>>> pcidev_get() should be used only when you already have a valid pointer
>>>> to the object or you are holding lock that protects one of the
>>>> lists (domain, pseg or ats) that store pci_dev structs.
>>>>
>>>> pcidev_get() is rarely used directly, because there already are
>>>> functions that will provide valid pointer to pci_dev struct:
>>>> pci_get_pdev(), pci_get_real_pdev(). They will lock appropriate list,
>>>> find needed object and increase its reference counter before returning
>>>> to the caller.
>>>>
>>>> Naturally, pci_put() should be called after finishing working with a
>>>> received object. This is the reason why this patch have so many
>>>> pcidev_put()s and so little pcidev_get()s: existing calls to
>>>> pci_get_*() functions now will increase reference counter
>>>> automatically, we just need to decrease it back when we finished.
>>>
>>> After looking a bit into this, I would like to ask whether it's been
>>> considered the need to increase the refcount for each use of a pdev.
>>>
>>> For example I would consider the initial alloc_pdev() to take a
>>> refcount, and then pci_remove_device() _must_ be the function that
>>> removes the last refcount, so that it can return -EBUSY otherwise (see
>>> my comment below).
>>
>> I thought I had replied to this, but couldn't find any record thereof;
>> apologies for a possible duplicate.
>>
>> In a get-/put-ref model, much like we have it for domheap pages, the
>> last put should trigger whatever is needed for "freeing" (here:
>> removing) the item. Therefore I think in this new model all
>> PHYSDEVOP_{pci_device_remove,manage_pci_remove} should cause is the
>> dropping of the ref that alloc_pdev() has put in place (plus some
>> marking of the device, so that another PHYSDEVOP_{pci_device_remove,
>> manage_pci_remove} can be properly ignored rather than dropping one
>> ref too many; this marking may then also prevent the obtaining of new
>> references, if such can be arranged for without breaking [cleanup]
>> functionality elsewhere). Whenever the last reference is put, that
>> would trigger the operations that pci_remove_device() presently
>> carries out.
> 
> Right, this all seems sensible.
> 
>>
>> Of course this would mean that if PHYSDEVOP_{pci_device_remove,
>> manage_pci_remove} didn't drop the last reference, it would need to
>> signal this to its caller, for it to be aware that the device is not
>> yet ready for (e.g.) hot-unplug. There'll then also need to be a way
>> for the caller to figure out when that situation has changed (which
>> might be via repeated invocations of the same hypercall sub-op, or
>> some new sub-op).
> 
> Returning -EBUSY and expecting the caller to repeat the call would
> likely be the easier one to implement and likely fine for our
> purposes.  There's a risk that the toolstack/kernel enters an infinite
> loop if there's a dangling extra ref somewhere, but that would be a
> bug anyway.
> 
> So device creation would take a reference, and device assignation would
> take another one.  Devices assigned are safe against removal, so there
> should be no need to take an extra reference in that case.
> 
> There are however a number of cases that use pci_get_pdev(NULL, ...)
> for example, at which point we would need to take an extra reference
> on those cases if the device is not assigned to a domain?

I think in this case a ref should be acquired, and independent of
whether the device is assigned anywhere (or else I expect this would
end up cumbersome for callers, when they need to figure whether to
drop a ref).

> Or would we just keep those under pcidevs_locked regions as-is?

This may be a short-term option, but longer term I think we want to
fully move over (and get rid of the global lock altogether, if at all
possible).

Jan

> (as PHYSDEVOP_{pci_device_remove, manage_pci_remove} will still take
> the pci_lock).
> 
> Thanks, Roger.