On Tue, Dec 12, 2023 at 09:34:43PM +0000, Jim Harris wrote:
> On Mon, Dec 11, 2023 at 09:20:06AM +0200, Leon Romanovsky wrote:
> > 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:
> > > >
> > > > 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
>
> IIUC, the synchronization was to block readers of sriov_numvfs if a writer was
> in process of the driver->sriov_configure(). Presumably sriov_configure()
> can take a long time, and it was better to block the sysfs read rather than
> return a stale value.
Nothing prevents from user to write to sriov_numvfs in one thread and
read from another thread. User will get stale value anyway.
>
> > 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.
>
> But I guess you're saying using the sysfs change as any kind of indicator
> is wrong to begin with.
Yes
>
> > 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);
>
> Ack. I'll post patches for both of these suggestions.
Thanks
