Re: [RFC PATCH 05/10] xen: pci: introduce reference counting for pdev

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.08.2022 16:11, Volodymyr Babchuk wrote:
> Prior to this change, lifetime of pci_dev objects was protected by global
> pcidevs_lock(). We are going to get if of this lock, so we need some
> 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 with
> better granularity than global super lock.
> 
> 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() and 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.
> 
> This patch removes "const" qualifier from some pdev pointers because
> pcidev_put() technically alters the contents of pci_dev structure.

I wonder if you have so few "get"s because in some cases references
would be needed, but aren't being obtained. As a rule of thumb I'd
expect any entity storing a pointer in a long-lived data structure
to obtain a ref first. And we have quite a few struct fields pointing
to devices. I'd also expect a reference to be held when a device is
e.g. put on a domain's list. This would then likely mean that for
example in deassign_device() (or maybe pci_add_device()) you wouldn't
drop the ref in the success case, but instead the ref would transfer
to the domain the device is added to.

> ---
> 
> - Jan, can I add your Suggested-by tag?

Sure, why not.

Jan