Re: [PATCH v5 06/10] iommu/arm-smmu-v3: Add a usage counter for cmdq

[Pranjal Shrivastava <praan@google.com>]

On Wed, Mar 11, 2026 at 03:56:55PM -0700, Daniel Mentz wrote:
> On Wed, Mar 11, 2026 at 6:53 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Tue, Mar 10, 2026 at 09:43:05PM -0700, Daniel Mentz wrote:
> > > Hi Pranjal,
> > >
> > > I have thought about this further and would like to propose an
> > > alternative design.
> > >
> > > A key difference in this design is that no commands are written to the
> > > command queue, and the producer index is not updated while the SMMU is
> > > suspended.
> > >
> > > Similar to the existing CMDQ_PROD_OWNED_FLAG, we could define a new
> > > flag, CMDQ_PROD_STOP_FLAG, stored in cmdq->q.llq.prod.
> > >
> > > In arm_smmu_cmdq_issue_cmdlist():
> > > Perform "old = cmpxchg_relaxed(&cmdq->q.llq.val, llq.val, head.val);".
> > > If CMDQ_PROD_STOP_FLAG is set in the variable named old, return from
> > > arm_smmu_cmdq_issue_cmdlist() immediately.
> > >
> > > In the Suspend handler:
> > >
> > > 1. Clear SMMU_CR0.SMMUEN.
> > > 2. Set CMDQ_PROD_STOP_FLAG in cmdq->q.llq.prod.
> > > 3. Wait until CMDQ_PROD_OWNED_FLAG is cleared from cmdq->q.llq.prod.
> > > 4. Wait until cmdq->owner_prod matches cmdq->q.llq.prod (excluding the
> > > CMDQ_PROD_STOP_FLAG).
> > > 5. Wait until cmdq->lock is 0.
> > > 6. Clear SMMU_CR0.
> > >
> > > In the Resume handler:
> > > Follow the same logic as your patch, but clear CMDQ_PROD_STOP_FLAG in
> > > cmdq->q.llq.prod instead of setting nr_cmdq_users to 1.
> > >
> > > One benefit of this proposal is that it avoids adding a new atomic
> > > variable that multiple CPU threads might compete for. Instead, we
> > > reuse existing atomic operations, which should reduce the performance
> > > impact for server use cases where many CPU threads compete for command
> > > queue access.
> > >
> >
> > I like the idea/direction! Although, there could be a few (solvable)
> > challenges, for example:
> >
> > 1. Abandoned Command Batch
> >   In the SMMUv3 driver, cmdq->q.llq.val is the shared global state in
> >   RAM. The cmpxchg is the "Point of Commitment." Consider the following
> >   sequence of events:
> >
> >    a. Assume STOP_FLAG is already set. Global prod is 10 | STOP_FLAG.
> >    b. CPU-A calls issue_cmdlist. It reads the global state:
> >       llq.val = 10 | STOP_FLAG.
> >    c. CPU-A calculates its new target:
> >         head.prod = 14 | STOP_FLAG | OWNED_FLAG.
> 
> head.prod is calculated like so, and queue_inc_prod_n() will clear the
> CMDQ_PROD_STOP_FLAG.
> 
> head.prod = queue_inc_prod_n(&llq, n + sync) |
>      CMDQ_PROD_OWNED_FLAG;
> 
> You're bringing up a good point, though. This would clear the
> CMDQ_PROD_STOP_FLAG which we don't want.
> 
> >    d. CPU-A executes: old = cmpxchg(&cmdq->q.llq.val, llq.val, head.val)
> >       The swap succeeds because the global state was exactly llq.val.
> >       Global prod is now 14. We have just told the entire system that
> >       there are 14 commands.
> >    e. CPU-A now checks the old variable. It sees STOP_FLAG is set and
> >       returns immediately.
> >
> > CPU-A has "reserved" the indices 10, 11, 12, and 13 in the global
> > producer pointer, but it never wrote the commands to those memory slots.
> > Indices 10–13 now contain garbage/stale data. When the SMMU resumes and
> > processes up to index 14, it will execute that garbage cmd leading to a
> > fault (CERROR_ILL).
> >
> > 2. The OWNED_FLAG Deadlock
> >
> >    a. The CPU who finds OWNED_FLAG is 0 in the cmpxchg becomes the owner
> >       Its job is to gather all concurrent work and update the hardware.
> >    b. Any subsequent CPUs that find OWNED_FLAG is 1 in the cmpxchg. They
> >       write their commands to RAM and then just wait. They rely on the
> >       Owner to "kick" the HW for them & to clear the flag for the next
> >       batch.
> >
> >   The Deadlock Scenario:
> >    a. Suspend Handler: Sets the STOP_FLAG in the global cmdq->q.llq.val.
> >    b. CPU-A (The Owner): Executes cmpxchg. It moves the global prod from
> >       10 to 14. Because it was the first in this batch, it does not set
> >       the OWNED_FLAG.
> 
> Everybody sets the OWNED_FLAG in
> 
> head.prod = queue_inc_prod_n(&llq, n + sync) |
>      CMDQ_PROD_OWNED_FLAG;
> 
> The OWNED_FLAG is always set when any CPU breaks out of that first
> do/while loop in arm_smmu_cmdq_issue_cmdlist.
> 
> >    c. CPU-B (The Follower): Executes cmpxchg. It moves the global prod
> >       from 14 to 18. Because it is joining CPU-A's batch, its cmpxchg
> >       sets the OWNED_FLAG in the global state.
> >    d. Now, CPU-A checks the old value, sees STOP_FLAG, and returns.
> >       CPU-B checks the old value, sees STOP_FLAG, and returns.
> >    e. The Consequence: The global state cmdq->q.llq.prod now has the
> >       OWNED_FLAG set.
> >    f. Suspend Handler: Calls Wait until CMDQ_PROD_OWNED_FLAG is cleared
> >    g. Since the Owner (CPU-A) should clear that flag (it does so at the
> >       very end of the function). The Suspend Handler will spin on this
> >       flag forever.
> >
> > This can be solved by checking the flag before setting the OWNED_FLAG.
> 
> Good point. We should check llq.prod for CMDQ_PROD_STOP_FLAG at the
> beginning of the do/while loop in arm_smmu_cmdq_issue_cmdlist and
> return early if it's set. The function arm_smmu_cmdq_issue_cmdlist
> should never clear CMDQ_PROD_STOP_FLAG.
> 
> > While the atomic counter (nr_cmdq_users) used in v5 adds an tomic, I
> > believe that it provides a cleaner "gate" that prevents CPUs from ever
> > entering the "Commitment" phase once a suspend is imminent (with fixes).
> >
> > However, I agree that the atomic cntr can cause pref drops on servers.
> 
> I think another benefit of my approach is that it avoids the problem
> where a high rate of calls to arm_smmu_cmdq_issue_cmdlist can starve
> the suspend handler. I'm afraid that with the nr_cmdq_users approach,
> we might end up in situations where the suspend handler times out
> because nr_cmdq_users never drops to 1.
>

I agree.

> > I'm okay to adopt Daniel's direction (it only impacts 2 patches) & post
> > a v6. However, given we've been working through this for a year now, I'd
> > want to make sure we have a solid consensus on the design from everyone.
> > I felt with v4 that we have consensus on the design but it still seems
> > to be evolving. I’m ready to post v6 once we settle on the path forward.
> 
> I think the fastest way forward is to post an updated patch right
> away. We can then use that as a baseline for discussions.

Agreed. I'll think through this CMDQ_STOP_FLAG approach (addressing the
issues I've discussed above) and post a new version soon.

Thanks
Praan