Received-SPF: pass (google.com: domain of linux-kernel+bounces-66084-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Jim Harris <jim.harris@samsung.com>
To: Bjorn Helgaas <helgaas@kernel.org>
CC: Leon Romanovsky <leonro@nvidia.com>, Kuppuswamy Sathyanarayanan
	<sathyanarayanan.kuppuswamy@linux.intel.com>, Bjorn Helgaas
	<bhelgaas@google.com>, "linux-pci@vger.kernel.org"
	<linux-pci@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, Jason Gunthorpe <jgg@nvidia.com>, "Alex
 Williamson" <alex.williamson@redhat.com>, =?iso-8859-1?Q?Pierre_Cr=E9gut?=
	<pierre.cregut@orange.com>
Subject: Re: [PATCH v2 1/2] PCI/IOV: Revert
 "PCI/IOV: Serialize sysfs sriov_numvfs reads vs writes"
Thread-Topic: [PATCH v2 1/2] PCI/IOV: Revert "PCI/IOV: Serialize sysfs
 sriov_numvfs reads vs writes"
Thread-Index: AQHaW7MAtwi2xwjfZkaUZDAOVEJQA7EDbuYAgAHuDQCAAlV9AIAAuocAgACNHQCAAB2nAIAAIYAAgADA4wCAAKQ8gIAADNIAgABVYgA=
Date: Wed, 14 Feb 2024 22:55:37 +0000
Message-ID: <Zc1E6MFaBKvndAD+@ubuntu>
In-Reply-To: <20240214175000.GA1260022@bhelgaas>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <76E6B18DAFFC284592B288B474CBC3ED@ssi.samsung.com>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
CMS-TYPE: 301P
References: <Zczyhya/+454IaQM@ubuntu> <20240214175000.GA1260022@bhelgaas>
	<CGME20240214225538uscas1p2d6502c3bd144e4d90d4506acf4847159@uscas1p2.samsung.com>

On Wed, Feb 14, 2024 at 11:50:00AM -0600, Bjorn Helgaas wrote:
> On Wed, Feb 14, 2024 at 05:04:08PM +0000, Jim Harris wrote:
> > On Wed, Feb 14, 2024 at 09:16:18AM +0200, Leon Romanovsky wrote:
> > > On Tue, Feb 13, 2024 at 01:45:56PM -0600, Bjorn Helgaas wrote:
> > > > On Tue, Feb 13, 2024 at 07:46:02PM +0200, Leon Romanovsky wrote:
> > > > > On Tue, Feb 13, 2024 at 09:59:54AM -0600, Bjorn Helgaas wrote:
> > > > > ...
> > > >=20
> > > > > > I guess that means that if we apply this revert, the problem Pi=
erre
> > > > > > reported will return.  Obviously the deadlock is more important=
 than
> > > > > > the inconsistency Pierre observed, but from the user's point of=
 view
> > > > > > this will look like a regression.
> > > > > >=20
> > > > > > Maybe listening to netlink and then looking at sysfs isn't the
> > > > > > "correct" way to do this, but I don't want to just casually bre=
ak
> > > > > > existing user code.  If we do contemplate doing the revert, at =
the
> > > > > > very least we should include specific details about what the us=
er code
> > > > > > *should* do instead, at the level of the actual commands to use
> > > > > > instead of "ip monitor dev; cat ${path}/device/sriov_numvfs".
> > > > >=20
> > > > > udevadm monitor will do the trick.
> > > > >=20
> > > > > Another possible solution is to refactor the code to make sure th=
at
> > > > > .probe on VFs happens only after sriov_numvfs is updated.
> > > >=20
> > > > I like the idea of refactoring it so as to preserve the existing
> > > > ordering while also fixing the deadlock.
> > >=20
> > > I think something like this will be enough (not tested). It will et t=
he number of VFs
> > > before we make VFs visible to probe:
> >=20
> > I'll push a v3, replacing the second patch with this one instead. Altho=
ugh
> > based on this discussion it seems we're moving towards squashing the re=
vert
> > with Leon's suggested patch. Bjorn, I'll assume you're still OK with ju=
st
> > squashing these on your end.
>=20
> Yep.
>=20
> > I would like some input on how to actually test this though.
> > Presumably we see some event on device PF and we want to make sure
> > if we read PF/device/sriov_numvfs that we see the updated value. But
> > the only type of event I think we can expect is the PF's
> > sriov_numvfs CHANGE event.
> >=20
> > Is there any way for VFs to be created outside of writing to the
> > sriov_numvfs sysfs file? My understanding is some older
> > devices/drivers will auto-create VFs when the PF is initialized, but
> > it wasn't clear from the bug report whether that was part of the
> > configuration here. Pierre, do you have any recollection on this?
> >=20
> > Or maybe testing for this case just means compile and verify with
> > udevadm monitor that we see the CHANGE event before any of the VFs
> > are actually created...
>=20
> I just want to make sure that Pierre's existing code continues to work
> unchanged.
>=20
> Ideally we could revert 35ff867b7657 ("PCI/IOV: Serialize sysfs
> sriov_numvfs reads vs writes"), reproduce the problem with the shell
> script attached to https://bugzilla.kernel.org/show_bug.cgi?id=3D202991
> (I assume Pierre used a /sys/.../sriov_numvfs write to trigger the
> change).
=20
That shell script generates no output when writing to sriov_numvfs, so I'm
unable to reproduce the problem.

Terminal 1:
# ip monitor dev ens7f0np0

Terminal 2:
# echo 1 > /sys/class/net/ens7f0np0/device/sriov_numvfs
#

No output in terminal 1.

I've done what testing I can with the proposed patch below, I'll send out t=
he
v3 series here shortly.

> Then we could verify that with 35ff867b7657 still reverted but the
> change below added, the problem is no longer reproducible.
>=20
> > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > > index aaa33e8dc4c9..0cdfaae80594 100644
> > > --- a/drivers/pci/iov.c
> > > +++ b/drivers/pci/iov.c
> > > @@ -679,12 +679,14 @@ static int sriov_enable(struct pci_dev *dev, in=
t nr_virtfn)
> > >  	msleep(100);
> > >  	pci_cfg_access_unlock(dev);
> > > =20
> > > +	iov->num_VFs =3D nr_virtfn;
> > >  	rc =3D sriov_add_vfs(dev, initial);
> > > -	if (rc)
> > > +	if (rc) {
> > > +		iov->num_VFs =3D 0;
> > >  		goto err_pcibios;
> > > +	}
> > > =20
> > >  	kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
> > > -	iov->num_VFs =3D nr_virtfn;
> > > =20
> > >  	return 0;
> > >  =