Re: [PATCH] vfio: Create the group chardev outside vfio.group_lock

[Alex Williamson]

On Fri, 11 Sep 2026 18:25:13 +0200
Pavol Sakac <sakacpav@xxxxxxxxx> wrote:

> VFIO holds the global group_lock while allocating, naming, and
> registering each group chardev. cdev_device_add() includes device_add()
> and the KOBJ_ADD uevent, so unrelated group creation is serialized.
> 
> Allocate and name a candidate without the lock, reserve its IOMMU-group
> identity on group_list, then build the chardev unlocked. A contender
> waits for an unpublished reservation and then retries the lookup.
> Keep removal locked through cdev_device_del() so a lookup miss also
> guarantees that the chardev name is free.
> 
> Suppress the ADD event until publication so a failed construction emits
> no uevents.
> 
> The ADD uevent also carries per-event cost (env allocation,
> kobject_get_path()) and a netlink broadcast that serializes globally under
> uevent_sock_mutex; sending it off the lock keeps that global section from
> extending vfio.group_lock hold times.
> 
> Under parallel device probing this lock is a top contention source; with
> the chardev built outside it, it disappears from the enable window's
> contention profile entirely.
> 
> Assisted-by: LLM
> Signed-off-by: Pavol Sakac <sakacpav@xxxxxxxxx>
> ---
> vfio.group_lock is held across cdev_device_add() -- device_add() plus
> the KOBJ_ADD uevent -- so one group's chardev creation serializes every
> unrelated one under the concurrent bring-up of "PCI/IOV: Initialize
> virtual functions in parallel" [1]. The patch reserves the group
> identity on group_list first, then builds the chardev outside the lock:
> three short uncontended holds replace one long contended one.
> 
> Lock statistics and SR-IOV init time for 4x PF (NVMe, 255 VFs each), on
> the reproducer from the parallel VF initialization cover letter [1]:
> 
>   lock_stat:
>   Lock                     wait: Before     After   contentions: Before   After
>   iommu_probe_device_lock       9154 ms  12367 ms                   783     990
>   &vfio.group_lock              3823 ms      0 ms                   730       0
>   &root->kernfs_rwsem           1285 ms   2189 ms                 55459   62799
>   gdp_mutex                        6 ms    314 ms                    23     191
> 
>   vfio.group_lock acquisitions / avg hold  1020 / 378 us -> 3060 / 11 us
> 
>   Removing vfio.group_lock contention lets the released concurrency
>   re-queue on iommu, kernfs and gdp_mutex, none of which this patch
>   touches; the staged sysfs series [2] absorbs most of the kernfs rise.
> 
>   Stage                 SR-IOV init time:
>   S0 (baseline)         3027 ms
>   S1                     999 ms
>   S2                     995 ms
>   S3                     991 ms
>   S4 (this patch)        943 ms
> 
> Reproducer disclaimer:
> I lean primarily on lock_stat numbers to defend the improvements. In
> the reproducer, the residual iommu_probe_device_lock dominates the
> window and masks the later series' wall-time gains; reducing that lock
> further is out of scope for this set. On real hardware the five series
> together cut SR-IOV initialization by 65% [1].
> 
> The lock_stat and timing figures come from the public reproducer. The
> full series has also been tested on current datacenter server hardware
> with thousands of VFs.

I'm having a hard time justifying what this actually does.  We're
optimizing for lock contentions, but we're just redistributing the time
elsewhere and disregarding the actual lock bouncing.  For example the
error path acquires and releases the global group_lock for list removal,
the discard path acquires and releases the group->group_lock even when a
group hasn't been published, and there's another global acquire and
release to set published.  It's not necessarily incorrect, but it
doesn't appear as the right balance of incremental or logical
improvement to base this so heavily on lock_stat alone.

It also seems like there's a bit of code refactoring without locking
changes that could precede this.  Thanks,

