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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

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

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

Thanks, Roger.