iommu: Avoid DMA ops domain refcount contention

John raised the issue[1] that we have some unnecessary refcount contention in the DMA ops path which shows scalability problems now that we have more real high-performance hardware using iommu-dma. The x86 IOMMU drivers are sidestepping this by stashing domain references in archdata, but since that's not very nice for architecture-agnostic code, I think it's time to look at a generic API-level solution.

These are a couple of quick patches based on the idea I had back when first implementing this lot, but didn't have any way to justify at the time. The third patch can be ignored for the sake of API discussion, but is included for completeness.

Robin.

[1] https://lists.linuxfoundation.org/pipermail/iommu/2018-August/029303.html

Robin Murphy (3):
iommu: Add fast hook for getting DMA domains iommu/dma: Use fast DMA domain lookup arm64/dma-mapping: Mildly optimise non-coherent IOMMU ops

 arch/arm64/mm/dma-mapping.c | 10 +++++-----
 drivers/iommu/dma-iommu.c   | 23 ++++++++++++-----------
 drivers/iommu/iommu.c       |  9 +++++++++
 include/linux/iommu.h       |  1 +
 4 files changed, 27 insertions(+), 16 deletions(-)

I plan to make the arm iommu dma ops generic and move them to drivers/iommu for the 4.20 merge window. Because of that it would be great to create a stable branch or even pull this in through the dma-mapping tree entirely.

While iommu_get_domain_for_dev() is the robust way for arbitrary IOMMU API callers to retrieve the domain pointer, for DMA ops domains it doesn't scale well for large systems and multi-queue devices, since the momentary refcount adjustment will lead to exclusive cacheline contention when multiple CPUs are operating in parallel on different mappings for the same device.

In the case of DMA ops domains, however, this refcounting is actually unnecessary, since they already imply that the group exists and is managed by platform code and IOMMU internals (by virtue of iommu_group_get_for_dev()) such that a reference will already be held for the lifetime of the device. Thus we can avoid the bottleneck by providing a fast lookup specifically for the DMA code to retrieve the default domain it already knows it has set up - a simple read-only dereference plays much nicer with cache-coherency protocols.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/iommu.c | 9 +++++++++
 include/linux/iommu.h | 1 +
 2 files changed, 10 insertions(+)

drivers/iommu/iommu.c +9 -0

index 63b37563db7e..63c586875df5 100644
@@ -1379,6 +1379,15 @@ struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
 
+/*
+ * For IOMMU_DOMAIN_DMA implementations which already provide their own
+ * guarantees that the group and its default domain are valid and correct.
+ */
+struct iommu_domain *iommu_get_dma_domain(struct device *dev)
+{
+	return dev->iommu_group->default_domain;
+}
+
 /*
  * IOMMU groups are really the natrual working unit of the IOMMU, but
  * the IOMMU API works on domains and devices.  Bridge that gap by

include/linux/iommu.h +0 -0

index 19938ee6eb31..16f2172698e5 100644

include/linux/iommu.h +0 -0

 extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
 		     phys_addr_t paddr, size_t size, int prot);
 extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova,

Quoted 52 lines · Robin Murphy

While iommu_get_domain_for_dev() is the robust way for arbitrary IOMMU API callers to retrieve the domain pointer, for DMA ops domains it doesn't scale well for large systems and multi-queue devices, since the momentary refcount adjustment will lead to exclusive cacheline contention when multiple CPUs are operating in parallel on different mappings for the same device.

In the case of DMA ops domains, however, this refcounting is actually unnecessary, since they already imply that the group exists and is managed by platform code and IOMMU internals (by virtue of iommu_group_get_for_dev()) such that a reference will already be held for the lifetime of the device. Thus we can avoid the bottleneck by providing a fast lookup specifically for the DMA code to retrieve the default domain it already knows it has set up - a simple read-only dereference plays much nicer with cache-coherency protocols.

Signed-off-by Robin Murphy <[email protected]>

---
?drivers/iommu/iommu.c | 9 +++++++++
?include/linux/iommu.h | 1 +
?2 files changed, 10 insertions(+)

drivers/iommu/iommu.c +3 -0

index 63b37563db7e..63c586875df5 100644
@@ -1379,6 +1379,15 @@ struct iommu_domain 
 *iommu_get_domain_for_dev(struct device *dev)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
 
+/*
+ * For IOMMU_DOMAIN_DMA implementations which already provide their own
+ * guarantees that the group and its default domain are valid and 

correct.
+ */

drivers/iommu/iommu.c +5 -0

+struct iommu_domain *iommu_get_dma_domain(struct device *dev)
+{
+??? return dev->iommu_group->default_domain;
+}
+

