Re: [PATCH] iommu/io-pgtable-arm: Optimize partial walk flush for large scatter-gather list

[Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>]

Hi Robin,

On 2021-06-10 20:59, Robin Murphy wrote:
> On 2021-06-10 12:54, Sai Prakash Ranjan wrote:
>> Hi Robin,
>> 
>> On 2021-06-10 17:03, Robin Murphy wrote:
>>> On 2021-06-10 10:36, Sai Prakash Ranjan wrote:
>>>> Hi Robin,
>>>> 
>>>> On 2021-06-10 14:38, Robin Murphy wrote:
>>>>> On 2021-06-10 06:24, Sai Prakash Ranjan wrote:
>>>>>> Hi Robin,
>>>>>> 
>>>>>> On 2021-06-10 00:14, Robin Murphy wrote:
>>>>>>> On 2021-06-09 15:53, Sai Prakash Ranjan wrote:
>>>>>>>> Currently for iommu_unmap() of large scatter-gather list with 
>>>>>>>> page size
>>>>>>>> elements, the majority of time is spent in flushing of partial 
>>>>>>>> walks in
>>>>>>>> __arm_lpae_unmap() which is a VA based TLB invalidation (TLBIVA 
>>>>>>>> for
>>>>>>>> arm-smmu).
>>>>>>>> 
>>>>>>>> For example: to unmap a 32MB scatter-gather list with page size 
>>>>>>>> elements
>>>>>>>> (8192 entries), there are 16->2MB buffer unmaps based on the 
>>>>>>>> pgsize (2MB
>>>>>>>> for 4K granule) and each of 2MB will further result in 512 
>>>>>>>> TLBIVAs (2MB/4K)
>>>>>>>> resulting in a total of 8192 TLBIVAs (512*16) for 16->2MB 
>>>>>>>> causing a huge
>>>>>>>> overhead.
>>>>>>>> 
>>>>>>>> So instead use io_pgtable_tlb_flush_all() to invalidate the 
>>>>>>>> entire context
>>>>>>>> if size (pgsize) is greater than the granule size (4K, 16K, 
>>>>>>>> 64K). For this
>>>>>>>> example of 32MB scatter-gather list unmap, this results in just 
>>>>>>>> 16 ASID
>>>>>>>> based TLB invalidations or tlb_flush_all() callback (TLBIASID in 
>>>>>>>> case of
>>>>>>>> arm-smmu) as opposed to 8192 TLBIVAs thereby increasing the 
>>>>>>>> performance of
>>>>>>>> unmaps drastically.
>>>>>>>> 
>>>>>>>> Condition (size > granule size) is chosen for 
>>>>>>>> io_pgtable_tlb_flush_all()
>>>>>>>> because for any granule with supported pgsizes, we will have at 
>>>>>>>> least 512
>>>>>>>> TLB invalidations for which tlb_flush_all() is already 
>>>>>>>> recommended. For
>>>>>>>> example, take 4K granule with 2MB pgsize, this will result in 
>>>>>>>> 512 TLBIVA
>>>>>>>> in partial walk flush.
>>>>>>>> 
>>>>>>>> Test on QTI SM8150 SoC for 10 iterations of 
>>>>>>>> iommu_{map_sg}/unmap:
>>>>>>>> (average over 10 iterations)
>>>>>>>> 
>>>>>>>> Before this optimization:
>>>>>>>> 
>>>>>>>>      size        iommu_map_sg      iommu_unmap
>>>>>>>>        4K            2.067 us         1.854 us
>>>>>>>>       64K            9.598 us         8.802 us
>>>>>>>>        1M          148.890 us       130.718 us
>>>>>>>>        2M          305.864 us        67.291 us
>>>>>>>>       12M         1793.604 us       390.838 us
>>>>>>>>       16M         2386.848 us       518.187 us
>>>>>>>>       24M         3563.296 us       775.989 us
>>>>>>>>       32M         4747.171 us      1033.364 us
>>>>>>>> 
>>>>>>>> After this optimization:
>>>>>>>> 
>>>>>>>>      size        iommu_map_sg      iommu_unmap
>>>>>>>>        4K            1.723 us         1.765 us
>>>>>>>>       64K            9.880 us         8.869 us
>>>>>>>>        1M          155.364 us       135.223 us
>>>>>>>>        2M          303.906 us         5.385 us
>>>>>>>>       12M         1786.557 us        21.250 us
>>>>>>>>       16M         2391.890 us        27.437 us
>>>>>>>>       24M         3570.895 us        39.937 us
>>>>>>>>       32M         4755.234 us        51.797 us
>>>>>>>> 
>>>>>>>> This is further reduced once the map/unmap_pages() support gets 
>>>>>>>> in which
>>>>>>>> will result in just 1 tlb_flush_all() as opposed to 16 
>>>>>>>> tlb_flush_all().
>>>>>>>> 
>>>>>>>> Signed-off-by: Sai Prakash Ranjan 
>>>>>>>> <saiprakash.ranjan@codeaurora.org>
>>>>>>>> ---
>>>>>>>>   drivers/iommu/io-pgtable-arm.c | 7 +++++--
>>>>>>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>>>> 
>>>>>>>> diff --git a/drivers/iommu/io-pgtable-arm.c 
>>>>>>>> b/drivers/iommu/io-pgtable-arm.c
>>>>>>>> index 87def58e79b5..c3cb9add3179 100644
>>>>>>>> --- a/drivers/iommu/io-pgtable-arm.c
>>>>>>>> +++ b/drivers/iommu/io-pgtable-arm.c
>>>>>>>> @@ -589,8 +589,11 @@ static size_t __arm_lpae_unmap(struct 
>>>>>>>> arm_lpae_io_pgtable *data,
>>>>>>>>             if (!iopte_leaf(pte, lvl, iop->fmt)) {
>>>>>>>>               /* Also flush any partial walks */
>>>>>>>> -            io_pgtable_tlb_flush_walk(iop, iova, size,
>>>>>>>> -                          ARM_LPAE_GRANULE(data));
>>>>>>>> +            if (size > ARM_LPAE_GRANULE(data))
>>>>>>>> +                io_pgtable_tlb_flush_all(iop);
>>>>>>>> +            else
>>>>>>> 
>>>>>>> Erm, when will the above condition ever not be true? ;)
>>>>>>> 
>>>>>> 
>>>>>> Ah right, silly me :)
>>>>>> 
>>>>>>> Taking a step back, though, what about the impact to drivers 
>>>>>>> other
>>>>>>> than SMMUv2?
>>>>>> 
>>>>>> Other drivers would be msm_iommu.c, qcom_iommu.c which does the 
>>>>>> same
>>>>>> thing as arm-smmu-v2 (page based invalidations), then there is 
>>>>>> ipmmu-vmsa.c
>>>>>> which does tlb_flush_all() for flush walk.
>>>>>> 
>>>>>>> In particular I'm thinking of SMMUv3.2 where the whole
>>>>>>> range can be invalidated by VA in a single command anyway, so the
>>>>>>> additional penalties of TLBIALL are undesirable.
>>>>>>> 
>>>>>> 
>>>>>> Right, so I am thinking we can have a new generic quirk 
>>>>>> IO_PGTABLE_QUIRK_RANGE_INV
>>>>>> to choose between range based invalidations(tlb_flush_walk) and 
>>>>>> tlb_flush_all().
>>>>>> In this case of arm-smmu-v3.2, we can tie up 
>>>>>> ARM_SMMU_FEAT_RANGE_INV with this quirk
>>>>>> and have something like below, thoughts?
>>>>>> 
>>>>>> if (iop->cfg.quirks & IO_PGTABLE_QUIRK_RANGE_INV)
>>>>>>          io_pgtable_tlb_flush_walk(iop, iova, size,
>>>>>>                                    ARM_LPAE_GRANULE(data));
>>>>>> else
>>>>>>          io_pgtable_tlb_flush_all(iop);
>>>>> 
>>>>> The design here has always been that io-pgtable says *what* needs
>>>>> invalidating, and we left it up to the drivers to decide exactly
>>>>> *how*. Even though things have evolved a bit I don't think that has
>>>>> fundamentally changed - tlb_flush_walk is now only used in this one
>>>>> place (technically I suppose it could be renamed tlb_flush_table 
>>>>> but
>>>>> it's not worth the churn), so drivers can implement their own
>>>>> preferred table-invalidating behaviour even more easily than 
>>>>> choosing
>>>>> whether to bounce a quirk through the common code or not. Consider
>>>>> what you've already seen for the Renesas IPMMU, or SMMUv1 stage 
>>>>> 2...
>>>>> 
>>>> 
>>>> Thanks for the explanation, makes sense. If I am not mistaken, I see 
>>>> that
>>>> you are suggesting to move this logic based on size and granule-size 
>>>> to
>>>> arm-smmu-v2 driver and one more thing below..
>>> 
>>> Simpler than that - following on from my original comment above,
>>> tlb_flush_walk already knows it's invalidating at least one full 
>>> level
>>> of table so there's nothing it even needs to check. Adding a
>>> size-based heuristic to arm_smmu_inv_range_* for leaf invalidations
>>> would be a separate concern (note that changing the non-leaf 
>>> behaviour
>>> might allow cleaning up the "reg" indirection there too).
>> 
>> Right, sorry I didn't mean to mention the size check as it was obvious
>> from your first reply, but rather just calling impl->tlb_inv() in
>> arm_smmu_tlb_inv_walk_s1().
>> 
>>> 
>>>>> I'm instinctively a little twitchy about making this a blanket
>>>>> optimisation for SMMUv2 since I still remember the palaver with our
>>>>> display and MMU-500 integrations, where it had to implement the 
>>>>> dodgy
>>>>> "prefetch" register to trigger translations before scanning out a
>>>>> frame since it couldn't ever afford a TLB miss, thus TLBIALL when
>>>>> freeing an old buffer would be a dangerous hammer to swing. However
>>>>> IIRC it also had to ensure everything was mapped as 2MB blocks to
>>>>> guarantee fitting everything in the TLBs in the first place, so I
>>>>> guess it would still work out OK due to never realistically 
>>>>> unmapping
>>>>> a whole table at once anyway.
>>>>> 
>>>> 
>>>> You are also hinting to not do this for all SMMUv2 implementations 
>>>> and make
>>>> it QCOM specific?
>>> 
>>> No, I'm really just wary that the performance implication is more
>>> complex than a simple unmap latency benefit, possibly even for QCOM.
>>> Consider the access latency, power and memory bandwidth hit from all
>>> the additional pagetable walks incurred by other ongoing traffic
>>> fighting against those 16 successive TLBIASIDs. Whether it's an
>>> overall win really depends on the specific workload and system
>>> conditions as much as the SMMU implementation.
>> 
>> No, the unmap latency is not just in some test case written, the issue
>> is very real and we have workloads where camera is reporting frame 
>> drops
>> because of this unmap latency in the order of 100s of milliseconds.
>> And hardware team recommends using ASID based invalidations for 
>> anything
>> larger than 128 TLB entries. So yes, we have taken note of impacts 
>> here
>> before going this way and hence feel more inclined to make this qcom
>> specific if required.
> 
> OK, that's good to know. I never suggested that CPU unmap latency
> wasn't a valid concern in itself - obviously spending millions of
> cycles in, say, an interrupt handler doing pointless busy work has
> some serious downsides - just that it might not always be the most
> important concern for everyone, so I wanted to make sure this
> discussion was had in the open.
> 

Right, my mistake that I missed to mention these details of real world
data in commit text, will add them in next version.

> TBH I *am* inclined to make this a core SMMU driver change provided
> nobody pops up with a strong counter-argument.
> 

Ok that's even better in case it helps others as well.

>>> Thinking some more, I
>>> wonder if the Tegra folks might have an opinion to add here, given
>>> that their multiple-SMMU solution was seemingly about trying to get
>>> enough TLB and pagetable walk bandwidth in the first place?
>>> 
>> 
>> Sure but I do not see how that will help with the unmap latency?
> 
> It won't. However it implies a use-case which is already sensitive to
> translation bandwidth, and thus is somewhat more likely to be
> sensitive to over-invalidation. But even then they also have more to
> gain from reducing the number of MMIO writes that have to be
> duplicated :)
> 

Ah I see, sorry I misunderstood. It's definitely better if this gets
tested on other systems as well.

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation