Messages in this thread | | | Subject | Re: [PATCH] iommu/io-pgtable-arm: Optimize partial walk flush for large scatter-gather list | From | Robin Murphy <> | Date | Thu, 10 Jun 2021 16:29:43 +0100 |
| |
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.
TBH I *am* inclined to make this a core SMMU driver change provided nobody pops up with a strong counter-argument.
>> 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 :)
Thanks, Robin.
| |