Re: [PATCH v4] iommu: Optimise PCI SAC address trick

[Vasant Hegde]

Robin,


On 4/13/2023 7:10 PM, Robin Murphy wrote:
> Per the reasoning in commit 4bf7fda4dce2 ("iommu/dma: Add config for
> PCI SAC address trick") and its subsequent revert, this mechanism no
> longer serves its original purpose, but now only works around broken
> hardware/drivers in a way that is unfortunately too impactful to remove.
> 
> This does not, however, prevent us from solving the performance impact
> which that workaround has on large-scale systems that don't need it.
> Once the 32-bit IOVA space fills up and a workload starts allocating and
> freeing on both sides of the boundary, the opportunistic SAC allocation
> can then end up spending significant time hunting down scattered
> fragments of free 32-bit space, or just reestablishing max32_alloc_size.
> This can easily be exacerbated by a change in allocation pattern, such
> as by changing the network MTU, which can increase pressure on the
> 32-bit space by leaving a large quantity of cached IOVAs which are now
> the wrong size to be recycled, but also won't be freed since the
> non-opportunistic allocations can still be satisfied from the whole
> 64-bit space without triggering the reclaim path.
> 
> However, in the context of a workaround where smaller DMA addresses
> aren't simply a preference but a necessity, if we get to that point at
> all then in fact it's already the endgame. The nature of the allocator
> is currently such that the first IOVA we give to a device after the
> 32-bit space runs out will be the highest possible address for that
> device, ever. If that works, then great, we know we can optimise for
> speed by always allocating from the full range. And if it doesn't, then
> the worst has already happened and any brokenness is now showing, so
> there's little point in continuing to try to hide it.
> 
> To that end, implement a flag to refine the SAC business into a
> per-device policy that can automatically get itself out of the way if
> and when it stops being useful.
> 
> CC: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> CC: Jakub Kicinski <kuba@xxxxxxxxxx>
> Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>

We hit kernel softlockup while running stress-ng test system having 384 CPU and
NVMe disk. This patch helped to solve one soft lockup in allocation path.

> ---
> 
> v4: Rebase to use the new bitfield in dev_iommu, expand commit message.
> 
>  drivers/iommu/dma-iommu.c | 26 ++++++++++++++++++++------
>  drivers/iommu/dma-iommu.h |  8 ++++++++
>  drivers/iommu/iommu.c     |  3 +++
>  include/linux/iommu.h     |  2 ++
>  4 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 99b2646cb5c7..9193ad5bc72f 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -630,7 +630,7 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
>  {
>  	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>  	struct iova_domain *iovad = &cookie->iovad;
> -	unsigned long shift, iova_len, iova = 0;
> +	unsigned long shift, iova_len, iova;
>  
>  	if (cookie->type == IOMMU_DMA_MSI_COOKIE) {
>  		cookie->msi_iova += size;
> @@ -645,15 +645,29 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
>  	if (domain->geometry.force_aperture)
>  		dma_limit = min(dma_limit, (u64)domain->geometry.aperture_end);
>  
> -	/* Try to get PCI devices a SAC address */
> -	if (dma_limit > DMA_BIT_MASK(32) && !iommu_dma_forcedac && dev_is_pci(dev))
> +	/*
> +	 * Try to use all the 32-bit PCI addresses first. The original SAC vs.
> +	 * DAC reasoning loses relevance with PCIe, but enough hardware and
> +	 * firmware bugs are still lurking out there that it's safest not to
> +	 * venture into the 64-bit space until necessary.
> +	 *
> +	 * If your device goes wrong after seeing the notice then likely either
> +	 * its driver is not setting DMA masks accurately, the hardware has
> +	 * some inherent bug in handling >32-bit addresses, or not all the
> +	 * expected address bits are wired up between the device and the IOMMU.
> +	 */
> +	if (dma_limit > DMA_BIT_MASK(32) && dev->iommu->pci_32bit_workaround) {
>  		iova = alloc_iova_fast(iovad, iova_len,
>  				       DMA_BIT_MASK(32) >> shift, false);
> +		if (iova)
> +			goto done;
>  
> -	if (!iova)
> -		iova = alloc_iova_fast(iovad, iova_len, dma_limit >> shift,
> -				       true);
> +		dev->iommu->pci_32bit_workaround = false;
> +		dev_notice(dev, "Using %d-bit DMA addresses\n", bits_per(dma_limit));

May be dev_notice_once? Otherwise we may see this message multiple time for same
device like below:

[  172.017120] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.022955] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.028720] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.031815] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.031816] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.038727] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.038726] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.038917] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.038968] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.038970] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.039007] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.039091] nvme 0000:41:00.0: Using 64-bit DMA addresses
[  172.039102] nvme 0000:41:00.0: Using 64-bit DMA addresses

Otherwise patch worked fine for us.

Tested-by: Vasant Hegde <vasant.hegde@xxxxxxx>

-Vasant