Alex

 
> [1] https://lore.kernel.org/r/20260911-vfopt-s1-v1-0-693271dc0226@xxxxxxxxx
> [2] https://lore.kernel.org/r/20260911-vfopt-s5-v1-0-fa4cacdb6ca8@xxxxxxxxx
> 
>  drivers/vfio/group.c | 192 ++++++++++++++++++++++++++++++-------------
>  drivers/vfio/vfio.h  |   9 ++
>  2 files changed, 146 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index b2299e5bc6df..692381151303 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -537,52 +537,157 @@ static struct vfio_group *vfio_group_alloc(struct iommu_group *iommu_group,
>  	group->cdev.owner = THIS_MODULE;
>  
>  	refcount_set(&group->drivers, 1);
> +	init_completion(&group->publish_done);
>  	mutex_init(&group->group_lock);
>  	spin_lock_init(&group->kvm_ref_lock);
>  	INIT_LIST_HEAD(&group->device_list);
>  	mutex_init(&group->device_lock);
>  	group->iommu_group = iommu_group;
> -	/* put in vfio_group_release() */
> +	/* put in vfio_device_remove_group() or vfio_group_discard() */
>  	iommu_group_ref_get(iommu_group);
>  	group->type = type;
>  
>  	return group;
>  }
>  
> -static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
> -		enum vfio_group_type type)
> +/*
> + * Undo vfio_group_alloc() for a never-published group: the teardown tail
> + * of vfio_device_remove_group(), except that unlinking the group from
> + * vfio.group_list is the caller's job, under vfio.group_lock.
> + */
> +static void vfio_group_discard(struct vfio_group *group)
> +{
> +	struct iommu_group *iommu_group;
> +
> +	/*
> +	 * An unpublished group holds only vfio_group_alloc()'s reference.
> +	 * On a count mismatch, leak rather than free under the other holder.
> +	 */
> +	if (WARN_ON(refcount_read(&group->drivers) != 1))
> +		return;
> +	/* No discard site leaves the group findable, so nothing can inc it. */
> +	refcount_set(&group->drivers, 0);
> +
> +	mutex_lock(&group->group_lock);
> +	WARN_ON(!list_empty(&group->device_list));
> +	if (group->container)
> +		vfio_group_detach_container(group);
> +	iommu_group = group->iommu_group;
> +	group->iommu_group = NULL;
> +	mutex_unlock(&group->group_lock);
> +
> +	iommu_group_put(iommu_group);
> +	put_device(&group->dev);
> +}
> +
> +static bool vfio_group_has_device(struct vfio_group *group, struct device *dev)
> +{
> +	struct vfio_device *device;
> +
> +	mutex_lock(&group->device_lock);
> +	list_for_each_entry(device, &group->device_list, group_next) {
> +		if (device->dev == dev) {
> +			mutex_unlock(&group->device_lock);
> +			return true;
> +		}
> +	}
> +	mutex_unlock(&group->device_lock);
> +	return false;
> +}
> +
> +/*
> + * vfio.group_lock is held only to claim the identity: a reserved group is
> + * linked on vfio.group_list before the lock drops, so a lookup miss proves
> + * the chardev name is free and a hit on an unpublished group waits for its
> + * builder.  Allocation, naming, and cdev_device_add() all run unlocked.
> + */
> +static struct vfio_group *
> +vfio_group_find_or_create(struct device *dev, struct iommu_group *iommu_group,
> +			  enum vfio_group_type type)
>  {
>  	struct vfio_group *group;
> -	struct vfio_group *ret;
> +	struct vfio_group *new;
>  	int err;
>  
> -	lockdep_assert_held(&vfio.group_lock);
> -
> -	group = vfio_group_alloc(iommu_group, type);
> -	if (IS_ERR(group))
> +retry:
> +	mutex_lock(&vfio.group_lock);
> +	group = vfio_group_find_from_iommu(iommu_group);
> +	if (group) {
> +		if (!group->published) {
> +			/*
> +			 * Wait unlocked and look up again -- the builder
> +			 * can still fail and unlink the group.  The device
> +			 * reference keeps the completion alive.
> +			 */
> +			get_device(&group->dev);
> +			mutex_unlock(&vfio.group_lock);
> +			while (!wait_for_completion_timeout(&group->publish_done,
> +							    10 * HZ))
> +				dev_warn(dev, "waiting for vfio group %s registration\n",
> +					 dev_name(&group->dev));
> +			put_device(&group->dev);
> +			goto retry;
> +		}
> +		if (WARN_ON(vfio_group_has_device(group, dev)))
> +			group = ERR_PTR(-EINVAL);
> +		else
> +			refcount_inc(&group->drivers);
> +		mutex_unlock(&vfio.group_lock);
>  		return group;
> +	}
> +
> +	mutex_unlock(&vfio.group_lock);
>  
> -	err = dev_set_name(&group->dev, "%s%d",
> -			   group->type == VFIO_NO_IOMMU ? "noiommu-" : "",
> +	new = vfio_group_alloc(iommu_group, type);
> +	if (IS_ERR(new))
> +		return new;
> +	err = dev_set_name(&new->dev, "%s%d",
> +			   new->type == VFIO_NO_IOMMU ? "noiommu-" : "",
>  			   iommu_group_id(iommu_group));
>  	if (err) {
> -		ret = ERR_PTR(err);
> -		goto err_put;
> +		vfio_group_discard(new);
> +		return ERR_PTR(err);
>  	}
>  
> -	err = cdev_device_add(&group->cdev, &group->dev);
> -	if (err) {
> -		ret = ERR_PTR(err);
> -		goto err_put;
> +	mutex_lock(&vfio.group_lock);
> +	if (vfio_group_find_from_iommu(iommu_group)) {
> +		/* Lost the race; drop ours and take theirs. */
> +		mutex_unlock(&vfio.group_lock);
> +		vfio_group_discard(new);
> +		goto retry;
>  	}
> +	list_add(&new->vfio_next, &vfio.group_list);
> +	mutex_unlock(&vfio.group_lock);
>  
> -	list_add(&group->vfio_next, &vfio.group_list);
> +	/*
> +	 * Hold back device_add()'s KOBJ_ADD until publication; on failure,
> +	 * suppression also keeps the device_add() unwind from emitting an
> +	 * unmatched KOBJ_REMOVE.
> +	 */
> +	dev_set_uevent_suppress(&new->dev, true);
> +	err = cdev_device_add(&new->cdev, &new->dev);
> +	if (err) {
> +		mutex_lock(&vfio.group_lock);
> +		list_del(&new->vfio_next);
> +		mutex_unlock(&vfio.group_lock);
> +		complete_all(&new->publish_done);
> +		vfio_group_discard(new);
> +		return ERR_PTR(err);
> +	}
>  
> -	return group;
> +	mutex_lock(&vfio.group_lock);
> +	new->published = true;
> +	mutex_unlock(&vfio.group_lock);
> +	complete_all(&new->publish_done);
>  
> -err_put:
> -	put_device(&group->dev);
> -	return ret;
> +	/*
> +	 * Send the deferred ADD unlocked.  The caller still owns the
> +	 * drivers reference, so vfio_device_remove_group() cannot reach
> +	 * cdev_device_del() before the ADD is sent.
> +	 */
> +	dev_set_uevent_suppress(&new->dev, false);
> +	kobject_uevent(&new->dev.kobj, KOBJ_ADD);
> +	return new;
>  }
>  
>  static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev,
> @@ -603,9 +708,11 @@ static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev,
>  	if (ret)
>  		goto out_put_group;
>  
> -	mutex_lock(&vfio.group_lock);
> -	group = vfio_create_group(iommu_group, type);
> -	mutex_unlock(&vfio.group_lock);
> +	/*
> +	 * The iommu_group is fresh and private, so the lookup and builder
> +	 * wait are unreachable; the shared helper is used for uniformity.
> +	 */
> +	group = vfio_group_find_or_create(dev, iommu_group, type);
>  	if (IS_ERR(group)) {
>  		ret = PTR_ERR(group);
>  		goto out_remove_device;
> @@ -620,21 +727,6 @@ static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev,
>  	return ERR_PTR(ret);
>  }
>  
> -static bool vfio_group_has_device(struct vfio_group *group, struct device *dev)
> -{
> -	struct vfio_device *device;
> -
> -	mutex_lock(&group->device_lock);
> -	list_for_each_entry(device, &group->device_list, group_next) {
> -		if (device->dev == dev) {
> -			mutex_unlock(&group->device_lock);
> -			return true;
> -		}
> -	}
> -	mutex_unlock(&group->device_lock);
> -	return false;
> -}
> -
>  static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
>  {
>  	struct iommu_group *iommu_group;
> @@ -659,17 +751,7 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
>  	if (!iommu_group)
>  		return ERR_PTR(-EINVAL);
>  
> -	mutex_lock(&vfio.group_lock);
> -	group = vfio_group_find_from_iommu(iommu_group);
> -	if (group) {
> -		if (WARN_ON(vfio_group_has_device(group, dev)))
> -			group = ERR_PTR(-EINVAL);
> -		else
> -			refcount_inc(&group->drivers);
> -	} else {
> -		group = vfio_create_group(iommu_group, VFIO_IOMMU);
> -	}
> -	mutex_unlock(&vfio.group_lock);
> +	group = vfio_group_find_or_create(dev, iommu_group, VFIO_IOMMU);
>  
>  	/* The vfio_group holds a reference to the iommu_group */
>  	iommu_group_put(iommu_group);
> @@ -702,16 +784,16 @@ void vfio_device_remove_group(struct vfio_device *device)
>  	if (group->type == VFIO_NO_IOMMU || group->type == VFIO_EMULATED_IOMMU)
>  		iommu_group_remove_device(device->dev);
>  
> -	/* Pairs with vfio_create_group() / vfio_group_get_from_iommu() */
> +	/* Pairs with vfio_group_alloc() / vfio_group_find_or_create() */
>  	if (!refcount_dec_and_mutex_lock(&group->drivers, &vfio.group_lock))
>  		return;
>  	list_del(&group->vfio_next);
>  
>  	/*
> -	 * We could concurrently probe another driver in the group that might
> -	 * race vfio_device_remove_group() with vfio_get_group(), so we have to
> -	 * ensure that the sysfs is all cleaned up under lock otherwise the
> -	 * cdev_device_add() will fail due to the name aready existing.
> +	 * We could concurrently probe another driver in the group racing this
> +	 * removal with vfio_group_find_or_create().  The sysfs name is all
> +	 * cleaned up under the lock, so once a creator's lookup misses, the
> +	 * name is guaranteed free.
>  	 */
>  	cdev_device_del(&group->cdev, &group->dev);
>  
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index 7728bc99b63d..cfc76e5752dd 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -9,6 +9,7 @@
>  #include <linux/file.h>
>  #include <linux/device.h>
>  #include <linux/cdev.h>
> +#include <linux/completion.h>
>  #include <linux/module.h>
>  #include <linux/vfio.h>
>  
> @@ -83,6 +84,14 @@ struct vfio_group {
>  	struct list_head		device_list;
>  	struct mutex			device_lock;
>  	struct list_head		vfio_next;
> +	/*
> +	 * Reserved on vfio.group_list while the chardev is built; published
> +	 * is set when the build succeeds (failure unlinks the group) and is
> +	 * accessed only under vfio.group_lock.  publish_done releases
> +	 * callers that found the group mid-build.
> +	 */
> +	bool				published;
> +	struct completion		publish_done;
>  #if IS_ENABLED(CONFIG_VFIO_CONTAINER)
>  	struct list_head		container_next;
>  #endif
> 
> base-commit: cee9395acd8043be0644b25c34bfa86623f2b935