Re: [RFC PATCH] iommu: Optimize IOMMU UnMap

Ashish Mhetre
2024-07-01T07:49:37Z

On 5/31/2024 2:52 PM, Ashish Mhetre wrote:
>
> On 5/24/2024 6:09 PM, Ashish Mhetre wrote:
>>
>> On 5/23/2024 7:11 PM, Robin Murphy wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 23/05/2024 4:19 am, Ashish Mhetre wrote:
>>>> The current __arm_lpae_unmap() function calls dma_sync() on individual
>>>> PTEs after clearing them. By updating the __arm_lpae_unmap() to call
>>>> dma_sync() once for all cleared PTEs, the overall performance can be
>>>> improved 25% for large buffer sizes.
>>>> Below is detailed analysis of average unmap latency(in us) with and
>>>> without this optimization obtained by running dma_map_benchmark for
>>>> different buffer sizes.
>>>>
>>>> Size  Time W/O        Time With       % Improvement
>>>>       Optimization    Optimization
>>>>       (us)            (us)
>>>>
>>>> 4KB   3.0             3.1             -3.33
>>>> 1MB   250.3           187.9           24.93
>>>
>>> This seems highly suspect - the smallest possible block size is 2MB
>>> so a
>>> 1MB unmap should not be affected by this path at all.
>>>
>> It will be unmapped at 4KB block size, right? The 'size' passed to
>> __arm_lpae_unmap will be 4KB and 'pgcount' will be 256 for 1MB
>> buffer from iommu_pgsize() unless the IOVA and phys address met
>> conditions for next bigger size i.e., 2MB.
>>>> 2MB   493.7           368.7 25.32
>>>> 4MB   974.7           723.4           25.78
>>>
>>> I'm guessing this is on Tegra with the workaround to force
>>> everything to
>>> PAGE_SIZE? In the normal case a 2MB unmap should be nominally *faster*
>>> than 4KB, since it would also be a single PTE, but with one fewer level
>>> of table to walk to reach it. The 25% figure is rather misleading if
>>> it's only a mitigation of an existing erratum workaround, and the
>>> actual
>>> impact on the majority of non-broken systems is unmeasured.
>>>
>> Yes, I forgot about the workaround we have and agree that without the
>> workaround, 2MB unmap will be faster without this optimization. But
>> for any size between 4KB and 2MB, this optimization would help in
>> improving the unmap latencies. To verify that, I reverted the workaround
>> and again got unmap latencies using dma_map_benchmark which are as
>> mentioned below. We can see an improvement around 20% to 25%:
>>
>> Size          Time WO Opt(us)     Time With Opt(us)       % improvement
>> 4KB          3                                  3.1
>> -3.33
>> 64KB        18.6                            15 19.36
>> 128KB      35.2                            27.7            21.31
>> 256KB      67.6                            52.6            22.19
>> 512KB      128.4                          97.7           23.91
>> 1MB         249.9                          188.1           24.72
>> 2MB         67.4                             67.5 -0.15
>> 4MB         121.3                          121.2           0.08
>>
>>> (As an aside, I think that workaround itself is a bit broken, since at
>>> least on Tegra234 with Cortex-A78, PAGE_SIZE could be 16KB which
>>> MMU-500
>>> doesn't support.)
>>>
>> Yes, that's true. For 16KB PAGE_SIZE, we need to fall back to 4KB
>> pgsize_bitmap.
>>>> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
>>>> ---
>>>>   drivers/iommu/io-pgtable-arm.c | 34
>>>> +++++++++++++++++++++++++---------
>>>>   1 file changed, 25 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/io-pgtable-arm.c
>>>> b/drivers/iommu/io-pgtable-arm.c
>>>> index 3d23b924cec1..94094b711cba 100644
>>>> --- a/drivers/iommu/io-pgtable-arm.c
>>>> +++ b/drivers/iommu/io-pgtable-arm.c
>>>> @@ -256,13 +256,15 @@ static void
>>>> __arm_lpae_sync_pte(arm_lpae_iopte *ptep, int num_entries,
>>>>                                  sizeof(*ptep) * num_entries,
>>>> DMA_TO_DEVICE);
>>>>   }
>>>>
>>>> -static void __arm_lpae_clear_pte(arm_lpae_iopte *ptep, struct
>>>> io_pgtable_cfg *cfg)
>>>> +static void __arm_lpae_clear_pte(arm_lpae_iopte *ptep, struct
>>>> io_pgtable_cfg *cfg, int num_entries)
>>>>   {
>>>> +     int i;
>>>>
>>>> -     *ptep = 0;
>>>> +     for (i = 0; i < num_entries; i++)
>>>> +             ptep[i] = 0;
>>>>
>>>>       if (!cfg->coherent_walk)
>>>> -             __arm_lpae_sync_pte(ptep, 1, cfg);
>>>> +             __arm_lpae_sync_pte(ptep, num_entries, cfg);
>>>>   }
>>>>
>>>>   static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>>>> @@ -633,13 +635,25 @@ static size_t __arm_lpae_unmap(struct
>>>> arm_lpae_io_pgtable *data,
>>>>       if (size == ARM_LPAE_BLOCK_SIZE(lvl, data)) {
>>>>               max_entries = ARM_LPAE_PTES_PER_TABLE(data) -
>>>> unmap_idx_start;
>>>>               num_entries = min_t(int, pgcount, max_entries);
>>>> -
>>>> -             while (i < num_entries) {
>>>> -                     pte = READ_ONCE(*ptep);
>>>> +             arm_lpae_iopte *pte_flush;
>>>> +             int j = 0;
>>>> +
>>>> +             pte_flush = kvcalloc(num_entries, sizeof(*pte_flush),
>>>> GFP_ATOMIC);
>>>
>>> kvmalloc() with GFP_ATOMIC isn't valid. However, I'm not sure if there
>>> isn't a more fundamental problem here - Rob, Boris; was it just the map
>>> path, or would any allocation on unmap risk the GPU reclaim deadlock
>>> thing as well?
>>>
>> I am using kvmalloc() here to create an array which is used to store
>> PTEs
>> that are going to be flushed after clearing. If we don't store them then
>> those will be lost once cleared and we won't be able to flush them.
>> I tried using GFP_KERNEL instead of GFP_ATOMIC but then I am getting
>> warning from might_sleep().
>> Is there any other alternative way we can use here to store the PTEs?
>>> Thanks,
>>> Robin.
>>>
>>>> +             if (pte_flush) {
>>>> +                     for (j = 0; j < num_entries; j++) {
>>>> +                             pte_flush[j] = READ_ONCE(ptep[j]);
>>>> +                             if (WARN_ON(!pte_flush[j]))
>>>> +                                     break;
>>>> +                     }
>>>> +                     __arm_lpae_clear_pte(ptep, &iop->cfg, j);
>>>> +             }
>>>> +             while (i < (pte_flush ? j : num_entries)) {
>>>> +                     pte = pte_flush ? pte_flush[i] :
>>>> READ_ONCE(*ptep);
>>>>                       if (WARN_ON(!pte))
>>>>                               break;
>>>>
>>>> -                     __arm_lpae_clear_pte(ptep, &iop->cfg);
>>>> +                     if (!pte_flush)
>>>> +                             __arm_lpae_clear_pte(ptep, &iop->cfg,
>>>> 1);
>>>>
>>>>                       if (!iopte_leaf(pte, lvl, iop->fmt)) {
>>>>                               /* Also flush any partial walks */
>>>> @@ -649,10 +663,12 @@ static size_t __arm_lpae_unmap(struct
>>>> arm_lpae_io_pgtable *data,
>>>>                       } else if (!iommu_iotlb_gather_queued(gather)) {
>>>>                               io_pgtable_tlb_add_page(iop, gather,
>>>> iova + i * size, size);
>>>>                       }
>>>> -
>>>> -                     ptep++;
>>>> +                     if (!pte_flush)
>>>> +                             ptep++;
>>>>                       i++;
>>>>               }
>>>> +             if (pte_flush)
>>>> +                     kvfree(pte_flush);
>>>>
>>>>               return i * size;
>>>>       } else if (iopte_leaf(pte, lvl, iop->fmt)) {
> Hi all,
>
> Can you please provide feedback on this patch? Is this optimization
> worth pursuing?
>
> Thanks,
> Ashish Mhetre
Hi Robin,

Can you please share feedback on this? Is more testing required
for this on non-Tegra platforms? Perhaps shall I send it as RFT ?
I have used 'dma_map_benchmark' available in kernel to test this.
Same can be used to test on other platforms.

Thanks and Regards,
Ashish Mhetre