Re: [RFC PATCH] iommu: Optimize IOMMU UnMap

Robin Murphy
2024-05-23T13:41:15Z

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.

> 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.

(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.)

> 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?

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)) {