Re: Locking between vfio hot-remove and pci sysfs sriov_numvfs

[Leon Romanovsky <leonro@nvidia.com>]

On Sun, Dec 10, 2023 at 03:05:49PM -0400, Jason Gunthorpe wrote:
> On Fri, Dec 08, 2023 at 08:09:34PM +0000, Jim Harris wrote:
> > On Fri, Dec 08, 2023 at 03:41:59PM -0400, Jason Gunthorpe wrote:
> > > On Fri, Dec 08, 2023 at 05:07:22PM +0000, Jim Harris wrote:
> > > > 
> > > > Maybe for now we just whack this specific mole with a separate mutex
> > > > for synchronizing access to sriov->num_VFs in the sysfs paths?
> > > > Something like this (tested on my system):
> > > 
> > > TBH, I don't have the time right now to unpack this locking
> > > mystery. Maybe Leon remembers?
> > > 
> > > device_lock() gets everywhere and does a lot of different stuff, so I
> > > would be surprised if it was so easy..
> > 
> > The store() side still keeps the device_lock(), it just also acquires this
> > new sriov lock. So store() side should observe zero differences. The only
> > difference is now the show() side can acquire just the more-granular lock,
> > since it is only trying to synchronize on sriov->num_VFs with the store()
> > side. But maybe I'm missing something subtle here...
> 
> Oh if that is the only goal then probably a READ_ONCE is fine

I would say that worth to revert the patch
35ff867b7657 ("PCI/IOV: Serialize sysfs sriov_numvfs reads vs writes")
as there is no such promise that netdev devices (as presented in script
https://bugzilla.kernel.org/show_bug.cgi?id=202991), which have different
lifetime model will be only after sysfs changes in PF.

netlink event means netdev FOO is ready and if someone needs to follow
after sriov_numvfs, he/she should listen to sysfs events.

In addition, I would do this change:

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 25dbe85c4217..3b768e20c7ab 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -683,8 +683,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
        if (rc)
                goto err_pcibios;

-       kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
        iov->num_VFs = nr_virtfn;
+       kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);

        return 0;



Thanks

> 
> Jason
>