While polling for n spaces in the cmdq, the current code instead checks
if the queue is full. If the queue is almost full but not enough space
(<n), then the CMDQ timeout warning is never triggered even if the
polling has exceeded timeout limit.
This patch polls for the availability of exact space instead of full and
emit timeout warning accordingly.
Fixes: 587e6c10a7ce ("iommu/arm-smmu-v3: Reduce contention during command-queue insertion")
Co-developed-by: Yu Zhang <zhangyu1@linux.microsoft.com>
Signed-off-by: Yu Zhang <zhangyu1@linux.microsoft.com>
Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
---
v2: - Reduced debug print info (Nicolin)
- Use a separate irq flags for exclusive lock
- Handle queue_poll err code other than ETIMEOUT
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 66 ++++++++-------------
1 file changed, 26 insertions(+), 40 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index bf67d9abc901..6959d99c74a3 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -117,12 +117,6 @@ static bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
return space >= n;
}
-static bool queue_full(struct arm_smmu_ll_queue *q)
-{
- return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
- Q_WRP(q, q->prod) != Q_WRP(q, q->cons);
-}
-
static bool queue_empty(struct arm_smmu_ll_queue *q)
{
return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
@@ -612,38 +606,6 @@ static void arm_smmu_cmdq_poll_valid_map(struct arm_smmu_cmdq *cmdq,
__arm_smmu_cmdq_poll_set_valid_map(cmdq, sprod, eprod, false);
}
-/* Wait for the command queue to become non-full */
-static int arm_smmu_cmdq_poll_until_not_full(struct arm_smmu_device *smmu,
- struct arm_smmu_cmdq *cmdq,
- struct arm_smmu_ll_queue *llq)
-{
- unsigned long flags;
- struct arm_smmu_queue_poll qp;
- int ret = 0;
-
- /*
- * Try to update our copy of cons by grabbing exclusive cmdq access. If
- * that fails, spin until somebody else updates it for us.
- */
- if (arm_smmu_cmdq_exclusive_trylock_irqsave(cmdq, flags)) {
- WRITE_ONCE(cmdq->q.llq.cons, readl_relaxed(cmdq->q.cons_reg));
- arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq, flags);
- llq->val = READ_ONCE(cmdq->q.llq.val);
- return 0;
- }
-
- queue_poll_init(smmu, &qp);
- do {
- llq->val = READ_ONCE(cmdq->q.llq.val);
- if (!queue_full(llq))
- break;
-
- ret = queue_poll(&qp);
- } while (!ret);
-
- return ret;
-}
-
/*
* Wait until the SMMU signals a CMD_SYNC completion MSI.
* Must be called with the cmdq lock held in some capacity.
@@ -775,6 +737,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
unsigned long flags;
bool owner;
struct arm_smmu_ll_queue llq, head;
+ struct arm_smmu_queue_poll qp;
int ret = 0;
llq.max_n_shift = cmdq->q.llq.max_n_shift;
@@ -785,10 +748,33 @@ static int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
do {
u64 old;
+ queue_poll_init(smmu, &qp);
while (!queue_has_space(&llq, n + sync)) {
+ unsigned long iflags;
+
local_irq_restore(flags);
- if (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
- dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");
+ /*
+ * Try to update our copy of cons by grabbing exclusive cmdq access. If
+ * that fails, spin until somebody else updates it for us.
+ */
+ if (arm_smmu_cmdq_exclusive_trylock_irqsave(cmdq, iflags)) {
+ WRITE_ONCE(cmdq->q.llq.cons, readl_relaxed(cmdq->q.cons_reg));
+ arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq, iflags);
+ llq.val = READ_ONCE(cmdq->q.llq.val);
+ local_irq_save(flags);
+ continue;
+ }
+
+ ret = queue_poll(&qp);
+ if (ret == -ETIMEDOUT) {
+ dev_err_ratelimited(smmu->dev, "CPU %d CMDQ Timeout, Cons: %08x, Prod: 0x%08x Lock 0x%x\n",
+ smp_processor_id(), llq.cons, llq.prod, atomic_read(&cmdq->lock));
+ queue_poll_init(smmu, &qp);
+ } else if (ret) {
+ dev_err_ratelimited(smmu->dev, "CPU %d CMDQ Poll error %d\n",
+ smp_processor_id(), ret);
+ }
+ llq.val = READ_ONCE(cmdq->q.llq.val);
local_irq_save(flags);
}
--
2.43.0On Mon, Oct 20, 2025 at 03:43:52PM -0700, Jacob Pan wrote:
> @@ -785,10 +748,33 @@ static int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> do {
> u64 old;
>
> + queue_poll_init(smmu, &qp);
> while (!queue_has_space(&llq, n + sync)) {
> + unsigned long iflags;
> +
> local_irq_restore(flags);
> - if (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
> - dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");
> + /*
> + * Try to update our copy of cons by grabbing exclusive cmdq access. If
> + * that fails, spin until somebody else updates it for us.
> + */
> + if (arm_smmu_cmdq_exclusive_trylock_irqsave(cmdq, iflags)) {
> + WRITE_ONCE(cmdq->q.llq.cons, readl_relaxed(cmdq->q.cons_reg));
> + arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq, iflags);
> + llq.val = READ_ONCE(cmdq->q.llq.val);
> + local_irq_save(flags);
I don't quite get the reason why it moves queue_poll_init() and
add local_irq_save(). It's quite different than what the driver
has, so it's nicer to explain in the commit message at least.
I still feel that we could just replace the _until_not_full()
with a _until_has_space()?
Nicolin
Hi Nicolin,
On Thu, 30 Oct 2025 15:41:57 -0700
Nicolin Chen <nicolinc@nvidia.com> wrote:
> On Mon, Oct 20, 2025 at 03:43:52PM -0700, Jacob Pan wrote:
> > @@ -785,10 +748,33 @@ static int arm_smmu_cmdq_issue_cmdlist(struct
> > arm_smmu_device *smmu, do {
> > u64 old;
> >
> > + queue_poll_init(smmu, &qp);
> > while (!queue_has_space(&llq, n + sync)) {
> > + unsigned long iflags;
> > +
> > local_irq_restore(flags);
> > - if
> > (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
> > - dev_err_ratelimited(smmu->dev,
> > "CMDQ timeout\n");
> > + /*
> > + * Try to update our copy of cons by
> > grabbing exclusive cmdq access. If
> > + * that fails, spin until somebody else
> > updates it for us.
> > + */
> > + if
> > (arm_smmu_cmdq_exclusive_trylock_irqsave(cmdq, iflags)) {
> > + WRITE_ONCE(cmdq->q.llq.cons,
> > readl_relaxed(cmdq->q.cons_reg));
> > +
> > arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq, iflags);
> > + llq.val =
> > READ_ONCE(cmdq->q.llq.val);
> > + local_irq_save(flags);
>
> I don't quite get the reason why it moves queue_poll_init() and
> add local_irq_save(). It's quite different than what the driver
> has, so it's nicer to explain in the commit message at least.
Let me add the following to the commit message.
The original code has three nested while loops,
do {
while (!queue_has_space(&llq, n + sync)) {
// inside arm_smmu_cmdq_poll_until_not_full
queue_poll_init(smmu, &qp);
do {
if(!queue_full(llq))
break;
ret = queue_poll(&qp);
}while (!ret);
}
check exit condition
} while (1);
Now, with this patch we reduced to two nested while loops and
calling queue_has_space() only without checking queue_full.
do {
queue_poll_init(smmu, &qp);
while (!queue_has_space(&llq, n + sync)) {
ret = queue_poll(&qp);
if (ret == -ETIMEDOUT) {
dev_err();
queue_poll_init(smmu, &qp);
}
}
check exit condition
} while (1);
An additional queue_poll_init is added outside inner while loop to arm
the timer. We can merge the two queue_poll_init with a local bool
variable to track whether init is needed, but IMHO it is not any better.
Adding local_irq_save() just to make sure it pairs up with
local_irq_restore(), no functional changes.
> I still feel that we could just replace the _until_not_full()
> with a _until_has_space()?
Since the current code uses three nested while loops, replacing the
inner _until_not_full() function means means retaining all three nested
while loops and calling queue_has_space in two places - once in the
middle while loop then again in this _until_has_space() function.
I tried to extract the inner loop into a function but it requires
passing in irqflags to restore. Not pretty.
On Mon, Nov 03, 2025 at 03:16:31PM -0800, Jacob Pan wrote:
> On Thu, 30 Oct 2025 15:41:57 -0700 Nicolin Chen <nicolinc@nvidia.com> wrote:
> > On Mon, Oct 20, 2025 at 03:43:52PM -0700, Jacob Pan wrote:
> > I still feel that we could just replace the _until_not_full()
> > with a _until_has_space()?
> Since the current code uses three nested while loops, replacing the
> inner _until_not_full() function means means retaining all three nested
> while loops and calling queue_has_space in two places - once in the
> middle while loop then again in this _until_has_space() function.
>
> I tried to extract the inner loop into a function but it requires
> passing in irqflags to restore. Not pretty.
I think we could do:
-----------------------------------------------------------------
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 2a8b46b948f05..1211e087dedca 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -138,12 +138,6 @@ static bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
return space >= n;
}
-static bool queue_full(struct arm_smmu_ll_queue *q)
-{
- return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
- Q_WRP(q, q->prod) != Q_WRP(q, q->cons);
-}
-
static bool queue_empty(struct arm_smmu_ll_queue *q)
{
return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
@@ -633,14 +627,13 @@ static void arm_smmu_cmdq_poll_valid_map(struct arm_smmu_cmdq *cmdq,
__arm_smmu_cmdq_poll_set_valid_map(cmdq, sprod, eprod, false);
}
-/* Wait for the command queue to become non-full */
-static int arm_smmu_cmdq_poll_until_not_full(struct arm_smmu_device *smmu,
- struct arm_smmu_cmdq *cmdq,
- struct arm_smmu_ll_queue *llq)
+/* Poll command queue PROD and CONS, using a continued timer */
+static inline void arm_smmu_cmdq_poll(struct arm_smmu_device *smmu,
+ struct arm_smmu_cmdq *cmdq,
+ struct arm_smmu_ll_queue *llq,
+ struct arm_smmu_queue_poll *qp)
{
unsigned long flags;
- struct arm_smmu_queue_poll qp;
- int ret = 0;
/*
* Try to update our copy of cons by grabbing exclusive cmdq access. If
@@ -650,19 +643,18 @@ static int arm_smmu_cmdq_poll_until_not_full(struct arm_smmu_device *smmu,
WRITE_ONCE(cmdq->q.llq.cons, readl_relaxed(cmdq->q.cons_reg));
arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq, flags);
llq->val = READ_ONCE(cmdq->q.llq.val);
- return 0;
+ return;
}
- queue_poll_init(smmu, &qp);
- do {
- llq->val = READ_ONCE(cmdq->q.llq.val);
- if (!queue_full(llq))
- break;
-
- ret = queue_poll(&qp);
- } while (!ret);
-
- return ret;
+ /* queue_poll() returns 0 or -ETIMEDOUT only */
+ if (queue_poll(qp)) {
+ dev_err_ratelimited(smmu->dev,
+ "CMDQ timeout at prod 0x%08x cons 0x%08x\n",
+ llq->prod, llq->cons);
+ /* Restart the timer */
+ queue_poll_init(smmu, qp);
+ }
+ llq->val = READ_ONCE(cmdq->q.llq.val);
}
/*
@@ -804,12 +796,13 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
local_irq_save(flags);
llq.val = READ_ONCE(cmdq->q.llq.val);
do {
+ struct arm_smmu_queue_poll qp;
u64 old;
+ queue_poll_init(smmu, &qp);
while (!queue_has_space(&llq, n + sync)) {
local_irq_restore(flags);
- if (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
- dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");
+ arm_smmu_cmdq_poll(smmu, cmdq, &llq, &qp);
local_irq_save(flags);
}
-----------------------------------------------------------------
And the commit message should point out:
The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit efficiently
nor ideally to the only caller arm_smmu_cmdq_issue_cmdlist():
- It uses a new timer at every single call, which fails to limit to the
preset ARM_SMMU_POLL_TIMEOUT_US per issue.
- It has a redundant internal queue_full(), which doesn't detect whether
there is a enough space for number of n commands.
So, rework it to be an inline helper to work with the queue_has_space().
Nicolin
Hi Nicolin,
On Mon, 3 Nov 2025 17:23:26 -0800
Nicolin Chen <nicolinc@nvidia.com> wrote:
> On Mon, Nov 03, 2025 at 03:16:31PM -0800, Jacob Pan wrote:
> > On Thu, 30 Oct 2025 15:41:57 -0700 Nicolin Chen
> > <nicolinc@nvidia.com> wrote:
> > > On Mon, Oct 20, 2025 at 03:43:52PM -0700, Jacob Pan wrote:
> > > I still feel that we could just replace the _until_not_full()
> > > with a _until_has_space()?
>
> > Since the current code uses three nested while loops, replacing the
> > inner _until_not_full() function means means retaining all three
> > nested while loops and calling queue_has_space in two places - once
> > in the middle while loop then again in this _until_has_space()
> > function.
> >
> > I tried to extract the inner loop into a function but it requires
> > passing in irqflags to restore. Not pretty.
>
> I think we could do:
>
> -----------------------------------------------------------------
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index
> 2a8b46b948f05..1211e087dedca 100644 ---
> a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -138,12 +138,6 @@
> static bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
> return space >= n; }
>
> -static bool queue_full(struct arm_smmu_ll_queue *q)
> -{
> - return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
> - Q_WRP(q, q->prod) != Q_WRP(q, q->cons);
> -}
> -
> static bool queue_empty(struct arm_smmu_ll_queue *q)
> {
> return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
> @@ -633,14 +627,13 @@ static void arm_smmu_cmdq_poll_valid_map(struct
> arm_smmu_cmdq *cmdq, __arm_smmu_cmdq_poll_set_valid_map(cmdq, sprod,
> eprod, false); }
>
> -/* Wait for the command queue to become non-full */
> -static int arm_smmu_cmdq_poll_until_not_full(struct arm_smmu_device
> *smmu,
> - struct arm_smmu_cmdq
> *cmdq,
> - struct
> arm_smmu_ll_queue *llq) +/* Poll command queue PROD and CONS, using a
> continued timer */ +static inline void arm_smmu_cmdq_poll(struct
> arm_smmu_device *smmu,
> + struct arm_smmu_cmdq *cmdq,
> + struct arm_smmu_ll_queue *llq,
> + struct arm_smmu_queue_poll *qp)
> {
> unsigned long flags;
> - struct arm_smmu_queue_poll qp;
> - int ret = 0;
>
> /*
> * Try to update our copy of cons by grabbing exclusive cmdq
> access. If @@ -650,19 +643,18 @@ static int
> arm_smmu_cmdq_poll_until_not_full(struct arm_smmu_device *smmu,
> WRITE_ONCE(cmdq->q.llq.cons, readl_relaxed(cmdq->q.cons_reg));
> arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq, flags); llq->val =
> READ_ONCE(cmdq->q.llq.val);
> - return 0;
> + return;
> }
>
> - queue_poll_init(smmu, &qp);
> - do {
> - llq->val = READ_ONCE(cmdq->q.llq.val);
> - if (!queue_full(llq))
> - break;
> -
> - ret = queue_poll(&qp);
> - } while (!ret);
> -
> - return ret;
> + /* queue_poll() returns 0 or -ETIMEDOUT only */
> + if (queue_poll(qp)) {
I would still prefer more a defensive approach to prevent future change
of queue_poll returning other error being treated as ETIMEOUT.
> + dev_err_ratelimited(smmu->dev,
> + "CMDQ timeout at prod 0x%08x
> cons 0x%08x\n",
> + llq->prod, llq->cons);
> + /* Restart the timer */
> + queue_poll_init(smmu, qp);
> + }
> + llq->val = READ_ONCE(cmdq->q.llq.val);
> }
>
> /*
> @@ -804,12 +796,13 @@ int arm_smmu_cmdq_issue_cmdlist(struct
> arm_smmu_device *smmu, local_irq_save(flags);
> llq.val = READ_ONCE(cmdq->q.llq.val);
> do {
> + struct arm_smmu_queue_poll qp;
> u64 old;
>
> + queue_poll_init(smmu, &qp);
> while (!queue_has_space(&llq, n + sync)) {
> local_irq_restore(flags);
> - if (arm_smmu_cmdq_poll_until_not_full(smmu,
> cmdq, &llq))
> - dev_err_ratelimited(smmu->dev, "CMDQ
> timeout\n");
> + arm_smmu_cmdq_poll(smmu, cmdq, &llq, &qp);
> local_irq_save(flags);
> }
>
yeah, that should work. it is more readable than open coding.
> -----------------------------------------------------------------
>
> And the commit message should point out:
>
> The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit
> efficiently nor ideally to the only caller
> arm_smmu_cmdq_issue_cmdlist():
> - It uses a new timer at every single call, which fails to limit to
> the preset ARM_SMMU_POLL_TIMEOUT_US per issue.
Not following what you mean.
The original code below does honor the timeout of
ARM_SMMU_POLL_TIMEOUT_US
- queue_poll_init(smmu, &qp);
- do {
- llq->val = READ_ONCE(cmdq->q.llq.val);
- if (!queue_full(llq))
- break;
-
- ret = queue_poll(&qp);
- } while (!ret);
> - It has a redundant internal queue_full(), which doesn't detect
> whether there is a enough space for number of n commands.
will incorporate, though the same points already mentioned in the
current commit message.
> So, rework it to be an inline helper to work with the
> queue_has_space().
>
> Nicolin
On Tue, Nov 04, 2025 at 10:25:39AM -0800, Jacob Pan wrote: > > ----------------------------------------------------------------- > > > > And the commit message should point out: > > > > The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit > > efficiently nor ideally to the only caller > > arm_smmu_cmdq_issue_cmdlist(): > > - It uses a new timer at every single call, which fails to limit to > > the preset ARM_SMMU_POLL_TIMEOUT_US per issue. > Not following what you mean. > The original code below does honor the timeout of > ARM_SMMU_POLL_TIMEOUT_US It sets the timeout per arm_smmu_cmdq_poll_until_not_full(), not the entire wait-for-space routine. And that's why you moved the queue_poll_init() to the caller, right? Nicolin
Hi Nicolin, On Tue, 4 Nov 2025 10:48:31 -0800 Nicolin Chen <nicolinc@nvidia.com> wrote: > On Tue, Nov 04, 2025 at 10:25:39AM -0800, Jacob Pan wrote: > > > ----------------------------------------------------------------- > > > > > > And the commit message should point out: > > > > > > The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit > > > efficiently nor ideally to the only caller > > > arm_smmu_cmdq_issue_cmdlist(): > > > - It uses a new timer at every single call, which fails to limit > > > to the preset ARM_SMMU_POLL_TIMEOUT_US per issue. > > > Not following what you mean. > > The original code below does honor the timeout of > > ARM_SMMU_POLL_TIMEOUT_US > > It sets the timeout per arm_smmu_cmdq_poll_until_not_full(), not > the entire wait-for-space routine. And that's why you moved the > queue_poll_init() to the caller, right? > Got you! will do. Thanks, Jacob
© 2016 - 2026 Red Hat, Inc.