Received-SPF: pass (google.com: domain of linux-kernel+bounces-63752-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 13 Feb 2024 10:59:45 -0400
From: Jason Gunthorpe <jgg@nvidia.com>
To: Jim Harris <jim.harris@samsung.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>, 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>,
	Alex Williamson <alex.williamson@redhat.com>,
	Pierre =?utf-8?Q?Cr=C3=A9gut?= <pierre.cregut@orange.com>
Subject: Re: [PATCH v2 1/2] PCI/IOV: Revert "PCI/IOV: Serialize sysfs
 sriov_numvfs reads vs writes"
Message-ID: <20240213145945.GA1088888@nvidia.com>
References: <20240211084844.GA805332@unreal>
 <20240212202714.GA1142983@bhelgaas>
 <CGME20240212225904uscas1p19a3d7743607a0abe3627fb8c4829f27b@uscas1p1.samsung.com>
 <ZcqitnWTh+zQ+H4p@ubuntu>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZcqitnWTh+zQ+H4p@ubuntu>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?boRLI4ijYYc+ojZgDPHuBj7YUK5i+JMfx6dEjOXhXX0XBfBJds+NzaytumTr?=
 =?us-ascii?Q?ZDwvVbvSeuzfq+4c5Xq5I5gUbTmvDwIDeqo6Hs93gf2lGf6eWiLZitHcvg+S?=
 =?us-ascii?Q?vk5z1jAQlMuT1N3Sp/dYyQLYeSijwtBkb0OGSqNzMxzYRLdaH5bxp5GGWHI6?=
 =?us-ascii?Q?S97mk7ltP08uJNnx15ZAFhOEoLKYMlDRcQS/chT3vQg4xNCws2b/vAEVXHDb?=
 =?us-ascii?Q?gg0ffOHwr8msiBEqmIW2VvdN6LFHAfH4rcfGyyVnZWz6f3FTVfcCJbxxIe44?=
 =?us-ascii?Q?6PK3OvgxerMQuc2Er47KipG0mGTRrzDMZmFUbTw8pTq7rME53KavMI6r5Sxu?=
 =?us-ascii?Q?D4YxN6xKYUkq/IHa0yLQDlmO8oC5Ix7QJdNTRw4tFkR5q6fQvRVIgafg5oYE?=
 =?us-ascii?Q?Y9a0ceFjyIaQvwPYdOrBG4EffiPF7HuFOBiNLEwz2jYFgFS8ofkkwSe/uJpR?=
 =?us-ascii?Q?BQeNZC0FmYXp1211wS/OP4YX15+es0J75+NjFNCj8ajPRB2oZrtaVEK99qc3?=
 =?us-ascii?Q?lf450RTIaCKEenw9olmziSfsX7V7YZV7Ikl4cKEQ1BCDFrcSKxH9SR5viWl+?=
 =?us-ascii?Q?6kcPYH7DRtIrM+CROcuLmQh9IGInBcP0tjhdcsdIUs4bHDksVTQeX36vyqyD?=
 =?us-ascii?Q?ZFXQl8fCg9Ba5k/3HR+bCPibC8a7iM81csFfUu+C24kNHJgYw8Rh3hrj+Hfz?=
 =?us-ascii?Q?RPTSKmDfhX6dvMnNIgqvVZZLUcpE3sH6lefYI375MeZ1Z89AALUpKgAkK45Z?=
 =?us-ascii?Q?Wnbfwk/l/PhsSoaWBT+7v3ZQG1wsQzfjy2hF1OZefR86n1FRG58BK9DIOMn8?=
 =?us-ascii?Q?Q2L0IbRtmr1URlN0wbhE5GIsfMJfW/durhBoOA/UoyrFVK1PGqg0Qu6miYU9?=
 =?us-ascii?Q?JyqoFqF0yfR7DXAulN7ltlG+xG3qCYgS0nyj6G5XGrlZjBToa5uiEh+USb6i?=
 =?us-ascii?Q?MztZCF1z9fKVEW4AxFH1hDAbavmWezk9kiXFwo+ZC5A3xXbiEpz3gRdpsWhD?=
 =?us-ascii?Q?X3gaXdJi0ct8AFwywjA8moBNXOOtuuvukV1PuvNgU91uxaI5y+Oh89bqaURz?=
 =?us-ascii?Q?83eX5nwhdOVuKqzYE75bTy+xtLaBWyoldSdgrMlOGupZ7cZ62UwlBtnm+34y?=
 =?us-ascii?Q?sKyWlaiMrFPelhepbQbX+CTo9qReCqM5T7jaAgcncgo7i5CKG4DX+w3eSbn0?=
 =?us-ascii?Q?Cy7kDbmuW/swNKAnpu4kThK6acoISQRN85lvVST/5lab137f2rGxgSbMqVEm?=
 =?us-ascii?Q?wFqPoGGfTlcBQRy0tzXNGXNo+Sy8+LTIZO37PxP6MK8OL/w9QC4K3DHtzBFp?=
 =?us-ascii?Q?6CY7DNLaQ6orqccBYwq1ib+7YE9yW0oDsmFbqxlhjDlXl4wxA/BPdYxnSf2U?=
 =?us-ascii?Q?i9ujvAiGOHAmUufMcXmSZ4ncus/J8lRoygK02tvghJpOf+TTsXA1FS0FbBeJ?=
 =?us-ascii?Q?Ai4EmQjH9Gs9foXhKnLkydD4nSfvxb/WlkWWvBw5XZmk8yoItD5lv8GaGIvl?=
 =?us-ascii?Q?tLP3ULJxZeeRCyoEk7rvPC7K+RcNMP9bHyVSwUXSbeouBX04X5vFyBa/Pui7?=
 =?us-ascii?Q?9/VAunV5jLD0trfnpA6ePAMavcTMXmjFDclZILer?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ebc8c1b1-2324-4e04-0271-08dc2ca46b58
X-MS-Exchange-CrossTenant-AuthSource: LV2PR12MB5869.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Feb 2024 14:59:46.8012
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: f6WXGQFb+vx8nsmEPwf5/bj35QClz9874pFQmK79eg08TshFG6NgfO3TjhKPHqMD
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR12MB4541

On Mon, Feb 12, 2024 at 10:59:03PM +0000, Jim Harris wrote:
> On Mon, Feb 12, 2024 at 02:27:14PM -0600, Bjorn Helgaas wrote:
> > On Sun, Feb 11, 2024 at 10:48:44AM +0200, Leon Romanovsky wrote:
> > > On Fri, Feb 09, 2024 at 07:20:28PM -0800, Kuppuswamy Sathyanarayanan wrote:
> > > > On 2/9/24 3:52 PM, Jim Harris wrote:
> > > > > If an SR-IOV enabled device is held by vfio, and the device is removed,
> > > > > vfio will hold device lock and notify userspace of the removal. If
> > > > > userspace reads the sriov_numvfs sysfs entry, that thread will be blocked
> > > > > since sriov_numvfs_show() also tries to acquire the device lock. If that
> > > > > same thread is responsible for releasing the device to vfio, it results in
> > > > > a deadlock.
> > > > >
> > > > > The proper way to detect a change to the num_VFs value is to listen for a
> > > > > sysfs event, not to add a device_lock() on the attribute _show() in the
> > > > > kernel.
> > 
> > The lock was not about detecting a change; Pierre did this:
> > 
> >   ip monitor dev ${DEVICE} | grep --line-buffered "^${id}:" | while read line; do \
> >     cat ${path}/device/sriov_numvfs; \
> > 
> > which I assume works by listening for sysfs events.  The problem was
> > that after the event occurred, the sriov_numvfs read got a stale value
> > (see https://bugzilla.kernel.org/show_bug.cgi?id=202991).
> 
> I don't think 'ip monitor dev' listens for any sysfs events. Or at least if
> I have this running and write values to sriov_numvfs, I don't see any
> output.

The issue is that the sysfs change inadvertently throws out a netlink
event (or udev event, or whatever) and something can observe that
event and then turn around and read the sysfs and observe a sysfs
result that hasn't caught up to the event launch.

The lock fixed this because it held it across the event launch and the
update of the internal state.

> It looks like the original bug report was against v5.0 (matching by dates
> and the patch file attached). In that code, we have:
> 
>     kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
>     iov->num_VFs = nr_virtfn;

This is a udev event, I suspect the ip monitor event was thrown by
driver binding during the VF creation.

Jason