Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1629738 > unrolled thread

Re: [PATCH] iommu/arm-smmu-v3: Increase SMMU CMD queue poll timeout

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] iommu/arm-smmu-v3: Increase SMMU CMD queue poll timeout Will Deacon <will.deacon@arm.com> - 2017-04-24 18:10 +0200
    Re: [PATCH] iommu/arm-smmu-v3: Increase SMMU CMD queue poll timeout Sunil Kovvuri <sunil.kovvuri@gmail.com> - 2017-04-24 19:00 +0200
      Re: [PATCH] iommu/arm-smmu-v3: Increase SMMU CMD queue poll timeout Will Deacon <will.deacon@arm.com> - 2017-04-24 19:10 +0200
        Re: [PATCH] iommu/arm-smmu-v3: Increase SMMU CMD queue poll timeout Sunil Kovvuri <sunil.kovvuri@gmail.com> - 2017-04-26 11:40 +0200
          Re: [PATCH] iommu/arm-smmu-v3: Increase SMMU CMD queue poll timeout Will Deacon <will.deacon@arm.com> - 2017-04-26 12:00 +0200

#1629738 — Re: [PATCH] iommu/arm-smmu-v3: Increase SMMU CMD queue poll timeout

On Mon, Apr 24, 2017 at 05:29:36PM +0530, Geetha sowjanya wrote:
> From: Geetha <gakula@cavium.com>
> 
> When large memory is being unmapped, huge no of tlb invalidation cmds are
> submitted followed by a SYNC command. This sometimes hits CMD queue full and
> poll on queue drain is being timedout throwing error message 'CMD_SYNC timeout'.
> 
> Although there is no functional issue, error message confuses user. Hence increased
> poll timeout to 500us

