Re: [PATCH v3 1/5] iommufd: Add iommufd_object_tombstone_user() helper

[Xu Yilun <yilun.xu@linux.intel.com>]

On Mon, Jun 30, 2025 at 05:52:26AM +0000, Tian, Kevin wrote:
> > From: Xu Yilun <yilun.xu@linux.intel.com>
> > Sent: Friday, June 27, 2025 11:38 AM
> >
> > @@ -239,6 +239,7 @@ static int iommufd_fops_release(struct inode *inode,
> > struct file *filp)
> >  	struct iommufd_sw_msi_map *next;
> >  	struct iommufd_sw_msi_map *cur;
> >  	struct iommufd_object *obj;
> > +	bool empty;
> 
> move into for(;;) loop

Yes.

> 
> > 
> >  	/*
> >  	 * The objects in the xarray form a graph of "users" counts, and we
> > have
> > @@ -249,23 +250,35 @@ static int iommufd_fops_release(struct inode
> > *inode, struct file *filp)
> >  	 * until the entire list is destroyed. If this can't progress then there
> >  	 * is some bug related to object refcounting.
> >  	 */
> > -	while (!xa_empty(&ictx->objects)) {
> > +	for (;;) {
> >  		unsigned int destroyed = 0;
> >  		unsigned long index;
> > 
> > +		empty = true;
> >  		xa_for_each(&ictx->objects, index, obj) {
> > +			empty = false;
> >  			if (!refcount_dec_if_one(&obj->users))
> >  				continue;
> > +
> >  			destroyed++;
> >  			xa_erase(&ictx->objects, index);
> >  			iommufd_object_ops[obj->type].destroy(obj);
> >  			kfree(obj);
> >  		}
> > +
> > +		if (empty)
> > +			break;
> > +
> >  		/* Bug related to users refcount */
> >  		if (WARN_ON(!destroyed))
> >  			break;
> >  	}
> > -	WARN_ON(!xa_empty(&ictx->groups));
> 
> I didn't get the rationale of this change. tombstone only means the
> object ID reserved, but all the destroy work for the object has been
> done, e.g. after all idevice objects are destroyed there should be no
> valid igroup entries in ictx->groups (and there is no tombstone
> state for igroup).
> 
> Is it a wrong change by misreading ictx->groups as ictx->objects?

Sorry, this is a true mistake.

> 
> > +
> > +	/*
> > +	 * There may be some tombstones left over from
> > +	 * iommufd_object_tombstone_user()
> > +	 */
> > +	xa_destroy(&ictx->groups);
> > 
> 
> And here should be ictx->objects?

Yes, thanks for catching up.

-       xa_destroy(&ictx->groups);
+       xa_destroy(&ictx->objects);
+
+       WARN_ON(!xa_empty(&ictx->groups));

Thanks,
Yilun