Received-SPF: pass (google.com: domain of linux-kernel+bounces-196576-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <d854664c-ab56-46b6-81f9-9ef712c32b3a@nvidia.com>
Date: Fri, 31 May 2024 14:52:55 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH] iommu: Optimize IOMMU UnMap
From: Ashish Mhetre <amhetre@nvidia.com>
To: Robin Murphy <robin.murphy@arm.com>, will@kernel.org, joro@8bytes.org,
 linux-arm-kernel@lists.infradead.org, Rob Clark <robdclark@gmail.com>,
 Boris Brezillon <boris.brezillon@collabora.com>
Cc: vdumpa@nvidia.com, linux-tegra@vger.kernel.org, treding@nvidia.com,
 jonathanh@nvidia.com, iommu@lists.linux.dev, linux-kernel@vger.kernel.org
References: <20240523031935.90856-1-amhetre@nvidia.com>
 <6b707eb4-5cf3-4b66-8152-5ba252f5df39@arm.com>
 <2a349725-72cf-45e2-9ca2-5a8b153a6fae@nvidia.com>
Content-Language: en-US
In-Reply-To: <2a349725-72cf-45e2-9ca2-5a8b153a6fae@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?Z0RtRGJjZHFwcEZFcm9PS0R2YSsvaHpHZTVKaExkMC9odTF2MUFZeXlvUTg2?=
 =?utf-8?B?SWdtaDdhWmlGTllraktZamFpWjc0bmJGQTQ2eTdWTnpnNzdOM1RiY2xBV3E1?=
 =?utf-8?B?KzRuNFN5WjIyZ0lvcUxEWjRzbUdDWkVOMWFrL1NHZGZTeDJoSTVwZ3VDOU5n?=
 =?utf-8?B?WkYxRWxFQ21lY0lLenBReUtqa2N2WkE1djB1OVNKaWZBRzV3eXRndjE5M2dR?=
 =?utf-8?B?aGlZaFdzeWQxUW81MkxONnBuMEdzU2NrV3BqUmRxbEpoUmU1ZHFxcWRHODky?=
 =?utf-8?B?WUlPVExDK0hQQVFweXc4RURXbE9DVzVtZUxwYTl5SHR3c2dnQXY4Vnd1Nk9s?=
 =?utf-8?B?QWtwUVoyb0EzbHdiQWNVWCs1UGxnMEQ5U1hrUExZVWNxZ0ZzMlpMVGNQYWhK?=
 =?utf-8?B?ODBoUlg2MzlYTWJJZFZmY3pyNFRablcybTNUdTlrT09LUnVneUN0azZVQXlU?=
 =?utf-8?B?UDZ0NHcrNXF6eEMvYVZlRVFiS1g2c3ltaUszeW56OFlockc4Z0k2Zkplays3?=
 =?utf-8?B?dnBXQ3puL3lBZ1pQVE5YZWlWSHNmM2E0NDNqS3FpSEJwYU9EdVAyTWgrS1hx?=
 =?utf-8?B?UHBmU0puMHZLWUVRRFpzQ1lBajdUOTg0eDJBTW5LWkdYRmtpTENsUFczMXNJ?=
 =?utf-8?B?RnJoNXNTdVNwQnFEVmhVKzcyTWczK0U2RUxrbWxGbHRmc0MvOWpNczRLb3I0?=
 =?utf-8?B?R0dmbjNXSVVkYUszcmVZUHkrcWdsZ1hnemtDY2J0YlUwUUJGdU5WNE5LeWdn?=
 =?utf-8?B?MFRYdnpRVFU4eDZON0xHODQvUFpOVnZ3QWsxa0VnMHVJaUo5MWJubndjS3dG?=
 =?utf-8?B?YitFNTZ5VG9iWm5wVHZoS2srNU96N3dTQ0RtdXg1cXNpV0FvWHpRalFkc2th?=
 =?utf-8?B?Yzd5UFB2T242T2VzNU1NYzhYZmZPM3VEbC9KcDlqS3FqbjlwdWtNb2Y3MGRt?=
 =?utf-8?B?MW9vazRMNy9vNnBNbmJQQTMyWEVGUTdMUzkzRFNSYmtuUGlrVTltelVFSFhI?=
 =?utf-8?B?S2gzWGpEaXU2S05LMjZZUHBOcmNEMjd3VTZJM0k5VjI2cXhIYUltV3ROV1E0?=
 =?utf-8?B?MytPZUxQYUdzRVIwUnlEVU9COXVYRG1KckY5SlhpVlcydXpCR1AvVkhXSThU?=
 =?utf-8?B?SXFoRjZrckp1amtVdWZRS3F5cEo3TGxVa1VMU1M0QlFqU0JEditrbHFrUlpj?=
 =?utf-8?B?MFlXK0F2RlRPQXFqZU1CdU5rb2x0MWZFK3ViTEs5NWtxMUtRRGtFUFVic2lZ?=
 =?utf-8?B?RisxUnFYWVFmUUJva2t1Qlo0NUJUZktERzdUYzBUOVBqQTduTmtoYjZlbkRG?=
 =?utf-8?B?VTJVTnZZMTh6Z3ZjODcrWDFJN1FiNmxMdUJLbm1vV2JyRkZRWVBIYm5wRWdz?=
 =?utf-8?B?UDNLak9SRnplbExObVZlREJrYnZPblpQZjlqSDNFNHhJVVE0bGhIQkQ2K1gx?=
 =?utf-8?B?anBzZmE0RmNWM21FV3ZJUkcrY044VEVxUENITjdWcE5oSVB6ZWtnczJaT1hX?=
 =?utf-8?B?VzFuNm0wSEdmWUlyN0NKTEJzbGl3OWkrYzF1ZnVPU1hib2FWRDBOMFlGK3hW?=
 =?utf-8?B?MTFTcXFjQ3RWZWRxcHlCSXlWOFVVTTRPdzl0aTRmUnZHeDRQazVLRGFzclBz?=
 =?utf-8?B?RTRUM0JBT2VldS9lcktxbERPUzBCS2V1aTFWQWxDUW02TzBKYlZ6UytORVJq?=
 =?utf-8?B?dFRtU1VGcVpZVDI0cTllL096cjE0alFpaE5hL3krZXQvZGFKOVVGSE84WjdM?=
 =?utf-8?B?Q0FxdURtaC81ZXRPNitrQkluOTBsdXMzMHVONmtFR29jUHNrMG1zMG41Ynl5?=
 =?utf-8?B?Q3dhN1JmaEluUS9za1B6Um55eEdtd2RhUTZYTWFtL0lSSXo3RXVXVDY5Yzlt?=
 =?utf-8?B?a3c1ZlJiNHE5dHd4TTNMdk55Wnk0bWVJZFVBZVQvd3RCbGpaUzcrR3BaNC90?=
 =?utf-8?B?T1h5QzBnQ1pFY0VEL0IzVitMc2puK1NITi9CdWV0b1krNWY5QTEwUEFDRHRl?=
 =?utf-8?B?Ym9rQ1kwejBPYjFuVlRFQm53cFF6SzIzdUF2b0xybzFZNzk2Vk9jTHB2QjAr?=
 =?utf-8?B?Rkt5SVBDa0R4S1B3d1N6OHo0SHpDV0dXRFUvbXJKZ0orWVhMNm9WNzRTUjVn?=
 =?utf-8?Q?I4NawtCGOklxrit0po2KvZo3T?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 82702143-86ec-4c46-cc3f-08dc8153467e
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB7426.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2024 09:23:04.8070
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /E0ivVhp55GGOtqUgG31eaoz96FwdENo9Q/nuZ39fQliQIudnOV7Yg5KaYyB969Vh5U/pj+IkJV7aVvJauQk7Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB6849


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