Hmm, what are you doing to unmap that much? Is this VFIO teardown? Do you
have 7c6d90e2bb1a ("iommu/io-pgtable-arm: Fix iova_to_phys for block
entries") applied?

Will

> 
> Signed-off-by: Geetha <gakula@cavium.com>
> ---
>  drivers/iommu/arm-smmu-v3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 591bb96..1dcd154 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -407,7 +407,7 @@
>  #define PRIQ_1_ADDR_MASK		0xfffffffffffffUL
>  
>  /* High-level queue structures */
> -#define ARM_SMMU_POLL_TIMEOUT_US	100
> +#define ARM_SMMU_POLL_TIMEOUT_US	500
>  
>  #define MSI_IOVA_BASE			0x8000000
>  #define MSI_IOVA_LENGTH			0x100000
> -- 
> 1.9.1
> 

[toc] | [next] | [standalone]

#1629784

On Mon, Apr 24, 2017 at 9:38 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Apr 24, 2017 at 05:29:36PM +0530, Geetha sowjanya wrote:
>> From: Geetha <gakula@cavium.com>
>>
>> When large memory is being unmapped, huge no of tlb invalidation cmds are
>> submitted followed by a SYNC command. This sometimes hits CMD queue full and
>> poll on queue drain is being timedout throwing error message 'CMD_SYNC timeout'.
>>
>> Although there is no functional issue, error message confuses user. Hence increased
>> poll timeout to 500us
>
> Hmm, what are you doing to unmap that much? Is this VFIO teardown? Do you
> have 7c6d90e2bb1a ("iommu/io-pgtable-arm: Fix iova_to_phys for block
> entries") applied?

Yes it's VFIO teardown and again yes the above fix is applied.
But i didn't get how above fix is related.
TLB invalidation commands are submitted at 'arm_smmu_tlb_inv_range_nosync()'
and it's a loop over granule size.

1357         do {
1358                 arm_smmu_cmdq_issue_cmd(smmu, &cmd);
1359                 cmd.tlbi.addr += granule;
1360         } while (size -= granule);

So if invalidation size is big then huge no of invalidation commands
will be submitted
irrespective of fix that you pointed above, right ?

Thanks,
Sunil.

>
> Will
>
>>
>> Signed-off-by: Geetha <gakula@cavium.com>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 591bb96..1dcd154 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -407,7 +407,7 @@
>>  #define PRIQ_1_ADDR_MASK             0xfffffffffffffUL
>>
>>  /* High-level queue structures */
>> -#define ARM_SMMU_POLL_TIMEOUT_US     100
>> +#define ARM_SMMU_POLL_TIMEOUT_US     500
>>
>>  #define MSI_IOVA_BASE                        0x8000000
>>  #define MSI_IOVA_LENGTH                      0x100000
>> --
>> 1.9.1
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[toc] | [prev] | [next] | [standalone]

#1629793

On Mon, Apr 24, 2017 at 10:26:53PM +0530, Sunil Kovvuri wrote:
> On Mon, Apr 24, 2017 at 9:38 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Mon, Apr 24, 2017 at 05:29:36PM +0530, Geetha sowjanya wrote:
> >> From: Geetha <gakula@cavium.com>
> >>
> >> When large memory is being unmapped, huge no of tlb invalidation cmds are
> >> submitted followed by a SYNC command. This sometimes hits CMD queue full and
> >> poll on queue drain is being timedout throwing error message 'CMD_SYNC timeout'.
> >>
> >> Although there is no functional issue, error message confuses user. Hence increased
> >> poll timeout to 500us
> >
> > Hmm, what are you doing to unmap that much? Is this VFIO teardown? Do you
> > have 7c6d90e2bb1a ("iommu/io-pgtable-arm: Fix iova_to_phys for block
> > entries") applied?
> 
> Yes it's VFIO teardown and again yes the above fix is applied.
> But i didn't get how above fix is related.
> TLB invalidation commands are submitted at 'arm_smmu_tlb_inv_range_nosync()'
> and it's a loop over granule size.
> 
> 1357         do {
> 1358                 arm_smmu_cmdq_issue_cmd(smmu, &cmd);
> 1359                 cmd.tlbi.addr += granule;
> 1360         } while (size -= granule);
> 
> So if invalidation size is big then huge no of invalidation commands
> will be submitted
> irrespective of fix that you pointed above, right ?

VFIO has some logic to batch up invalidations, but this didn't work properly
for us without the fix above. However, I guess you have a huge memory range
that's mapped with 2M sections or something, so there are still loads of
entries to invalidate.

I would much prefer it if VFIO could just teardown the whole address space
so that we could do an invalidate all, but there's a chicken-and-egg problem
with page accounting iirc.

Will

[toc] | [prev] | [next] | [standalone]

#1631282

On Mon, Apr 24, 2017 at 10:35 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Apr 24, 2017 at 10:26:53PM +0530, Sunil Kovvuri wrote:
>> On Mon, Apr 24, 2017 at 9:38 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Mon, Apr 24, 2017 at 05:29:36PM +0530, Geetha sowjanya wrote:
>> >> From: Geetha <gakula@cavium.com>
>> >>
>> >> When large memory is being unmapped, huge no of tlb invalidation cmds are
>> >> submitted followed by a SYNC command. This sometimes hits CMD queue full and
>> >> poll on queue drain is being timedout throwing error message 'CMD_SYNC timeout'.
>> >>
>> >> Although there is no functional issue, error message confuses user. Hence increased
>> >> poll timeout to 500us
>> >
>> > Hmm, what are you doing to unmap that much? Is this VFIO teardown? Do you
>> > have 7c6d90e2bb1a ("iommu/io-pgtable-arm: Fix iova_to_phys for block
>> > entries") applied?
>>
>> Yes it's VFIO teardown and again yes the above fix is applied.
>> But i didn't get how above fix is related.
>> TLB invalidation commands are submitted at 'arm_smmu_tlb_inv_range_nosync()'
>> and it's a loop over granule size.
>>
>> 1357         do {
>> 1358                 arm_smmu_cmdq_issue_cmd(smmu, &cmd);
>> 1359                 cmd.tlbi.addr += granule;
>> 1360         } while (size -= granule);
>>
>> So if invalidation size is big then huge no of invalidation commands
>> will be submitted
>> irrespective of fix that you pointed above, right ?
>
> VFIO has some logic to batch up invalidations, but this didn't work properly
> for us without the fix above. However, I guess you have a huge memory range
> that's mapped with 2M sections or something, so there are still loads of
> entries to invalidate.
>
> I would much prefer it if VFIO could just teardown the whole address space
> so that we could do an invalidate all, but there's a chicken-and-egg problem
> with page accounting iirc.
>

We can definitely look into this from VFIO perspective but for now I am guessing
this patch is fine, as no functionality is being changed.
What do you say ?

Thanks,
Sunil.

[toc] | [prev] | [next] | [standalone]

#1631330

On Wed, Apr 26, 2017 at 02:50:04PM +0530, Sunil Kovvuri wrote:
> On Mon, Apr 24, 2017 at 10:35 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Mon, Apr 24, 2017 at 10:26:53PM +0530, Sunil Kovvuri wrote:
> >> On Mon, Apr 24, 2017 at 9:38 PM, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Mon, Apr 24, 2017 at 05:29:36PM +0530, Geetha sowjanya wrote:
> >> >> From: Geetha <gakula@cavium.com>
> >> >>
> >> >> When large memory is being unmapped, huge no of tlb invalidation cmds are
> >> >> submitted followed by a SYNC command. This sometimes hits CMD queue full and
> >> >> poll on queue drain is being timedout throwing error message 'CMD_SYNC timeout'.
> >> >>
> >> >> Although there is no functional issue, error message confuses user. Hence increased
> >> >> poll timeout to 500us
> >> >
> >> > Hmm, what are you doing to unmap that much? Is this VFIO teardown? Do you
> >> > have 7c6d90e2bb1a ("iommu/io-pgtable-arm: Fix iova_to_phys for block
> >> > entries") applied?
> >>
> >> Yes it's VFIO teardown and again yes the above fix is applied.
> >> But i didn't get how above fix is related.
> >> TLB invalidation commands are submitted at 'arm_smmu_tlb_inv_range_nosync()'
> >> and it's a loop over granule size.
> >>
> >> 1357         do {
> >> 1358                 arm_smmu_cmdq_issue_cmd(smmu, &cmd);
> >> 1359                 cmd.tlbi.addr += granule;
> >> 1360         } while (size -= granule);
> >>
> >> So if invalidation size is big then huge no of invalidation commands
> >> will be submitted
> >> irrespective of fix that you pointed above, right ?
> >
> > VFIO has some logic to batch up invalidations, but this didn't work properly
> > for us without the fix above. However, I guess you have a huge memory range
> > that's mapped with 2M sections or something, so there are still loads of
> > entries to invalidate.
> >
> > I would much prefer it if VFIO could just teardown the whole address space
> > so that we could do an invalidate all, but there's a chicken-and-egg problem
> > with page accounting iirc.
> >
> 
> We can definitely look into this from VFIO perspective but for now I am guessing
> this patch is fine, as no functionality is being changed.
> What do you say ?

Thinking about it some more, I'd rather we rework the polling loop so that:

1. It's structured more like the arm-smmu.c TLB loop queued for 4.11
   (so we don't udelay(1) if the thing doesn't sync immediately)

2. Have a larger timeout for the drain case, which I think is what you're
   running into. This could even be 1s, like arm-smmu.c.

Will

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web