Re: [PATCH 2/2] iommu/arm-smmu-v3: Improve CMDQ lock fairness and efficiency

[Jacob Pan <jacob.pan@linux.microsoft.com>]

On Mon, 6 Oct 2025 18:08:14 -0700
Nicolin Chen <nicolinc@nvidia.com> wrote:

> On Wed, Sep 24, 2025 at 10:54:38AM -0700, Jacob Pan wrote:
> >  static void arm_smmu_cmdq_shared_lock(struct arm_smmu_cmdq *cmdq)
> >  {
> > -	int val;
> > -
> >  	/*
> > -	 * We can try to avoid the cmpxchg() loop by simply
> > incrementing the
> > -	 * lock counter. When held in exclusive state, the lock
> > counter is set
> > -	 * to INT_MIN so these increments won't hurt as the value
> > will remain
> > -	 * negative.
> > +	 * We can simply increment the lock counter. When held in
> > exclusive
> > +	 * state, the lock counter is set to INT_MIN so these
> > increments won't
> > +	 * hurt as the value will remain negative.  
> 
> It seems to me that the change at the first statement is not very
> necessary.
> 
I can delete "We can simply increment the lock counter." since it is
obvious. But the change to delete cmpxchg in the comment matches the
code change the follows.

> > This will also signal the
> > +	 * exclusive locker that there are shared waiters. Once
> > the exclusive
> > +	 * locker releases the lock, the sign bit will be cleared
> > and our
> > +	 * increment will make the lock counter positive, allowing
> > us to
> > +	 * proceed.
> >  	 */
> >  	if (atomic_fetch_inc_relaxed(&cmdq->lock) >= 0)
> >  		return;
> >  
> > -	do {
> > -		val = atomic_cond_read_relaxed(&cmdq->lock, VAL >=
> > 0);
> > -	} while (atomic_cmpxchg_relaxed(&cmdq->lock, val, val + 1)
> > != val);
> > +	atomic_cond_read_relaxed(&cmdq->lock, VAL >= 0);  
> 
> The returned value is not captured for anything. Is this read()
> necessary? If so, a line of comments elaborating it?
We don't need the return value, how about this explanation?
/*
 * Someone else is holding the lock in exclusive state, so wait
 * for them to finish. Since we already incremented the lock counter,
 * no exclusive lock can be acquired until we finish. We don't need
 * the return value since we only care that the exclusive lock is
 * released (i.e. the lock counter is non-negative).
 */
> > +/*
> > + * Only clear the sign bit when releasing the exclusive lock this
> > will
> > + * allow any shared_lock() waiters to proceed without the
> > possibility
> > + * of entering the exclusive lock in a tight loop.
> > + */
> >  #define arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq,
> > flags)		\ ({
> > 				\
> > -	atomic_set_release(&cmdq->lock, 0);
> > 	\
> > +	atomic_fetch_and_release(~INT_MIN, &cmdq->lock);
> > 			\  
> 
> By a quick skim, the whole thing looks quite smart to me. But I
> need some time to revisit and perhaps test it as well.
> 
> It's also important to get feedback from Will. Both patches are
> touching his writing that has been running for years already..
Definitely, really appreciated your review. I think part of the reason
is that cmdq size is usually quite large, queue full is a rare case.