Re: [RFC PATCH] iommu: Optimize IOMMU UnMap
From: Ashish Mhetre
Date: Tue Jul 09 2024 - 00:40:02 EST
On 7/3/2024 9:05 PM, Robin Murphy wrote:
External email: Use caution opening links or attachments
On 2024-07-01 8:49 am, Ashish Mhetre wrote:
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@xxxxxxxxxx>
---
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.
Apologies I was slightly mistaken before - I confess I was trying to
remember how the code worked from the patch context alone, and forgot
that this same path is actually used for clearing leaf entries all the
way down to L3 as well as freeing tables. So yes, indeed there should be
something to gain in general from combining the syncs for adjacent leaf
entries. However we still have the problem that we can't put an
unconditional allocation in here, so we'd have to do something like
combine up to the next non-leaf entry and keep the "inline" sync for
those. Or perhaps it might end up quite a neat compromise overall to do
your current idea on a smaller scale, with a fixed number of PTEs that's
reasonable to keep on the stack - even in the worst case, I'd expect to
still get a fair boost from doing, say, 32 syncs of 2 cachelines each
vs. 512 that touch each line multiple times.
Thanks,
Robin.
Thanks for the feedback Robin. I am thinking of going with "combining sync
up to next non-leaf entry" approach. At this point there is a very less
chance
of encountering a non-leaf entry as we are entering this part of code after
matching size with BLOCK_SIZE.
Shall I send a new version with this update?
Thanks,
Ashish Mhetre