Re: [PATCH v10 09/18] iommufd/device: Add pasid_attach array to track per-PASID attach

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Mar 20, 2025 at 12:19:33PM -0700, Nicolin Chen wrote:
> > -	attach = igroup->attach;
> > +	attach = xa_cmpxchg(&igroup->pasid_attach, pasid, NULL,
> > +			    XA_ZERO_ENTRY, GFP_KERNEL);
> > +	if (xa_is_err(attach)) {
> > +		rc = xa_err(attach);
> > +		goto err_unlock;
> > +	}
> > +
> >  	if (!attach) {
> >  		attach = kzalloc(sizeof(*attach), GFP_KERNEL);
> 
> Since this is attach() and we do xa_cmpxchg() with an "old=NULL",
> should !attach always be true?

No, all this xa_cmpxchg is a sort of odd way to atomically tell what
is already in the xarray. If there is already an entry then it will
fail the cmpxchange and return the old entry. This gives the 'if
(attach)' condition.

> > -	attach = igroup->attach;
> > +	attach = xa_cmpxchg(&igroup->pasid_attach, pasid, NULL,
> > +			    XA_ZERO_ENTRY, GFP_KERNEL);
> 
> Hmm, this is replace(). Should the "pasid" position in pasid_attach
> be already filled? I mean the old entry shouldn't be "NULL" so as to
> call it a "replace"?

Same xa oddness, the logic is sound but this could probably just be a
xa_load() under the mutex to confirm a non-NULL entry is present.

Jason