* [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
@ 2017-04-27 11:13 sunil.kovvuri
2017-05-03 13:19 ` Sunil Kovvuri
2017-05-03 15:33 ` Robin Murphy
0 siblings, 2 replies; 9+ messages in thread
From: sunil.kovvuri @ 2017-04-27 11:13 UTC (permalink / raw)
To: will.deacon, robin.murphy, iommu
Cc: linux-kernel, linux-arm-kernel, robert.richter, jcm,
Sunil Goutham, Geetha
From: Sunil Goutham <sgoutham@cavium.com>
Modified polling on CMDQ consumer similar to how polling is done for TLB SYNC
completion in SMMUv2 driver. Code changes are done with reference to
8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
Poll timeout has been increased which addresses issue of 100us timeout not
sufficient, when command queue is full with TLB invalidation commands.
Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
Signed-off-by: Geetha <gakula@cavium.com>
---
drivers/iommu/arm-smmu-v3.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index d412bdd..34599d4 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -379,6 +379,9 @@
#define CMDQ_SYNC_0_CS_NONE (0UL << CMDQ_SYNC_0_CS_SHIFT)
#define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
+#define CMDQ_DRAIN_TIMEOUT_US 1000
+#define CMDQ_SPIN_COUNT 10
+
/* Event queue */
#define EVTQ_ENT_DWORDS 4
#define EVTQ_MAX_SZ_SHIFT 7
@@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
*/
static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
{
- ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
+ ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
+ unsigned int spin_cnt, delay = 1;
while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
if (ktime_compare(ktime_get(), timeout) > 0)
@@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
if (wfe) {
wfe();
} else {
- cpu_relax();
- udelay(1);
+ for (spin_cnt = 0;
+ spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
+ cpu_relax();
+ continue;
+ }
+ udelay(delay);
+ delay *= 2;
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
2017-04-27 11:13 [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively sunil.kovvuri
@ 2017-05-03 13:19 ` Sunil Kovvuri
2017-05-03 15:37 ` Will Deacon
2017-05-03 15:33 ` Robin Murphy
1 sibling, 1 reply; 9+ messages in thread
From: Sunil Kovvuri @ 2017-05-03 13:19 UTC (permalink / raw)
To: Will Deacon, Robin Murphy, iommu
Cc: LKML, LAKML, Robert Richter, jcm, Sunil Goutham, Geetha
On Thu, Apr 27, 2017 at 4:43 PM, <sunil.kovvuri@gmail.com> wrote:
> From: Sunil Goutham <sgoutham@cavium.com>
>
> Modified polling on CMDQ consumer similar to how polling is done for TLB SYNC
> completion in SMMUv2 driver. Code changes are done with reference to
>
> 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
>
> Poll timeout has been increased which addresses issue of 100us timeout not
> sufficient, when command queue is full with TLB invalidation commands.
>
> Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
> Signed-off-by: Geetha <gakula@cavium.com>
> ---
> drivers/iommu/arm-smmu-v3.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index d412bdd..34599d4 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -379,6 +379,9 @@
> #define CMDQ_SYNC_0_CS_NONE (0UL << CMDQ_SYNC_0_CS_SHIFT)
> #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
>
> +#define CMDQ_DRAIN_TIMEOUT_US 1000
> +#define CMDQ_SPIN_COUNT 10
> +
> /* Event queue */
> #define EVTQ_ENT_DWORDS 4
> #define EVTQ_MAX_SZ_SHIFT 7
> @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
> */
> static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> {
> - ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
> + unsigned int spin_cnt, delay = 1;
>
> while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
> if (ktime_compare(ktime_get(), timeout) > 0)
> @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> if (wfe) {
> wfe();
> } else {
> - cpu_relax();
> - udelay(1);
> + for (spin_cnt = 0;
> + spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
> + cpu_relax();
> + continue;
> + }
> + udelay(delay);
> + delay *= 2;
> }
> }
>
> --
> 2.7.4
>
Sorry for the ignorance.
Is there a patchwork where I can check current status of ARM IOMMU
related patches ?
And is this patch accepted, if not any comments / feedback ?
Thanks,
Sunil.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
2017-04-27 11:13 [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively sunil.kovvuri
2017-05-03 13:19 ` Sunil Kovvuri
@ 2017-05-03 15:33 ` Robin Murphy
2017-05-03 15:40 ` Will Deacon
1 sibling, 1 reply; 9+ messages in thread
From: Robin Murphy @ 2017-05-03 15:33 UTC (permalink / raw)
To: sunil.kovvuri, will.deacon, iommu
Cc: linux-kernel, linux-arm-kernel, robert.richter, jcm,
Sunil Goutham, Geetha
On 27/04/17 12:13, sunil.kovvuri@gmail.com wrote:
> From: Sunil Goutham <sgoutham@cavium.com>
>
> Modified polling on CMDQ consumer similar to how polling is done for TLB SYNC
> completion in SMMUv2 driver. Code changes are done with reference to
>
> 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
>
> Poll timeout has been increased which addresses issue of 100us timeout not
> sufficient, when command queue is full with TLB invalidation commands.
>
> Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
> Signed-off-by: Geetha <gakula@cavium.com>
> ---
> drivers/iommu/arm-smmu-v3.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index d412bdd..34599d4 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -379,6 +379,9 @@
> #define CMDQ_SYNC_0_CS_NONE (0UL << CMDQ_SYNC_0_CS_SHIFT)
> #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
>
> +#define CMDQ_DRAIN_TIMEOUT_US 1000
> +#define CMDQ_SPIN_COUNT 10
> +
> /* Event queue */
> #define EVTQ_ENT_DWORDS 4
> #define EVTQ_MAX_SZ_SHIFT 7
> @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
> */
> static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> {
> - ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
> + unsigned int spin_cnt, delay = 1;
>
> while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
> if (ktime_compare(ktime_get(), timeout) > 0)
> @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> if (wfe) {
> wfe();
> } else {
> - cpu_relax();
> - udelay(1);
> + for (spin_cnt = 0;
> + spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
> + cpu_relax();
> + continue;
> + }
> + udelay(delay);
> + delay *= 2;
Sorry, I can't make sense of this. The referenced commit uses the spin
loop to poll opportunistically a few times before delaying. This loop
just adds a short open-coded udelay to an exponential udelay, and it's
not really clear that that's any better than a fixed udelay (especially
as the two cases in which we poll are somewhat different).
What's wrong with simply increasing the timeout value alone?
Robin.
> }
> }
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
2017-05-03 13:19 ` Sunil Kovvuri
@ 2017-05-03 15:37 ` Will Deacon
2017-05-03 15:54 ` Sunil Kovvuri
0 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2017-05-03 15:37 UTC (permalink / raw)
To: Sunil Kovvuri
Cc: Robin Murphy, iommu, LKML, LAKML, Robert Richter, jcm,
Sunil Goutham, Geetha
On Wed, May 03, 2017 at 06:49:09PM +0530, Sunil Kovvuri wrote:
> On Thu, Apr 27, 2017 at 4:43 PM, <sunil.kovvuri@gmail.com> wrote:
> > From: Sunil Goutham <sgoutham@cavium.com>
> >
> > Modified polling on CMDQ consumer similar to how polling is done for TLB SYNC
> > completion in SMMUv2 driver. Code changes are done with reference to
> >
> > 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
> >
> > Poll timeout has been increased which addresses issue of 100us timeout not
> > sufficient, when command queue is full with TLB invalidation commands.
> >
> > Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
> > Signed-off-by: Geetha <gakula@cavium.com>
> > ---
> > drivers/iommu/arm-smmu-v3.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index d412bdd..34599d4 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -379,6 +379,9 @@
> > #define CMDQ_SYNC_0_CS_NONE (0UL << CMDQ_SYNC_0_CS_SHIFT)
> > #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
> >
> > +#define CMDQ_DRAIN_TIMEOUT_US 1000
> > +#define CMDQ_SPIN_COUNT 10
> > +
> > /* Event queue */
> > #define EVTQ_ENT_DWORDS 4
> > #define EVTQ_MAX_SZ_SHIFT 7
> > @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
> > */
> > static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> > {
> > - ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> > + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
> > + unsigned int spin_cnt, delay = 1;
> >
> > while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
> > if (ktime_compare(ktime_get(), timeout) > 0)
> > @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> > if (wfe) {
> > wfe();
> > } else {
> > - cpu_relax();
> > - udelay(1);
> > + for (spin_cnt = 0;
> > + spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
> > + cpu_relax();
> > + continue;
> > + }
> > + udelay(delay);
> > + delay *= 2;
> > }
> > }
> >
> > --
> > 2.7.4
> >
>
> Sorry for the ignorance.
> Is there a patchwork where I can check current status of ARM IOMMU
> related patches ?
>
> And is this patch accepted, if not any comments / feedback ?
Please be patient: the merge window is open and it's not been long since you
posted the patch, which looks pretty bonkers at first glance.
Will
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
2017-05-03 15:33 ` Robin Murphy
@ 2017-05-03 15:40 ` Will Deacon
2017-05-03 16:23 ` Sunil Kovvuri
0 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2017-05-03 15:40 UTC (permalink / raw)
To: Robin Murphy
Cc: sunil.kovvuri, iommu, linux-kernel, linux-arm-kernel,
robert.richter, jcm, Sunil Goutham, Geetha
On Wed, May 03, 2017 at 04:33:57PM +0100, Robin Murphy wrote:
> On 27/04/17 12:13, sunil.kovvuri@gmail.com wrote:
> > From: Sunil Goutham <sgoutham@cavium.com>
> >
> > Modified polling on CMDQ consumer similar to how polling is done for TLB SYNC
> > completion in SMMUv2 driver. Code changes are done with reference to
> >
> > 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
> >
> > Poll timeout has been increased which addresses issue of 100us timeout not
> > sufficient, when command queue is full with TLB invalidation commands.
> >
> > Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
> > Signed-off-by: Geetha <gakula@cavium.com>
> > ---
> > drivers/iommu/arm-smmu-v3.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index d412bdd..34599d4 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -379,6 +379,9 @@
> > #define CMDQ_SYNC_0_CS_NONE (0UL << CMDQ_SYNC_0_CS_SHIFT)
> > #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
> >
> > +#define CMDQ_DRAIN_TIMEOUT_US 1000
> > +#define CMDQ_SPIN_COUNT 10
> > +
> > /* Event queue */
> > #define EVTQ_ENT_DWORDS 4
> > #define EVTQ_MAX_SZ_SHIFT 7
> > @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
> > */
> > static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> > {
> > - ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> > + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
> > + unsigned int spin_cnt, delay = 1;
> >
> > while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
> > if (ktime_compare(ktime_get(), timeout) > 0)
> > @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> > if (wfe) {
> > wfe();
> > } else {
> > - cpu_relax();
> > - udelay(1);
> > + for (spin_cnt = 0;
> > + spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
> > + cpu_relax();
> > + continue;
> > + }
> > + udelay(delay);
> > + delay *= 2;
>
> Sorry, I can't make sense of this. The referenced commit uses the spin
> loop to poll opportunistically a few times before delaying. This loop
> just adds a short open-coded udelay to an exponential udelay, and it's
> not really clear that that's any better than a fixed udelay (especially
> as the two cases in which we poll are somewhat different).
>
> What's wrong with simply increasing the timeout value alone?
I asked that the timeout is only increased for the drain case, and that
we fix the issue here where we udelat if cons didn't move immediately:
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/503389.html
but I don't think the patch above actually achieves any of that.
Will
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
2017-05-03 15:37 ` Will Deacon
@ 2017-05-03 15:54 ` Sunil Kovvuri
2017-05-03 15:59 ` Will Deacon
0 siblings, 1 reply; 9+ messages in thread
From: Sunil Kovvuri @ 2017-05-03 15:54 UTC (permalink / raw)
To: Will Deacon
Cc: Robin Murphy, iommu, LKML, LAKML, Robert Richter, jcm,
Sunil Goutham, Geetha
On Wed, May 3, 2017 at 9:07 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, May 03, 2017 at 06:49:09PM +0530, Sunil Kovvuri wrote:
>> On Thu, Apr 27, 2017 at 4:43 PM, <sunil.kovvuri@gmail.com> wrote:
>> > From: Sunil Goutham <sgoutham@cavium.com>
>> >
>> > Modified polling on CMDQ consumer similar to how polling is done for TLB SYNC
>> > completion in SMMUv2 driver. Code changes are done with reference to
>> >
>> > 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
>> >
>> > Poll timeout has been increased which addresses issue of 100us timeout not
>> > sufficient, when command queue is full with TLB invalidation commands.
>> >
>> > Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
>> > Signed-off-by: Geetha <gakula@cavium.com>
>> > ---
>> > drivers/iommu/arm-smmu-v3.c | 15 ++++++++++++---
>> > 1 file changed, 12 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> > index d412bdd..34599d4 100644
>> > --- a/drivers/iommu/arm-smmu-v3.c
>> > +++ b/drivers/iommu/arm-smmu-v3.c
>> > @@ -379,6 +379,9 @@
>> > #define CMDQ_SYNC_0_CS_NONE (0UL << CMDQ_SYNC_0_CS_SHIFT)
>> > #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
>> >
>> > +#define CMDQ_DRAIN_TIMEOUT_US 1000
>> > +#define CMDQ_SPIN_COUNT 10
>> > +
>> > /* Event queue */
>> > #define EVTQ_ENT_DWORDS 4
>> > #define EVTQ_MAX_SZ_SHIFT 7
>> > @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
>> > */
>> > static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
>> > {
>> > - ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
>> > + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
>> > + unsigned int spin_cnt, delay = 1;
>> >
>> > while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
>> > if (ktime_compare(ktime_get(), timeout) > 0)
>> > @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
>> > if (wfe) {
>> > wfe();
>> > } else {
>> > - cpu_relax();
>> > - udelay(1);
>> > + for (spin_cnt = 0;
>> > + spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
>> > + cpu_relax();
>> > + continue;
>> > + }
>> > + udelay(delay);
>> > + delay *= 2;
>> > }
>> > }
>> >
>> > --
>> > 2.7.4
>> >
>>
>> Sorry for the ignorance.
>> Is there a patchwork where I can check current status of ARM IOMMU
>> related patches ?
>>
>> And is this patch accepted, if not any comments / feedback ?
>
> Please be patient: the merge window is open and it's not been long since you
> posted the patch, which looks pretty bonkers at first glance.
>
> Will
Look at this
https://lkml.org/lkml/2017/4/3/605
The same thing, i pinged after a week and you said you already picked it up.
All I am asking is how do i know the current status, how many days
would normally
be considered being patient ?
Instead of troubling you, is there a patchwork where i can check the status ?
Thanks,
Sunil.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
2017-05-03 15:54 ` Sunil Kovvuri
@ 2017-05-03 15:59 ` Will Deacon
2017-05-03 16:24 ` Sunil Kovvuri
0 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2017-05-03 15:59 UTC (permalink / raw)
To: Sunil Kovvuri
Cc: Robin Murphy, iommu, LKML, LAKML, Robert Richter, jcm,
Sunil Goutham, Geetha
On Wed, May 03, 2017 at 09:24:13PM +0530, Sunil Kovvuri wrote:
> On Wed, May 3, 2017 at 9:07 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Wed, May 03, 2017 at 06:49:09PM +0530, Sunil Kovvuri wrote:
> >> On Thu, Apr 27, 2017 at 4:43 PM, <sunil.kovvuri@gmail.com> wrote:
> >> > From: Sunil Goutham <sgoutham@cavium.com>
> >> >
> >> > Modified polling on CMDQ consumer similar to how polling is done for TLB SYNC
> >> > completion in SMMUv2 driver. Code changes are done with reference to
> >> >
> >> > 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
> >> >
> >> > Poll timeout has been increased which addresses issue of 100us timeout not
> >> > sufficient, when command queue is full with TLB invalidation commands.
> >> >
> >> > Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
> >> > Signed-off-by: Geetha <gakula@cavium.com>
> >> > ---
> >> > drivers/iommu/arm-smmu-v3.c | 15 ++++++++++++---
> >> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> >> > index d412bdd..34599d4 100644
> >> > --- a/drivers/iommu/arm-smmu-v3.c
> >> > +++ b/drivers/iommu/arm-smmu-v3.c
> >> > @@ -379,6 +379,9 @@
> >> > #define CMDQ_SYNC_0_CS_NONE (0UL << CMDQ_SYNC_0_CS_SHIFT)
> >> > #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
> >> >
> >> > +#define CMDQ_DRAIN_TIMEOUT_US 1000
> >> > +#define CMDQ_SPIN_COUNT 10
> >> > +
> >> > /* Event queue */
> >> > #define EVTQ_ENT_DWORDS 4
> >> > #define EVTQ_MAX_SZ_SHIFT 7
> >> > @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
> >> > */
> >> > static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> >> > {
> >> > - ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> >> > + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
> >> > + unsigned int spin_cnt, delay = 1;
> >> >
> >> > while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
> >> > if (ktime_compare(ktime_get(), timeout) > 0)
> >> > @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> >> > if (wfe) {
> >> > wfe();
> >> > } else {
> >> > - cpu_relax();
> >> > - udelay(1);
> >> > + for (spin_cnt = 0;
> >> > + spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
> >> > + cpu_relax();
> >> > + continue;
> >> > + }
> >> > + udelay(delay);
> >> > + delay *= 2;
> >> > }
> >> > }
> >> >
> >> > --
> >> > 2.7.4
> >> >
> >>
> >> Sorry for the ignorance.
> >> Is there a patchwork where I can check current status of ARM IOMMU
> >> related patches ?
> >>
> >> And is this patch accepted, if not any comments / feedback ?
> >
> > Please be patient: the merge window is open and it's not been long since you
> > posted the patch, which looks pretty bonkers at first glance.
> >
> > Will
>
> Look at this
> https://lkml.org/lkml/2017/4/3/605
> The same thing, i pinged after a week and you said you already picked it up.
> All I am asking is how do i know the current status, how many days
> would normally
> be considered being patient ?
At least wait until the merge window is over if it's not a fix, or keep an
eye on the relevant branches (see below).
> Instead of troubling you, is there a patchwork where i can check the status ?
No, but I pick patches up on my iommu/devel branch here:
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/
and at some point they appear on for-joerg/arm-smmu/updates, which I send
to Joerg (who is the iommu maintainer). He then puts them into linux-next
before they get sent for inclusion in mainline during the next merge window.
Will
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
2017-05-03 15:40 ` Will Deacon
@ 2017-05-03 16:23 ` Sunil Kovvuri
0 siblings, 0 replies; 9+ messages in thread
From: Sunil Kovvuri @ 2017-05-03 16:23 UTC (permalink / raw)
To: Will Deacon
Cc: Robin Murphy, iommu, LKML, LAKML, Robert Richter, jcm,
Sunil Goutham, Geetha
On Wed, May 3, 2017 at 9:10 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, May 03, 2017 at 04:33:57PM +0100, Robin Murphy wrote:
>> On 27/04/17 12:13, sunil.kovvuri@gmail.com wrote:
>> > From: Sunil Goutham <sgoutham@cavium.com>
>> >
>> > Modified polling on CMDQ consumer similar to how polling is done for TLB SYNC
>> > completion in SMMUv2 driver. Code changes are done with reference to
>> >
>> > 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
>> >
>> > Poll timeout has been increased which addresses issue of 100us timeout not
>> > sufficient, when command queue is full with TLB invalidation commands.
>> >
>> > Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
>> > Signed-off-by: Geetha <gakula@cavium.com>
>> > ---
>> > drivers/iommu/arm-smmu-v3.c | 15 ++++++++++++---
>> > 1 file changed, 12 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> > index d412bdd..34599d4 100644
>> > --- a/drivers/iommu/arm-smmu-v3.c
>> > +++ b/drivers/iommu/arm-smmu-v3.c
>> > @@ -379,6 +379,9 @@
>> > #define CMDQ_SYNC_0_CS_NONE (0UL << CMDQ_SYNC_0_CS_SHIFT)
>> > #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
>> >
>> > +#define CMDQ_DRAIN_TIMEOUT_US 1000
>> > +#define CMDQ_SPIN_COUNT 10
>> > +
>> > /* Event queue */
>> > #define EVTQ_ENT_DWORDS 4
>> > #define EVTQ_MAX_SZ_SHIFT 7
>> > @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
>> > */
>> > static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
>> > {
>> > - ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
>> > + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
>> > + unsigned int spin_cnt, delay = 1;
>> >
>> > while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
>> > if (ktime_compare(ktime_get(), timeout) > 0)
>> > @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
>> > if (wfe) {
>> > wfe();
>> > } else {
>> > - cpu_relax();
>> > - udelay(1);
>> > + for (spin_cnt = 0;
>> > + spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
>> > + cpu_relax();
>> > + continue;
>> > + }
>> > + udelay(delay);
>> > + delay *= 2;
>>
>> Sorry, I can't make sense of this. The referenced commit uses the spin
>> loop to poll opportunistically a few times before delaying. This loop
>> just adds a short open-coded udelay to an exponential udelay, and it's
>> not really clear that that's any better than a fixed udelay (especially
>> as the two cases in which we poll are somewhat different).
>>
>> What's wrong with simply increasing the timeout value alone?
>
> I asked that the timeout is only increased for the drain case, and that
> we fix the issue here where we udelat if cons didn't move immediately:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/503389.html
>
> but I don't think the patch above actually achieves any of that.
>
> Will
Sorry, I completely screwed up the spin poll above.
Will resubmit.
Thanks,
Sunil.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
2017-05-03 15:59 ` Will Deacon
@ 2017-05-03 16:24 ` Sunil Kovvuri
0 siblings, 0 replies; 9+ messages in thread
From: Sunil Kovvuri @ 2017-05-03 16:24 UTC (permalink / raw)
To: Will Deacon
Cc: Robin Murphy, iommu, LKML, LAKML, Robert Richter, jcm,
Sunil Goutham, Geetha
On Wed, May 3, 2017 at 9:29 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, May 03, 2017 at 09:24:13PM +0530, Sunil Kovvuri wrote:
>> On Wed, May 3, 2017 at 9:07 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Wed, May 03, 2017 at 06:49:09PM +0530, Sunil Kovvuri wrote:
>> >> On Thu, Apr 27, 2017 at 4:43 PM, <sunil.kovvuri@gmail.com> wrote:
>> >> > From: Sunil Goutham <sgoutham@cavium.com>
>> >> >
>> >> > Modified polling on CMDQ consumer similar to how polling is done for TLB SYNC
>> >> > completion in SMMUv2 driver. Code changes are done with reference to
>> >> >
>> >> > 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
>> >> >
>> >> > Poll timeout has been increased which addresses issue of 100us timeout not
>> >> > sufficient, when command queue is full with TLB invalidation commands.
>> >> >
>> >> > Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
>> >> > Signed-off-by: Geetha <gakula@cavium.com>
>> >> > ---
>> >> > drivers/iommu/arm-smmu-v3.c | 15 ++++++++++++---
>> >> > 1 file changed, 12 insertions(+), 3 deletions(-)
>> >> >
>> >> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> >> > index d412bdd..34599d4 100644
>> >> > --- a/drivers/iommu/arm-smmu-v3.c
>> >> > +++ b/drivers/iommu/arm-smmu-v3.c
>> >> > @@ -379,6 +379,9 @@
>> >> > #define CMDQ_SYNC_0_CS_NONE (0UL << CMDQ_SYNC_0_CS_SHIFT)
>> >> > #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
>> >> >
>> >> > +#define CMDQ_DRAIN_TIMEOUT_US 1000
>> >> > +#define CMDQ_SPIN_COUNT 10
>> >> > +
>> >> > /* Event queue */
>> >> > #define EVTQ_ENT_DWORDS 4
>> >> > #define EVTQ_MAX_SZ_SHIFT 7
>> >> > @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
>> >> > */
>> >> > static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
>> >> > {
>> >> > - ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
>> >> > + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
>> >> > + unsigned int spin_cnt, delay = 1;
>> >> >
>> >> > while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
>> >> > if (ktime_compare(ktime_get(), timeout) > 0)
>> >> > @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
>> >> > if (wfe) {
>> >> > wfe();
>> >> > } else {
>> >> > - cpu_relax();
>> >> > - udelay(1);
>> >> > + for (spin_cnt = 0;
>> >> > + spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
>> >> > + cpu_relax();
>> >> > + continue;
>> >> > + }
>> >> > + udelay(delay);
>> >> > + delay *= 2;
>> >> > }
>> >> > }
>> >> >
>> >> > --
>> >> > 2.7.4
>> >> >
>> >>
>> >> Sorry for the ignorance.
>> >> Is there a patchwork where I can check current status of ARM IOMMU
>> >> related patches ?
>> >>
>> >> And is this patch accepted, if not any comments / feedback ?
>> >
>> > Please be patient: the merge window is open and it's not been long since you
>> > posted the patch, which looks pretty bonkers at first glance.
>> >
>> > Will
>>
>> Look at this
>> https://lkml.org/lkml/2017/4/3/605
>> The same thing, i pinged after a week and you said you already picked it up.
>> All I am asking is how do i know the current status, how many days
>> would normally
>> be considered being patient ?
>
> At least wait until the merge window is over if it's not a fix, or keep an
> eye on the relevant branches (see below).
>
>> Instead of troubling you, is there a patchwork where i can check the status ?
>
> No, but I pick patches up on my iommu/devel branch here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/
>
> and at some point they appear on for-joerg/arm-smmu/updates, which I send
> to Joerg (who is the iommu maintainer). He then puts them into linux-next
> before they get sent for inclusion in mainline during the next merge window.
>
> Will
Thanks for the info.
Sunil.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-05-03 16:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-27 11:13 [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively sunil.kovvuri
2017-05-03 13:19 ` Sunil Kovvuri
2017-05-03 15:37 ` Will Deacon
2017-05-03 15:54 ` Sunil Kovvuri
2017-05-03 15:59 ` Will Deacon
2017-05-03 16:24 ` Sunil Kovvuri
2017-05-03 15:33 ` Robin Murphy
2017-05-03 15:40 ` Will Deacon
2017-05-03 16:23 ` Sunil Kovvuri
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).