?/*
? * IOMMU groups are really the natrual working unit of the IOMMU, but ? * the IOMMU API works on domains and devices.? Bridge that gap by

include/linux/iommu.h +0 -0

index 19938ee6eb31..16f2172698e5 100644
@@ -297,6 +297,7 @@ extern int iommu_attach_device(struct iommu_domain 
 *domain,
 ?extern void iommu_detach_device(struct iommu_domain *domain,
 ???????????????? struct device *dev);

Quoted 3 lines

?extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
+extern struct iommu_domain *iommu_get_dma_domain(struct device *dev);

Hi Robin,

I was wondering whether it's standard to provide a stubbed version of this function for !CONFIG_IOMMU_API?

Cheers,
John

Quoted 5 lines

?extern int iommu_map(struct iommu_domain *domain, unsigned long iova, ????????????? phys_addr_t paddr, size_t size, int prot); ?extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova,

I plan to make the arm iommu dma ops generic and move them to drivers/iommu for the 4.20 merge window.

Because of that it would
be great to create a stable branch or even pull this in through the dma-mapping tree entirely.

Quoted 2 lines · Christoph Hellwig

I plan to make the arm iommu dma ops generic and move them to drivers/iommu for the 4.20 merge window.

You mean 32-bit arm?

The only place that code should move to is /dev/null ;)
- the plan has always been to convert it to use groups and default domains so it can just plug iommu-dma in. The tricky bit is either weaning the users of the private arm_iommu_*() API onto generic IOMMU API usage, or at least implementing transitional wrappers in a way that doesn't break anything.

John raised the issue[1] that we have some unnecessary refcount contention in the DMA ops path which shows scalability problems now that we have more real high-performance hardware using iommu-dma. The x86 IOMMU drivers are sidestepping this by stashing domain references in archdata, but since that's not very nice for architecture-agnostic code, I think it's time to look at a generic API-level solution.

These are a couple of quick patches based on the idea I had back when first implementing this lot, but didn't have any way to justify at the time. The third patch can be ignored for the sake of API discussion, but is included for completeness.

Robin.

[1] https://lists.linuxfoundation.org/pipermail/iommu/2018-August/029303.html

Robin Murphy (3):
iommu: Add fast hook for getting DMA domains iommu/dma: Use fast DMA domain lookup arm64/dma-mapping: Mildly optimise non-coherent IOMMU ops

 arch/arm64/mm/dma-mapping.c | 10 +++++-----
 drivers/iommu/dma-iommu.c   | 23 ++++++++++++-----------
 drivers/iommu/iommu.c       |  9 +++++++++
 include/linux/iommu.h       |  1 +
 4 files changed, 27 insertions(+), 16 deletions(-)

While iommu_get_domain_for_dev() is the robust way for arbitrary IOMMU API callers to retrieve the domain pointer, for DMA ops domains it doesn't scale well for large systems and multi-queue devices, since the momentary refcount adjustment will lead to exclusive cacheline contention when multiple CPUs are operating in parallel on different mappings for the same device.

In the case of DMA ops domains, however, this refcounting is actually unnecessary, since they already imply that the group exists and is managed by platform code and IOMMU internals (by virtue of iommu_group_get_for_dev()) such that a reference will already be held for the lifetime of the device. Thus we can avoid the bottleneck by providing a fast lookup specifically for the DMA code to retrieve the default domain it already knows it has set up - a simple read-only dereference plays much nicer with cache-coherency protocols.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/iommu.c | 9 +++++++++
  include/linux/iommu.h | 1 +
  2 files changed, 10 insertions(+)

drivers/iommu/iommu.c +0 -0

index 63b37563db7e..63c586875df5 100644
@@ -1379,6 +1379,15 @@ struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
  }

Whilst the symmetry of deferring to the existing sync callback in __iommu_map_page() is nice, taking a round-trip through iommu_iova_to_phys() is a pretty heavyweight way to get an address we can trivially compute from the page we already have. Tweaking it to just perform the cache maintenance directly when appropriate doesn't really make the code any more complicated, and the runtime efficiency gain can only be a benefit.

Furthermore, the sync operations themselves know they can only be invoked on a managed DMA ops domain, so can use the fast specific domain lookup to avoid excessive manipulation of the group refcount (particularly in the scatterlist cases).

Signed-off-by Robin Murphy <[email protected]>

---
  arch/arm64/mm/dma-mapping.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

arch/arm64/mm/dma-mapping.c +2 -2

index 61e93f0b5482..5d4144093c20 100644
@@ -712,7 +712,7 @@ static void __iommu_sync_single_for_cpu(struct device *dev,
  	if (is_device_dma_coherent(dev))
  		return;
 
-	phys = iommu_iova_to_phys(iommu_get_domain_for_dev(dev), dev_addr);
+	phys = iommu_iova_to_phys(iommu_get_dma_domain(dev), dev_addr);
  	__dma_unmap_area(phys_to_virt(phys), size, dir);
  }
 

@@ -725,7 +725,7 @@ static void __iommu_sync_single_for_device(struct device *dev,
  	if (is_device_dma_coherent(dev))
  		return;
 
-	phys = iommu_iova_to_phys(iommu_get_domain_for_dev(dev), dev_addr);
+	phys = iommu_iova_to_phys(iommu_get_dma_domain(dev), dev_addr);
  	__dma_map_area(phys_to_virt(phys), size, dir);
  }
 

arch/arm64/mm/dma-mapping.c +0 -0

  	return dev_addr;
  }