| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d854664c-ab56-46b6-81f9-9ef712c32b3a@nvidia.com>
Date: Fri, 31 May 2024 14:52:55 +0530
From: Ashish Mhetre <amhetre@...dia.com>
To: Robin Murphy <robin.murphy@....com>, will@...nel.org, joro@...tes.org,
linux-arm-kernel@...ts.infradead.org, Rob Clark <robdclark@...il.com>,
Boris Brezillon <boris.brezillon@...labora.com>
Cc: vdumpa@...dia.com, linux-tegra@...r.kernel.org, treding@...dia.com,
jonathanh@...dia.com, iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] iommu: Optimize IOMMU UnMap
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@...dia.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
Powered by blists - more mailing lists