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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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).

Jan