[PATCH 0/8] io-pgtable lock removal

[PATCH 0/8] io-pgtable lock removal

[Robin Murphy]

Hi all,

Here's the cleaned up nominally-final version of the patches everybody's
keen to see. #1 is just a non-critical thing-I-spotted-in-passing fix,
#2-#4 do some preparatory work (and bid farewell to everyone's least
favourite bit of code, hooray!), and #5-#8 do the dirty deed itself.

The branch I've previously shared has been updated too:

  git://linux-arm.org/linux-rm  iommu/pgtable

All feedback welcome, as I'd really like to land this for 4.13.

Robin.


Robin Murphy (8):
  iommu/io-pgtable-arm-v7s: Check table PTEs more precisely
  iommu/io-pgtable-arm: Improve split_blk_unmap
  iommu/io-pgtable-arm-v7s: Refactor split_blk_unmap
  iommu/io-pgtable: Introduce explicit coherency
  iommu/io-pgtable-arm: Support lockless operation
  iommu/io-pgtable-arm-v7s: Support lockless operation
  iommu/arm-smmu: Remove io-pgtable spinlock
  iommu/arm-smmu-v3: Remove io-pgtable spinlock

 drivers/iommu/arm-smmu-v3.c        |  36 ++-----
 drivers/iommu/arm-smmu.c           |  48 ++++------
 drivers/iommu/io-pgtable-arm-v7s.c | 173 +++++++++++++++++++++------------
 drivers/iommu/io-pgtable-arm.c     | 190 ++++++++++++++++++++++++-------------
 drivers/iommu/io-pgtable.h         |   6 ++
 5 files changed, 268 insertions(+), 185 deletions(-)

-- 
2.12.2.dirty

[PATCH 1/8] iommu/io-pgtable-arm-v7s: Check table PTEs more precisely

[Robin Murphy]

Whilst we don't support the PXN bit at all, so should never encounter a
level 1 section or supersection PTE with it set, it would still be wise
to check both table type bits to resolve any theoretical ambiguity.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/io-pgtable-arm-v7s.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
index 8d6ca28c3e1f..a490db032c51 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -92,7 +92,8 @@
 #define ARM_V7S_PTE_TYPE_CONT_PAGE	0x1
 
 #define ARM_V7S_PTE_IS_VALID(pte)	(((pte) & 0x3) != 0)
-#define ARM_V7S_PTE_IS_TABLE(pte, lvl)	(lvl == 1 && ((pte) & ARM_V7S_PTE_TYPE_TABLE))
+#define ARM_V7S_PTE_IS_TABLE(pte, lvl) \
+	((lvl) == 1 && (((pte) & 0x3) == ARM_V7S_PTE_TYPE_TABLE))
 
 /* Page table bits */
 #define ARM_V7S_ATTR_XN(lvl)		BIT(4 * (2 - (lvl)))
-- 
2.12.2.dirty

[PATCH 2/8] iommu/io-pgtable-arm: Improve split_blk_unmap

[Robin Murphy]

The current split_blk_unmap implementation suffers from some inscrutable
pointer trickery for creating the tables to replace the block entry, but
more than that it also suffers from hideous inefficiency. For example,
the most pathological case of unmapping a level 3 page from a level 1
block will allocate 513 lower-level tables to remap the entire block at
page granularity, when only 2 are actually needed (the rest can be
covered by level 2 block entries).

Also, we would like to be able to relax the spinlock requirement in
future, for which the roll-back-and-try-again logic for race resolution
would be pretty hideous under the current paradigm.

Both issues can be resolved most neatly by turning things sideways:
instead of repeatedly recursing into __arm_lpae_map() map to build up an
entire new sub-table depth-first, we can directly replace the block
entry with a next-level table of block/page entries, then repeat by
unmapping at the next level if necessary. With a little refactoring of
some helper functions, the code ends up not much bigger than before, but
considerably easier to follow and to adapt in future.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/io-pgtable-arm.c | 116 ++++++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 48 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 6e5df5e0a3bd..97d039952367 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -264,13 +264,32 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
 			    unsigned long iova, size_t size, int lvl,
 			    arm_lpae_iopte *ptep);
 
+static arm_lpae_iopte __arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
+					  arm_lpae_iopte prot, int lvl,
+					  phys_addr_t paddr)
+{
+	arm_lpae_iopte pte = prot;
+
+	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
+		pte |= ARM_LPAE_PTE_NS;
+
+	if (lvl == ARM_LPAE_MAX_LEVELS - 1)
+		pte |= ARM_LPAE_PTE_TYPE_PAGE;
+	else
+		pte |= ARM_LPAE_PTE_TYPE_BLOCK;
+
+	pte |= ARM_LPAE_PTE_AF | ARM_LPAE_PTE_SH_IS;
+	pte |= pfn_to_iopte(paddr >> data->pg_shift, data);
+
+	return pte;
+}
+
 static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
 			     unsigned long iova, phys_addr_t paddr,
 			     arm_lpae_iopte prot, int lvl,
 			     arm_lpae_iopte *ptep)
 {
-	arm_lpae_iopte pte = prot;
-	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	arm_lpae_iopte pte;
 
 	if (iopte_leaf(*ptep, lvl)) {
 		/* We require an unmap first */
@@ -289,21 +308,25 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
 			return -EINVAL;
 	}
 
-	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
-		pte |= ARM_LPAE_PTE_NS;
-
-	if (lvl == ARM_LPAE_MAX_LEVELS - 1)
-		pte |= ARM_LPAE_PTE_TYPE_PAGE;
-	else
-		pte |= ARM_LPAE_PTE_TYPE_BLOCK;
-
-	pte |= ARM_LPAE_PTE_AF | ARM_LPAE_PTE_SH_IS;
-	pte |= pfn_to_iopte(paddr >> data->pg_shift, data);
-
-	__arm_lpae_set_pte(ptep, pte, cfg);
+	pte = __arm_lpae_init_pte(data, prot, lvl, paddr);
+	__arm_lpae_set_pte(ptep, pte, &data->iop.cfg);
 	return 0;
 }
 
+static arm_lpae_iopte arm_lpae_install_table(arm_lpae_iopte *table,
+					     arm_lpae_iopte *ptep,
+					     struct io_pgtable_cfg *cfg)
+{
+	arm_lpae_iopte new;
+
+	new = __pa(table) | ARM_LPAE_PTE_TYPE_TABLE;
+	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
+		new |= ARM_LPAE_PTE_NSTABLE;
+
+	__arm_lpae_set_pte(ptep, new, cfg);
+	return new;
+}
+
 static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
 			  phys_addr_t paddr, size_t size, arm_lpae_iopte prot,
 			  int lvl, arm_lpae_iopte *ptep)
@@ -331,10 +354,7 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
 		if (!cptep)
 			return -ENOMEM;
 
-		pte = __pa(cptep) | ARM_LPAE_PTE_TYPE_TABLE;
-		if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
-			pte |= ARM_LPAE_PTE_NSTABLE;
-		__arm_lpae_set_pte(ptep, pte, cfg);
+		arm_lpae_install_table(cptep, ptep, cfg);
 	} else if (!iopte_leaf(pte, lvl)) {
 		cptep = iopte_deref(pte, data);
 	} else {
@@ -452,40 +472,42 @@ static void arm_lpae_free_pgtable(struct io_pgtable *iop)
 
 static int arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
 				    unsigned long iova, size_t size,
-				    arm_lpae_iopte prot, int lvl,
-				    arm_lpae_iopte *ptep, size_t blk_size)
+				    arm_lpae_iopte blk_pte, int lvl,
+				    arm_lpae_iopte *ptep)
 {
-	unsigned long blk_start, blk_end;
-	phys_addr_t blk_paddr;
-	arm_lpae_iopte table = 0;
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	arm_lpae_iopte pte, *tablep;
+	size_t tablesz = ARM_LPAE_GRANULE(data);
+	size_t split_sz = ARM_LPAE_BLOCK_SIZE(lvl, data);
+	int i, unmap_idx = -1;
 
-	blk_start = iova & ~(blk_size - 1);
-	blk_end = blk_start + blk_size;
-	blk_paddr = iopte_to_pfn(*ptep, data) << data->pg_shift;
+	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
+		return 0;
 
-	for (; blk_start < blk_end; blk_start += size, blk_paddr += size) {
-		arm_lpae_iopte *tablep;
+	tablep = __arm_lpae_alloc_pages(tablesz, GFP_ATOMIC, cfg);
+	if (!tablep)
+		return 0; /* Bytes unmapped */
 
+	if (size == split_sz)
+		unmap_idx = ARM_LPAE_LVL_IDX(iova, lvl, data);
+
+	pte = __arm_lpae_init_pte(data, iopte_prot(blk_pte), lvl,
+				iopte_to_pfn(blk_pte, data) << data->pg_shift);
+
+	for (i = 0; i < tablesz / sizeof(pte); pte += split_sz, i++) {
 		/* Unmap! */
-		if (blk_start == iova)
+		if (i == unmap_idx)
 			continue;
 
-		/* __arm_lpae_map expects a pointer to the start of the table */
-		tablep = &table - ARM_LPAE_LVL_IDX(blk_start, lvl, data);
-		if (__arm_lpae_map(data, blk_start, blk_paddr, size, prot, lvl,
-				   tablep) < 0) {
-			if (table) {
-				/* Free the table we allocated */
-				tablep = iopte_deref(table, data);
-				__arm_lpae_free_pgtable(data, lvl + 1, tablep);
-			}
-			return 0; /* Bytes unmapped */
-		}
+		__arm_lpae_set_pte(&tablep[i], pte, cfg);
 	}
 
-	__arm_lpae_set_pte(ptep, table, &data->iop.cfg);
-	iova &= ~(blk_size - 1);
-	io_pgtable_tlb_add_flush(&data->iop, iova, blk_size, blk_size, true);
+	arm_lpae_install_table(tablep, ptep, cfg);
+
+	if (unmap_idx < 0)
+		return __arm_lpae_unmap(data, iova, size, lvl, tablep);
+
+	io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
 	return size;
 }
 
@@ -495,7 +517,6 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
 {
 	arm_lpae_iopte pte;
 	struct io_pgtable *iop = &data->iop;
-	size_t blk_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
 
 	/* Something went horribly wrong and we ran out of page table */
 	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
@@ -507,7 +528,7 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
 		return 0;
 
 	/* If the size matches this level, we're in the right place */
-	if (size == blk_size) {
+	if (size == ARM_LPAE_BLOCK_SIZE(lvl, data)) {
 		__arm_lpae_set_pte(ptep, 0, &iop->cfg);
 
 		if (!iopte_leaf(pte, lvl)) {
@@ -527,9 +548,8 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
 		 * Insert a table at the next level to map the old region,
 		 * minus the part we want to unmap
 		 */
-		return arm_lpae_split_blk_unmap(data, iova, size,
-						iopte_prot(pte), lvl, ptep,
-						blk_size);
+		return arm_lpae_split_blk_unmap(data, iova, size, pte,
+						lvl + 1, ptep);
 	}
 
 	/* Keep on walkin' */
-- 
2.12.2.dirty

[PATCH 3/8] iommu/io-pgtable-arm-v7s: Refactor split_blk_unmap

[Robin Murphy]

Whilst the short-descriptor format's split_blk_unmap implementation has
no need to be recursive, it followed the pattern of the LPAE version
anyway for the sake of consistency. With the latter now reworked for
both efficiency and future scalability improvements, tweak the former
similarly, not least to make it less obtuse.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/io-pgtable-arm-v7s.c | 85 ++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 40 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
index a490db032c51..3ee8e61eeb18 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -281,6 +281,13 @@ static arm_v7s_iopte arm_v7s_prot_to_pte(int prot, int lvl,
 	else if (prot & IOMMU_CACHE)
 		pte |= ARM_V7S_ATTR_B | ARM_V7S_ATTR_C;
 
+	pte |= ARM_V7S_PTE_TYPE_PAGE;
+	if (lvl == 1 && (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS))
+		pte |= ARM_V7S_ATTR_NS_SECTION;
+
+	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_4GB)
+		pte |= ARM_V7S_ATTR_MTK_4GB;
+
 	return pte;
 }
 
@@ -353,7 +360,7 @@ static int arm_v7s_init_pte(struct arm_v7s_io_pgtable *data,
 			    int lvl, int num_entries, arm_v7s_iopte *ptep)
 {
 	struct io_pgtable_cfg *cfg = &data->iop.cfg;
-	arm_v7s_iopte pte = arm_v7s_prot_to_pte(prot, lvl, cfg);
+	arm_v7s_iopte pte;
 	int i;
 
 	for (i = 0; i < num_entries; i++)
@@ -375,13 +382,7 @@ static int arm_v7s_init_pte(struct arm_v7s_io_pgtable *data,
 			return -EEXIST;
 		}
 
-	pte |= ARM_V7S_PTE_TYPE_PAGE;
-	if (lvl == 1 && (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS))
-		pte |= ARM_V7S_ATTR_NS_SECTION;
-
-	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_4GB)
-		pte |= ARM_V7S_ATTR_MTK_4GB;
-
+	pte = arm_v7s_prot_to_pte(prot, lvl, cfg);
 	if (num_entries > 1)
 		pte = arm_v7s_pte_to_cont(pte, lvl);
 
@@ -391,6 +392,20 @@ static int arm_v7s_init_pte(struct arm_v7s_io_pgtable *data,
 	return 0;
 }
 
+static arm_v7s_iopte arm_v7s_install_table(arm_v7s_iopte *table,
+					   arm_v7s_iopte *ptep,
+					   struct io_pgtable_cfg *cfg)
+{
+	arm_v7s_iopte new;
+
+	new = virt_to_phys(table) | ARM_V7S_PTE_TYPE_TABLE;
+	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
+		new |= ARM_V7S_ATTR_NS_TABLE;
+
+	__arm_v7s_set_pte(ptep, new, 1, cfg);
+	return new;
+}
+
 static int __arm_v7s_map(struct arm_v7s_io_pgtable *data, unsigned long iova,
 			 phys_addr_t paddr, size_t size, int prot,
 			 int lvl, arm_v7s_iopte *ptep)
@@ -418,11 +433,7 @@ static int __arm_v7s_map(struct arm_v7s_io_pgtable *data, unsigned long iova,
 		if (!cptep)
 			return -ENOMEM;
 
-		pte = virt_to_phys(cptep) | ARM_V7S_PTE_TYPE_TABLE;
-		if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
-			pte |= ARM_V7S_ATTR_NS_TABLE;
-
-		__arm_v7s_set_pte(ptep, pte, 1, cfg);
+		arm_v7s_install_table(cptep, ptep, cfg);
 	} else if (ARM_V7S_PTE_IS_TABLE(pte, lvl)) {
 		cptep = iopte_deref(pte, lvl);
 	} else {
@@ -503,41 +514,35 @@ static void arm_v7s_split_cont(struct arm_v7s_io_pgtable *data,
 
 static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
 				   unsigned long iova, size_t size,
-				   arm_v7s_iopte *ptep)
+				   arm_v7s_iopte blk_pte, arm_v7s_iopte *ptep)
 {
-	unsigned long blk_start, blk_end, blk_size;
-	phys_addr_t blk_paddr;
-	arm_v7s_iopte table = 0;
-	int prot = arm_v7s_pte_to_prot(*ptep, 1);
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	arm_v7s_iopte pte, *tablep;
+	int i, unmap_idx, num_entries, num_ptes;
 
-	blk_size = ARM_V7S_BLOCK_SIZE(1);
-	blk_start = iova & ARM_V7S_LVL_MASK(1);
-	blk_end = blk_start + ARM_V7S_BLOCK_SIZE(1);
-	blk_paddr = *ptep & ARM_V7S_LVL_MASK(1);
+	tablep = __arm_v7s_alloc_table(2, GFP_ATOMIC, data);
+	if (!tablep)
+		return 0; /* Bytes unmapped */
 
-	for (; blk_start < blk_end; blk_start += size, blk_paddr += size) {
-		arm_v7s_iopte *tablep;
+	num_ptes = ARM_V7S_PTES_PER_LVL(2);
+	num_entries = size >> ARM_V7S_LVL_SHIFT(2);
+	unmap_idx = ARM_V7S_LVL_IDX(iova, 2);
 
+	pte = arm_v7s_prot_to_pte(arm_v7s_pte_to_prot(blk_pte, 1), 2, cfg);
+	if (num_entries > 1)
+		pte = arm_v7s_pte_to_cont(pte, 2);
+
+	for (i = 0; i < num_ptes; pte += size, i += num_entries) {
 		/* Unmap! */
-		if (blk_start == iova)
+		if (i == unmap_idx)
 			continue;
 
-		/* __arm_v7s_map expects a pointer to the start of the table */
-		tablep = &table - ARM_V7S_LVL_IDX(blk_start, 1);
-		if (__arm_v7s_map(data, blk_start, blk_paddr, size, prot, 1,
-				  tablep) < 0) {
-			if (table) {
-				/* Free the table we allocated */
-				tablep = iopte_deref(table, 1);
-				__arm_v7s_free_table(tablep, 2, data);
-			}
-			return 0; /* Bytes unmapped */
-		}
+		__arm_v7s_set_pte(&tablep[i], pte, num_entries, cfg);
 	}
 
-	__arm_v7s_set_pte(ptep, table, 1, &data->iop.cfg);
-	iova &= ~(blk_size - 1);
-	io_pgtable_tlb_add_flush(&data->iop, iova, blk_size, blk_size, true);
+	arm_v7s_install_table(tablep, ptep, cfg);
+
+	io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
 	return size;
 }
 
@@ -594,7 +599,7 @@ static int __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
 		 * Insert a table at the next level to map the old region,
 		 * minus the part we want to unmap
 		 */
-		return arm_v7s_split_blk_unmap(data, iova, size, ptep);
+		return arm_v7s_split_blk_unmap(data, iova, size, pte[0], ptep);
 	}
 
 	/* Keep on walkin' */
-- 
2.12.2.dirty

[PATCH 4/8] iommu/io-pgtable: Introduce explicit coherency

[Robin Murphy]

Once we remove the serialising spinlock, a potential race opens up for
non-coherent IOMMUs whereby a caller of .map() can be sure that cache
maintenance has been performed on their new PTE, but will have no
guarantee that such maintenance for table entries above it has actually
completed (e.g. if another CPU took an interrupt immediately after
writing the table entry, but before initiating the DMA sync).

Handling this race safely will add some potentially non-trivial overhead
to installing a table entry, which we would much rather avoid on
coherent systems where it will be unnecessary, and where we are stirivng
to minimise latency by removing the locking in the first place.

To that end, let's introduce an explicit notion of cache-coherency to
io-pgtable, such that we will be able to avoid penalising IOMMUs which
know enough to know when they are coherent.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu-v3.c        |  3 +++
 drivers/iommu/arm-smmu.c           |  3 +++
 drivers/iommu/io-pgtable-arm-v7s.c | 17 ++++++++++-------
 drivers/iommu/io-pgtable-arm.c     | 11 ++++++-----
 drivers/iommu/io-pgtable.h         |  6 ++++++
 5 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 380969aa60d5..b9c4cf4ccca2 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1555,6 +1555,9 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
 		.iommu_dev	= smmu->dev,
 	};
 
+	if (smmu->features & ARM_SMMU_FEAT_COHERENCY)
+		pgtbl_cfg.quirks = IO_PGTABLE_QUIRK_NO_DMA;
+
 	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
 	if (!pgtbl_ops)
 		return -ENOMEM;
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 7ec30b08b3bd..1f42f339a284 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1010,6 +1010,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 		.iommu_dev	= smmu->dev,
 	};
 
+	if (smmu->features & ARM_SMMU_FEAT_COHERENT_WALK)
+		pgtbl_cfg.quirks = IO_PGTABLE_QUIRK_NO_DMA;
+
 	smmu_domain->smmu = smmu;
 	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
 	if (!pgtbl_ops) {
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
index 3ee8e61eeb18..2437f2899661 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -187,7 +187,8 @@ static arm_v7s_iopte *iopte_deref(arm_v7s_iopte pte, int lvl)
 static void *__arm_v7s_alloc_table(int lvl, gfp_t gfp,
 				   struct arm_v7s_io_pgtable *data)
 {
-	struct device *dev = data->iop.cfg.iommu_dev;
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	struct device *dev = cfg->iommu_dev;
 	dma_addr_t dma;
 	size_t size = ARM_V7S_TABLE_SIZE(lvl);
 	void *table = NULL;
@@ -196,7 +197,7 @@ static void *__arm_v7s_alloc_table(int lvl, gfp_t gfp,
 		table = (void *)__get_dma_pages(__GFP_ZERO, get_order(size));
 	else if (lvl == 2)
 		table = kmem_cache_zalloc(data->l2_tables, gfp | GFP_DMA);
-	if (table && !selftest_running) {
+	if (table && !(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA)) {
 		dma = dma_map_single(dev, table, size, DMA_TO_DEVICE);
 		if (dma_mapping_error(dev, dma))
 			goto out_free;
@@ -225,10 +226,11 @@ static void *__arm_v7s_alloc_table(int lvl, gfp_t gfp,
 static void __arm_v7s_free_table(void *table, int lvl,
 				 struct arm_v7s_io_pgtable *data)
 {
-	struct device *dev = data->iop.cfg.iommu_dev;
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	struct device *dev = cfg->iommu_dev;
 	size_t size = ARM_V7S_TABLE_SIZE(lvl);
 
-	if (!selftest_running)
+	if (!(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA))
 		dma_unmap_single(dev, __arm_v7s_dma_addr(table), size,
 				 DMA_TO_DEVICE);
 	if (lvl == 1)
@@ -240,7 +242,7 @@ static void __arm_v7s_free_table(void *table, int lvl,
 static void __arm_v7s_pte_sync(arm_v7s_iopte *ptep, int num_entries,
 			       struct io_pgtable_cfg *cfg)
 {
-	if (selftest_running)
+	if (!(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA))
 		return;
 
 	dma_sync_single_for_device(cfg->iommu_dev, __arm_v7s_dma_addr(ptep),
@@ -657,7 +659,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
 	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
 			    IO_PGTABLE_QUIRK_NO_PERMS |
 			    IO_PGTABLE_QUIRK_TLBI_ON_MAP |
-			    IO_PGTABLE_QUIRK_ARM_MTK_4GB))
+			    IO_PGTABLE_QUIRK_ARM_MTK_4GB |
+			    IO_PGTABLE_QUIRK_NO_DMA))
 		return NULL;
 
 	/* If ARM_MTK_4GB is enabled, the NO_PERMS is also expected. */
@@ -774,7 +777,7 @@ static int __init arm_v7s_do_selftests(void)
 		.tlb = &dummy_tlb_ops,
 		.oas = 32,
 		.ias = 32,
-		.quirks = IO_PGTABLE_QUIRK_ARM_NS,
+		.quirks = IO_PGTABLE_QUIRK_ARM_NS | IO_PGTABLE_QUIRK_NO_DMA,
 		.pgsize_bitmap = SZ_4K | SZ_64K | SZ_1M | SZ_16M,
 	};
 	unsigned int iova, size, iova_start;
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 97d039952367..bdc954cb98c1 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -217,7 +217,7 @@ static void *__arm_lpae_alloc_pages(size_t size, gfp_t gfp,
 	if (!pages)
 		return NULL;
 
-	if (!selftest_running) {
+	if (!(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA)) {
 		dma = dma_map_single(dev, pages, size, DMA_TO_DEVICE);
 		if (dma_mapping_error(dev, dma))
 			goto out_free;
@@ -243,7 +243,7 @@ static void *__arm_lpae_alloc_pages(size_t size, gfp_t gfp,
 static void __arm_lpae_free_pages(void *pages, size_t size,
 				  struct io_pgtable_cfg *cfg)
 {
-	if (!selftest_running)
+	if (!(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA))
 		dma_unmap_single(cfg->iommu_dev, __arm_lpae_dma_addr(pages),
 				 size, DMA_TO_DEVICE);
 	free_pages_exact(pages, size);
@@ -254,7 +254,7 @@ static void __arm_lpae_set_pte(arm_lpae_iopte *ptep, arm_lpae_iopte pte,
 {
 	*ptep = pte;
 
-	if (!selftest_running)
+	if (!(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA))
 		dma_sync_single_for_device(cfg->iommu_dev,
 					   __arm_lpae_dma_addr(ptep),
 					   sizeof(pte), DMA_TO_DEVICE);
@@ -693,7 +693,7 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
 	u64 reg;
 	struct arm_lpae_io_pgtable *data;
 
-	if (cfg->quirks & ~IO_PGTABLE_QUIRK_ARM_NS)
+	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | IO_PGTABLE_QUIRK_NO_DMA))
 		return NULL;
 
 	data = arm_lpae_alloc_pgtable(cfg);
@@ -782,7 +782,7 @@ arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
 	struct arm_lpae_io_pgtable *data;
 
 	/* The NS quirk doesn't apply at stage 2 */
-	if (cfg->quirks)
+	if (cfg->quirks & ~IO_PGTABLE_QUIRK_NO_DMA)
 		return NULL;
 
 	data = arm_lpae_alloc_pgtable(cfg);
@@ -1086,6 +1086,7 @@ static int __init arm_lpae_do_selftests(void)
 	struct io_pgtable_cfg cfg = {
 		.tlb = &dummy_tlb_ops,
 		.oas = 48,
+		.quirks = IO_PGTABLE_QUIRK_NO_DMA,
 	};
 
 	for (i = 0; i < ARRAY_SIZE(pgsize); ++i) {
diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
index 969d82cc92ca..524263a7ae6f 100644
--- a/drivers/iommu/io-pgtable.h
+++ b/drivers/iommu/io-pgtable.h
@@ -65,11 +65,17 @@ struct io_pgtable_cfg {
 	 *	PTEs, for Mediatek IOMMUs which treat it as a 33rd address bit
 	 *	when the SoC is in "4GB mode" and they can only access the high
 	 *	remap of DRAM (0x1_00000000 to 0x1_ffffffff).
+	 *
+	 * IO_PGTABLE_QUIRK_NO_DMA: Guarantees that the tables will only ever
+	 *	be accessed by a fully cache-coherent IOMMU or CPU (e.g. for a
+	 *	software-emulated IOMMU), such that pagetable updates need not
+	 *	be treated as explicit DMA data.
 	 */
 	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
 	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
 	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2)
 	#define IO_PGTABLE_QUIRK_ARM_MTK_4GB	BIT(3)
+	#define IO_PGTABLE_QUIRK_NO_DMA		BIT(4)
 	unsigned long			quirks;
 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;
-- 
2.12.2.dirty

[PATCH 5/8] iommu/io-pgtable-arm: Support lockless operation

[Robin Murphy]

For parallel I/O with multiple concurrent threads servicing the same
device (or devices, if several share a domain), serialising page table
updates becomes a massive bottleneck. On reflection, though, we don't
strictly need to do that - for valid IOMMU API usage, there are in fact
only two races that we need to guard against: multiple map requests for
different blocks within the same region, when the intermediate-level
table for that region does not yet exist; and multiple unmaps of
different parts of the same block entry. Both of those are fairly easily
solved by using a cmpxchg to install the new table, such that if we then
find that someone else's table got there first, we can simply free ours
and continue.

Make the requisite changes such that we can withstand being called
without the caller maintaining a lock. In theory, this opens up a few
corners in which wildly misbehaving callers making nonsensical
overlapping requests might lead to crashes instead of just unpredictable
results, but correct code really does not deserve to pay a significant
performance cost for the sake of masking bugs in theoretical broken code.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/io-pgtable-arm.c | 75 ++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index bdc954cb98c1..d857961af1d3 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -20,6 +20,7 @@
 
 #define pr_fmt(fmt)	"arm-lpae io-pgtable: " fmt
 
+#include <linux/atomic.h>
 #include <linux/iommu.h>
 #include <linux/kernel.h>
 #include <linux/sizes.h>
@@ -99,6 +100,8 @@
 #define ARM_LPAE_PTE_ATTR_HI_MASK	(((arm_lpae_iopte)6) << 52)
 #define ARM_LPAE_PTE_ATTR_MASK		(ARM_LPAE_PTE_ATTR_LO_MASK |	\
 					 ARM_LPAE_PTE_ATTR_HI_MASK)
+/* Software bit for solving coherency races */
+#define ARM_LPAE_PTE_SW_SYNC		(((arm_lpae_iopte)1) << 55)
 
 /* Stage-1 PTE */
 #define ARM_LPAE_PTE_AP_UNPRIV		(((arm_lpae_iopte)1) << 6)
@@ -249,15 +252,20 @@ static void __arm_lpae_free_pages(void *pages, size_t size,
 	free_pages_exact(pages, size);
 }
 
+static void __arm_lpae_sync_pte(arm_lpae_iopte *ptep,
+				struct io_pgtable_cfg *cfg)
+{
+	dma_sync_single_for_device(cfg->iommu_dev, __arm_lpae_dma_addr(ptep),
+				   sizeof(*ptep), DMA_TO_DEVICE);
+}
+
 static void __arm_lpae_set_pte(arm_lpae_iopte *ptep, arm_lpae_iopte pte,
 			       struct io_pgtable_cfg *cfg)
 {
 	*ptep = pte;
 
 	if (!(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA))
-		dma_sync_single_for_device(cfg->iommu_dev,
-					   __arm_lpae_dma_addr(ptep),
-					   sizeof(pte), DMA_TO_DEVICE);
+		__arm_lpae_sync_pte(ptep, cfg);
 }
 
 static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
@@ -289,13 +297,13 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
 			     arm_lpae_iopte prot, int lvl,
 			     arm_lpae_iopte *ptep)
 {
-	arm_lpae_iopte pte;
+	arm_lpae_iopte pte = *ptep;
 
-	if (iopte_leaf(*ptep, lvl)) {
+	if (iopte_leaf(pte, lvl)) {
 		/* We require an unmap first */
 		WARN_ON(!selftest_running);
 		return -EEXIST;
-	} else if (iopte_type(*ptep, lvl) == ARM_LPAE_PTE_TYPE_TABLE) {
+	} else if (iopte_type(pte, lvl) == ARM_LPAE_PTE_TYPE_TABLE) {
 		/*
 		 * We need to unmap and free the old table before
 		 * overwriting it with a block entry.
@@ -315,16 +323,30 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
 
 static arm_lpae_iopte arm_lpae_install_table(arm_lpae_iopte *table,
 					     arm_lpae_iopte *ptep,
+					     arm_lpae_iopte curr,
 					     struct io_pgtable_cfg *cfg)
 {
-	arm_lpae_iopte new;
+	arm_lpae_iopte old, new;
 
 	new = __pa(table) | ARM_LPAE_PTE_TYPE_TABLE;
 	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
 		new |= ARM_LPAE_PTE_NSTABLE;
 
-	__arm_lpae_set_pte(ptep, new, cfg);
-	return new;
+	old = cmpxchg64_relaxed(ptep, curr, new);
+
+	if (cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA)
+		return old;
+
+	/* Even if it's not ours, there's no point waiting; just kick it */
+	if (!(old & ARM_LPAE_PTE_SW_SYNC))
+		__arm_lpae_sync_pte(ptep, cfg);
+	if (old == curr) {
+		/* Ensure our sync is finished before we mark it as such */
+		wmb();
+		*ptep |= ARM_LPAE_PTE_SW_SYNC;
+	}
+
+	return old;
 }
 
 static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
@@ -333,6 +355,7 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
 {
 	arm_lpae_iopte *cptep, pte;
 	size_t block_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
+	size_t tblsz = ARM_LPAE_GRANULE(data);
 	struct io_pgtable_cfg *cfg = &data->iop.cfg;
 
 	/* Find our entry at the current level */
@@ -347,20 +370,26 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
 		return -EINVAL;
 
 	/* Grab a pointer to the next level */
-	pte = *ptep;
+	pte = READ_ONCE(*ptep);
 	if (!pte) {
-		cptep = __arm_lpae_alloc_pages(ARM_LPAE_GRANULE(data),
-					       GFP_ATOMIC, cfg);
+		cptep = __arm_lpae_alloc_pages(tblsz, GFP_ATOMIC, cfg);
 		if (!cptep)
 			return -ENOMEM;
 
-		arm_lpae_install_table(cptep, ptep, cfg);
-	} else if (!iopte_leaf(pte, lvl)) {
-		cptep = iopte_deref(pte, data);
-	} else {
+		pte = arm_lpae_install_table(cptep, ptep, 0, cfg);
+		if (pte)
+			__arm_lpae_free_pages(cptep, tblsz, cfg);
+	} else if (!(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA) &&
+		   !(pte & ARM_LPAE_PTE_SW_SYNC)) {
+		__arm_lpae_sync_pte(ptep, cfg);
+	}
+
+	if (iopte_leaf(pte, lvl)) {
 		/* We require an unmap first */
 		WARN_ON(!selftest_running);
 		return -EEXIST;
+	} else if (pte) {
+		cptep = iopte_deref(pte, data);
 	}
 
 	/* Rinse, repeat */
@@ -502,7 +531,19 @@ static int arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
 		__arm_lpae_set_pte(&tablep[i], pte, cfg);
 	}
 
-	arm_lpae_install_table(tablep, ptep, cfg);
+	pte = arm_lpae_install_table(tablep, ptep, blk_pte, cfg);
+	if (pte != blk_pte) {
+		__arm_lpae_free_pages(tablep, tablesz, cfg);
+		/*
+		 * We may race against someone unmapping another part of this
+		 * block, but anything else is invalid. We can't misinterpret
+		 * a page entry here since we're never at the last level.
+		 */
+		if (iopte_type(pte, lvl) != ARM_LPAE_PTE_TYPE_TABLE)
+			return 0;
+
+		tablep = iopte_deref(pte, data);
+	}
 
 	if (unmap_idx < 0)
 		return __arm_lpae_unmap(data, iova, size, lvl, tablep);
-- 
2.12.2.dirty

[PATCH 6/8] iommu/io-pgtable-arm-v7s: Support lockless operation

[Robin Murphy]

Mirroring the LPAE implementation, rework the v7s code to be robust
against concurrent operations. The same two potential races exist, and
are solved in the same manner, with the fixed 2-level structure making
life ever so slightly simpler.

What complicates matters compared to LPAE, however, is large page
entries, since we can't update a block of 16 PTEs atomically, nor assume
available software bits to do clever things with. As most users are
never likely to do partial unmaps anyway (due to DMA API rules), it
doesn't seem unreasonable for this case to remain behind a serialising
lock; we just pull said lock down into the bowels of the implementation
so it's well out of the way of the normal call paths.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/io-pgtable-arm-v7s.c | 78 ++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
index 2437f2899661..35d0f41af276 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -32,6 +32,7 @@
 
 #define pr_fmt(fmt)	"arm-v7s io-pgtable: " fmt
 
+#include <linux/atomic.h>
 #include <linux/dma-mapping.h>
 #include <linux/gfp.h>
 #include <linux/iommu.h>
@@ -39,6 +40,7 @@
 #include <linux/kmemleak.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 
 #include <asm/barrier.h>
@@ -168,6 +170,7 @@ struct arm_v7s_io_pgtable {
 
 	arm_v7s_iopte		*pgd;
 	struct kmem_cache	*l2_tables;
+	spinlock_t		split_lock;
 };
 
 static dma_addr_t __arm_v7s_dma_addr(void *pages)
@@ -396,16 +399,19 @@ static int arm_v7s_init_pte(struct arm_v7s_io_pgtable *data,
 
 static arm_v7s_iopte arm_v7s_install_table(arm_v7s_iopte *table,
 					   arm_v7s_iopte *ptep,
+					   arm_v7s_iopte curr,
 					   struct io_pgtable_cfg *cfg)
 {
-	arm_v7s_iopte new;
+	arm_v7s_iopte old, new;
 
 	new = virt_to_phys(table) | ARM_V7S_PTE_TYPE_TABLE;
 	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
 		new |= ARM_V7S_ATTR_NS_TABLE;
 
-	__arm_v7s_set_pte(ptep, new, 1, cfg);
-	return new;
+	old = cmpxchg_relaxed(ptep, curr, new);
+	__arm_v7s_pte_sync(ptep, 1, cfg);
+
+	return old;
 }
 
 static int __arm_v7s_map(struct arm_v7s_io_pgtable *data, unsigned long iova,
@@ -429,16 +435,23 @@ static int __arm_v7s_map(struct arm_v7s_io_pgtable *data, unsigned long iova,
 		return -EINVAL;
 
 	/* Grab a pointer to the next level */
-	pte = *ptep;
+	pte = READ_ONCE(*ptep);
 	if (!pte) {
 		cptep = __arm_v7s_alloc_table(lvl + 1, GFP_ATOMIC, data);
 		if (!cptep)
 			return -ENOMEM;
 
-		arm_v7s_install_table(cptep, ptep, cfg);
-	} else if (ARM_V7S_PTE_IS_TABLE(pte, lvl)) {
-		cptep = iopte_deref(pte, lvl);
+		pte = arm_v7s_install_table(cptep, ptep, 0, cfg);
+		if (pte)
+			__arm_v7s_free_table(cptep, lvl + 1, data);
 	} else {
+		/* We've no easy way of knowing if it's synced yet, so... */
+		__arm_v7s_pte_sync(ptep, 1, cfg);
+	}
+
+	if (ARM_V7S_PTE_IS_TABLE(pte, lvl)) {
+		cptep = iopte_deref(pte, lvl);
+	} else if (pte) {
 		/* We require an unmap first */
 		WARN_ON(!selftest_running);
 		return -EEXIST;
@@ -491,27 +504,31 @@ static void arm_v7s_free_pgtable(struct io_pgtable *iop)
 	kfree(data);
 }
 
-static void arm_v7s_split_cont(struct arm_v7s_io_pgtable *data,
-			       unsigned long iova, int idx, int lvl,
-			       arm_v7s_iopte *ptep)
+static arm_v7s_iopte arm_v7s_split_cont(struct arm_v7s_io_pgtable *data,
+					unsigned long iova, int idx, int lvl,
+					arm_v7s_iopte *ptep)
 {
 	struct io_pgtable *iop = &data->iop;
 	arm_v7s_iopte pte;
 	size_t size = ARM_V7S_BLOCK_SIZE(lvl);
 	int i;
 
+	/* Check that we didn't lose a race to get the lock */
+	pte = *ptep;
+	if (!arm_v7s_pte_is_cont(pte, lvl))
+		return pte;
+
 	ptep -= idx & (ARM_V7S_CONT_PAGES - 1);
-	pte = arm_v7s_cont_to_pte(*ptep, lvl);
-	for (i = 0; i < ARM_V7S_CONT_PAGES; i++) {
-		ptep[i] = pte;
-		pte += size;
-	}
+	pte = arm_v7s_cont_to_pte(pte, lvl);
+	for (i = 0; i < ARM_V7S_CONT_PAGES; i++)
+		ptep[i] = pte + i * size;
 
 	__arm_v7s_pte_sync(ptep, ARM_V7S_CONT_PAGES, &iop->cfg);
 
 	size *= ARM_V7S_CONT_PAGES;
 	io_pgtable_tlb_add_flush(iop, iova, size, size, true);
 	io_pgtable_tlb_sync(iop);
+	return pte;
 }
 
 static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
@@ -542,7 +559,16 @@ static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
 		__arm_v7s_set_pte(&tablep[i], pte, num_entries, cfg);
 	}
 
-	arm_v7s_install_table(tablep, ptep, cfg);
+	pte = arm_v7s_install_table(tablep, ptep, blk_pte, cfg);
+	if (pte != blk_pte) {
+		__arm_v7s_free_table(tablep, 2, data);
+
+		if (!ARM_V7S_PTE_IS_TABLE(pte, 1))
+			return 0;
+
+		tablep = iopte_deref(pte, 1);
+		return __arm_v7s_unmap(data, iova, size, 2, tablep);
+	}
 
 	io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
 	return size;
@@ -563,17 +589,28 @@ static int __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
 	idx = ARM_V7S_LVL_IDX(iova, lvl);
 	ptep += idx;
 	do {
-		if (WARN_ON(!ARM_V7S_PTE_IS_VALID(ptep[i])))
+		pte[i] = READ_ONCE(ptep[i]);
+		if (WARN_ON(!ARM_V7S_PTE_IS_VALID(pte[i])))
 			return 0;
-		pte[i] = ptep[i];
 	} while (++i < num_entries);
 
 	/*
 	 * If we've hit a contiguous 'large page' entry@this level, it
 	 * needs splitting first, unless we're unmapping the whole lot.
+	 *
+	 * For splitting, we can't rewrite 16 PTEs atomically, and since we
+	 * can't necessarily assume TEX remap we don't have a software bit to
+	 * mark live entries being split. In practice (i.e. DMA API code), we
+	 * will never be splitting large pages anyway, so just wrap this edge
+	 * case in a lock for the sake of correctness and be done with it.
 	 */
-	if (num_entries <= 1 && arm_v7s_pte_is_cont(pte[0], lvl))
-		arm_v7s_split_cont(data, iova, idx, lvl, ptep);
+	if (num_entries <= 1 && arm_v7s_pte_is_cont(pte[0], lvl)) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&data->split_lock, flags);
+		pte[0] = arm_v7s_split_cont(data, iova, idx, lvl, ptep);
+		spin_unlock_irqrestore(&data->split_lock, flags);
+	}
 
 	/* If the size matches this level, we're in the right place */
 	if (num_entries) {
@@ -672,6 +709,7 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
 	if (!data)
 		return NULL;
 
+	spin_lock_init(&data->split_lock);
 	data->l2_tables = kmem_cache_create("io-pgtable_armv7s_l2",
 					    ARM_V7S_TABLE_SIZE(2),
 					    ARM_V7S_TABLE_SIZE(2),
-- 
2.12.2.dirty

[PATCH 7/8] iommu/arm-smmu: Remove io-pgtable spinlock

[Robin Murphy]

With the io-pgtable code now robust against (valid) races, we no longer
need to serialise all operations with a lock. This might make broken
callers who issue concurrent operations on overlapping addresses go even
more wrong than before, but hey, they already had little hope of useful
or deterministic results.

We do however still have to keep a lock around to serialise the ATS1*
translation ops, as parallel iova_to_phys() calls could lead to
unpredictable hardware behaviour otherwise.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu.c | 45 ++++++++++++++-------------------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 1f42f339a284..b8d069a2b31d 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -425,10 +425,10 @@ enum arm_smmu_domain_stage {
 struct arm_smmu_domain {
 	struct arm_smmu_device		*smmu;
 	struct io_pgtable_ops		*pgtbl_ops;
-	spinlock_t			pgtbl_lock;
 	struct arm_smmu_cfg		cfg;
 	enum arm_smmu_domain_stage	stage;
 	struct mutex			init_mutex; /* Protects smmu pointer */
+	spinlock_t			cb_lock; /* Serialises ATS1* ops */
 	struct iommu_domain		domain;
 };
 
@@ -1105,7 +1105,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 	}
 
 	mutex_init(&smmu_domain->init_mutex);
-	spin_lock_init(&smmu_domain->pgtbl_lock);
+	spin_lock_init(&smmu_domain->cb_lock);
 
 	return &smmu_domain->domain;
 }
@@ -1383,35 +1383,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
 			phys_addr_t paddr, size_t size, int prot)
 {
-	int ret;
-	unsigned long flags;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	struct io_pgtable_ops *ops= smmu_domain->pgtbl_ops;
+	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
 
 	if (!ops)
 		return -ENODEV;
 
-	spin_lock_irqsave(&smmu_domain->pgtbl_lock, flags);
-	ret = ops->map(ops, iova, paddr, size, prot);
-	spin_unlock_irqrestore(&smmu_domain->pgtbl_lock, flags);
-	return ret;
+	return ops->map(ops, iova, paddr, size, prot);
 }
 
 static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
 			     size_t size)
 {
-	size_t ret;
-	unsigned long flags;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	struct io_pgtable_ops *ops= smmu_domain->pgtbl_ops;
+	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
 
 	if (!ops)
 		return 0;
 
-	spin_lock_irqsave(&smmu_domain->pgtbl_lock, flags);
-	ret = ops->unmap(ops, iova, size);
-	spin_unlock_irqrestore(&smmu_domain->pgtbl_lock, flags);
-	return ret;
+	return ops->unmap(ops, iova, size);
 }
 
 static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
@@ -1425,10 +1413,11 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
 	void __iomem *cb_base;
 	u32 tmp;
 	u64 phys;
-	unsigned long va;
+	unsigned long va, flags;
 
 	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
 
+	spin_lock_irqsave(&smmu_domain->cb_lock, flags);
 	/* ATS1 registers can only be written atomically */
 	va = iova & ~0xfffUL;
 	if (smmu->version == ARM_SMMU_V2)
@@ -1438,6 +1427,7 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
 
 	if (readl_poll_timeout_atomic(cb_base + ARM_SMMU_CB_ATSR, tmp,
 				      !(tmp & ATSR_ACTIVE), 5, 50)) {
+		spin_unlock_irqrestore(&smmu_domain->cb_lock, flags);
 		dev_err(dev,
 			"iova to phys timed out on %pad. Falling back to software table walk.\n",
 			&iova);
@@ -1445,6 +1435,7 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
 	}
 
 	phys = readq_relaxed(cb_base + ARM_SMMU_CB_PAR);
+	spin_unlock_irqrestore(&smmu_domain->cb_lock, flags);
 	if (phys & CB_PAR_F) {
 		dev_err(dev, "translation fault!\n");
 		dev_err(dev, "PAR = 0x%llx\n", phys);
@@ -1457,10 +1448,8 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
 static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
 					dma_addr_t iova)
 {
-	phys_addr_t ret;
-	unsigned long flags;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	struct io_pgtable_ops *ops= smmu_domain->pgtbl_ops;
+	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
 
 	if (domain->type == IOMMU_DOMAIN_IDENTITY)
 		return iova;
@@ -1468,17 +1457,11 @@ static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
 	if (!ops)
 		return 0;
 
-	spin_lock_irqsave(&smmu_domain->pgtbl_lock, flags);
 	if (smmu_domain->smmu->features & ARM_SMMU_FEAT_TRANS_OPS &&
-			smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-		ret = arm_smmu_iova_to_phys_hard(domain, iova);
-	} else {
-		ret = ops->iova_to_phys(ops, iova);
-	}
+			smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
+		return arm_smmu_iova_to_phys_hard(domain, iova);
 
-	spin_unlock_irqrestore(&smmu_domain->pgtbl_lock, flags);
-
-	return ret;
+	return ops->iova_to_phys(ops, iova);
 }
 
 static bool arm_smmu_capable(enum iommu_cap cap)
-- 
2.12.2.dirty

[PATCH 8/8] iommu/arm-smmu-v3: Remove io-pgtable spinlock

[Robin Murphy]

As for SMMUv2, take advantage of io-pgtable's newfound tolerance for
concurrency. Unfortunately in this case the command queue lock remains a
point of serialisation for the unmap path, but there may be a little
more we can do to ameliorate that in future.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu-v3.c | 33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index b9c4cf4ccca2..291da5f918d5 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -645,7 +645,6 @@ struct arm_smmu_domain {
 	struct mutex			init_mutex; /* Protects smmu pointer */
 
 	struct io_pgtable_ops		*pgtbl_ops;
-	spinlock_t			pgtbl_lock;
 
 	enum arm_smmu_domain_stage	stage;
 	union {
@@ -1406,7 +1405,6 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 	}
 
 	mutex_init(&smmu_domain->init_mutex);
-	spin_lock_init(&smmu_domain->pgtbl_lock);
 	return &smmu_domain->domain;
 }
 
@@ -1678,44 +1676,29 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
 			phys_addr_t paddr, size_t size, int prot)
 {
-	int ret;
-	unsigned long flags;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
+	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
 
 	if (!ops)
 		return -ENODEV;
 
-	spin_lock_irqsave(&smmu_domain->pgtbl_lock, flags);
-	ret = ops->map(ops, iova, paddr, size, prot);
-	spin_unlock_irqrestore(&smmu_domain->pgtbl_lock, flags);
-	return ret;
+	return ops->map(ops, iova, paddr, size, prot);
 }
 
 static size_t
 arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
 {
-	size_t ret;
-	unsigned long flags;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
+	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
 
 	if (!ops)
 		return 0;
 
-	spin_lock_irqsave(&smmu_domain->pgtbl_lock, flags);
-	ret = ops->unmap(ops, iova, size);
-	spin_unlock_irqrestore(&smmu_domain->pgtbl_lock, flags);
-	return ret;
+	return ops->unmap(ops, iova, size);
 }
 
 static phys_addr_t
 arm_smmu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
 {
-	phys_addr_t ret;
-	unsigned long flags;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
+	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
 
 	if (domain->type == IOMMU_DOMAIN_IDENTITY)
 		return iova;
@@ -1723,11 +1706,7 @@ arm_smmu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
 	if (!ops)
 		return 0;
 
-	spin_lock_irqsave(&smmu_domain->pgtbl_lock, flags);
-	ret = ops->iova_to_phys(ops, iova);
-	spin_unlock_irqrestore(&smmu_domain->pgtbl_lock, flags);
-
-	return ret;
+	return ops->iova_to_phys(ops, iova);
 }
 
 static struct platform_driver arm_smmu_driver;
-- 
2.12.2.dirty

[PATCH 0/8] io-pgtable lock removal

[Nate Watterson]

Hi Robin,

On 6/8/2017 7:51 AM, Robin Murphy wrote:
> Hi all,
> 
> Here's the cleaned up nominally-final version of the patches everybody's
> keen to see. #1 is just a non-critical thing-I-spotted-in-passing fix,
> #2-#4 do some preparatory work (and bid farewell to everyone's least
> favourite bit of code, hooray!), and #5-#8 do the dirty deed itself.
> 
> The branch I've previously shared has been updated too:
> 
>    git://linux-arm.org/linux-rm  iommu/pgtable
> 
> All feedback welcome, as I'd really like to land this for 4.13.
>

I tested the series on a QDF2400 development platform and see notable
performance improvements particularly in workloads that make concurrent
accesses to a single iommu_domain.

> Robin.
> 
> 
> Robin Murphy (8):
>    iommu/io-pgtable-arm-v7s: Check table PTEs more precisely
>    iommu/io-pgtable-arm: Improve split_blk_unmap
>    iommu/io-pgtable-arm-v7s: Refactor split_blk_unmap
>    iommu/io-pgtable: Introduce explicit coherency
>    iommu/io-pgtable-arm: Support lockless operation
>    iommu/io-pgtable-arm-v7s: Support lockless operation
>    iommu/arm-smmu: Remove io-pgtable spinlock
>    iommu/arm-smmu-v3: Remove io-pgtable spinlock
> 
>   drivers/iommu/arm-smmu-v3.c        |  36 ++-----
>   drivers/iommu/arm-smmu.c           |  48 ++++------
>   drivers/iommu/io-pgtable-arm-v7s.c | 173 +++++++++++++++++++++------------
>   drivers/iommu/io-pgtable-arm.c     | 190 ++++++++++++++++++++++++-------------
>   drivers/iommu/io-pgtable.h         |   6 ++
>   5 files changed, 268 insertions(+), 185 deletions(-)
> 

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 0/8] io-pgtable lock removal

[Ray Jui]

Hi Robin,

I have applied this patch series on top of v4.12-rc4, and ran various
Ethernet and NVMf target throughput tests on it.

To give you some background of my setup:

The system is a ARMv8 based system with 8 cores. It has various PCIe
root complexes that can be used to connect to PCIe endpoint devices
including NIC cards and NVMe SSDs.

I'm particularly interested in the performance of the PCIe root complex
that connects to the NIC card, and during my test, IOMMU is
enabled/disabled against that particular PCIe root complex. The root
complexes connected to NVMe SSDs remain unchanged (i.e., without IOMMU).

For the Ethernet throughput out of 50G link:

Note during the multiple TCP session test, each session will be spread
to different CPU cores for optimized performance

Without IOMMU:

TX TCP x1 - 29.7 Gbps
TX TCP x4 - 30.5 Gbps
TX TCP x8 - 28 Gbps

RX TCP x1 - 15 Gbps
RX TCP x4 - 33.7 Gbps
RX TCP x8 - 36 Gbps

With IOMMU, but without your latest patch:

TX TCP x1 - 15.2 Gbps
TX TCP x4 - 14.3 Gbps
TX TCP x8 - 13 Gbps

RX TCP x1 - 7.88 Gbps
RX TCP x4 - 13.2 Gbps
RX TCP x8 - 12.6 Gbps

With IOMMU and your latest patch:

TX TCP x1 - 21.4 Gbps
TX TCP x4 - 30.5 Gbps
TX TCP x8 - 21.3 Gbps

RX TCP x1 - 7.7 Gbps
RX TCP x4 - 20.1 Gbps
RX TCP x8 - 27.1 Gbps

With the NVMf target test with 4 SSDs, fio based test, random read, 4k,
8 jobs:

Without IOMMU:

IOPS = 1080K

With IOMMU, but without your latest patch:

IOPS = 520K

With IOMMU and your latest patch:

IOPS = 500K ~ 850K (a lot of variation observed during the same test run)

As you can see, performance has improved significantly with this patch
series! That is very impressive!

However, it is still off, compared to the test runs without the IOMMU.
I'm wondering if more improvement is expected.

In addition, a much larger throughput variation is observed in the tests
with these latest patches, when multiple CPUs are involved. I'm
wondering if that is caused by some remaining lock in the driver?

Also, in a few occasions, I observed the following message during the
test, when multiple cores are involved:

arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked

Thanks,

Ray

On 6/9/17 12:28 PM, Nate Watterson wrote:
> Hi Robin,
> 
> On 6/8/2017 7:51 AM, Robin Murphy wrote:
>> Hi all,
>>
>> Here's the cleaned up nominally-final version of the patches everybody's
>> keen to see. #1 is just a non-critical thing-I-spotted-in-passing fix,
>> #2-#4 do some preparatory work (and bid farewell to everyone's least
>> favourite bit of code, hooray!), and #5-#8 do the dirty deed itself.
>>
>> The branch I've previously shared has been updated too:
>>
>>    git://linux-arm.org/linux-rm  iommu/pgtable
>>
>> All feedback welcome, as I'd really like to land this for 4.13.
>>
> 
> I tested the series on a QDF2400 development platform and see notable
> performance improvements particularly in workloads that make concurrent
> accesses to a single iommu_domain.
> 
>> Robin.
>>
>>
>> Robin Murphy (8):
>>    iommu/io-pgtable-arm-v7s: Check table PTEs more precisely
>>    iommu/io-pgtable-arm: Improve split_blk_unmap
>>    iommu/io-pgtable-arm-v7s: Refactor split_blk_unmap
>>    iommu/io-pgtable: Introduce explicit coherency
>>    iommu/io-pgtable-arm: Support lockless operation
>>    iommu/io-pgtable-arm-v7s: Support lockless operation
>>    iommu/arm-smmu: Remove io-pgtable spinlock
>>    iommu/arm-smmu-v3: Remove io-pgtable spinlock
>>
>>   drivers/iommu/arm-smmu-v3.c        |  36 ++-----
>>   drivers/iommu/arm-smmu.c           |  48 ++++------
>>   drivers/iommu/io-pgtable-arm-v7s.c | 173
>> +++++++++++++++++++++------------
>>   drivers/iommu/io-pgtable-arm.c     | 190
>> ++++++++++++++++++++++++-------------
>>   drivers/iommu/io-pgtable.h         |   6 ++
>>   5 files changed, 268 insertions(+), 185 deletions(-)
>>
> 

[PATCH 0/8] io-pgtable lock removal

[John Garry]

On 15/06/2017 01:40, Ray Jui via iommu wrote:

Hi Robin,

wangzhou tested this patchset on our SMMUv3-based development board with 
a 10G PCI NIC card.

Currently we see a ~17% performance (throughput) drop when enabling the 
SMMU, but only a ~8% drop with your patchset.

FYI, for our integrated storage and network adapter, we see a big 
performance hit (maybe 40%) when enabling the SMMU with or without the 
patchset. Leizhen has been doing some investigation on this.

Thanks,
John

> Hi Robin,
>
> I have applied this patch series on top of v4.12-rc4, and ran various
> Ethernet and NVMf target throughput tests on it.
>
> To give you some background of my setup:
>
> The system is a ARMv8 based system with 8 cores. It has various PCIe
> root complexes that can be used to connect to PCIe endpoint devices
> including NIC cards and NVMe SSDs.
>
> I'm particularly interested in the performance of the PCIe root complex
> that connects to the NIC card, and during my test, IOMMU is
> enabled/disabled against that particular PCIe root complex. The root
> complexes connected to NVMe SSDs remain unchanged (i.e., without IOMMU).
>
> For the Ethernet throughput out of 50G link:
>
> Note during the multiple TCP session test, each session will be spread
> to different CPU cores for optimized performance
>
> Without IOMMU:
>
> TX TCP x1 - 29.7 Gbps
> TX TCP x4 - 30.5 Gbps
> TX TCP x8 - 28 Gbps
>
> RX TCP x1 - 15 Gbps
> RX TCP x4 - 33.7 Gbps
> RX TCP x8 - 36 Gbps
>
> With IOMMU, but without your latest patch:
>
> TX TCP x1 - 15.2 Gbps
> TX TCP x4 - 14.3 Gbps
> TX TCP x8 - 13 Gbps
>
> RX TCP x1 - 7.88 Gbps
> RX TCP x4 - 13.2 Gbps
> RX TCP x8 - 12.6 Gbps
>
> With IOMMU and your latest patch:
>
> TX TCP x1 - 21.4 Gbps
> TX TCP x4 - 30.5 Gbps
> TX TCP x8 - 21.3 Gbps
>
> RX TCP x1 - 7.7 Gbps
> RX TCP x4 - 20.1 Gbps
> RX TCP x8 - 27.1 Gbps
>
> With the NVMf target test with 4 SSDs, fio based test, random read, 4k,
> 8 jobs:
>
> Without IOMMU:
>
> IOPS = 1080K
>
> With IOMMU, but without your latest patch:
>
> IOPS = 520K
>
> With IOMMU and your latest patch:
>
> IOPS = 500K ~ 850K (a lot of variation observed during the same test run)
>
> As you can see, performance has improved significantly with this patch
> series! That is very impressive!
>
> However, it is still off, compared to the test runs without the IOMMU.
> I'm wondering if more improvement is expected.
>
> In addition, a much larger throughput variation is observed in the tests
> with these latest patches, when multiple CPUs are involved. I'm
> wondering if that is caused by some remaining lock in the driver?
>
> Also, in a few occasions, I observed the following message during the
> test, when multiple cores are involved:
>
> arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked
>
> Thanks,
>
> Ray
>
> On 6/9/17 12:28 PM, Nate Watterson wrote:
>> Hi Robin,
>>
>> On 6/8/2017 7:51 AM, Robin Murphy wrote:
>>> Hi all,
>>>
>>> Here's the cleaned up nominally-final version of the patches everybody's
>>> keen to see. #1 is just a non-critical thing-I-spotted-in-passing fix,
>>> #2-#4 do some preparatory work (and bid farewell to everyone's least
>>> favourite bit of code, hooray!), and #5-#8 do the dirty deed itself.
>>>
>>> The branch I've previously shared has been updated too:
>>>
>>>    git://linux-arm.org/linux-rm  iommu/pgtable
>>>
>>> All feedback welcome, as I'd really like to land this for 4.13.
>>>
>>
>> I tested the series on a QDF2400 development platform and see notable
>> performance improvements particularly in workloads that make concurrent
>> accesses to a single iommu_domain.
>>
>>> Robin.
>>>
>>>
>>> Robin Murphy (8):
>>>    iommu/io-pgtable-arm-v7s: Check table PTEs more precisely
>>>    iommu/io-pgtable-arm: Improve split_blk_unmap
>>>    iommu/io-pgtable-arm-v7s: Refactor split_blk_unmap
>>>    iommu/io-pgtable: Introduce explicit coherency
>>>    iommu/io-pgtable-arm: Support lockless operation
>>>    iommu/io-pgtable-arm-v7s: Support lockless operation
>>>    iommu/arm-smmu: Remove io-pgtable spinlock
>>>    iommu/arm-smmu-v3: Remove io-pgtable spinlock
>>>
>>>   drivers/iommu/arm-smmu-v3.c        |  36 ++-----
>>>   drivers/iommu/arm-smmu.c           |  48 ++++------
>>>   drivers/iommu/io-pgtable-arm-v7s.c | 173
>>> +++++++++++++++++++++------------
>>>   drivers/iommu/io-pgtable-arm.c     | 190
>>> ++++++++++++++++++++++++-------------
>>>   drivers/iommu/io-pgtable.h         |   6 ++
>>>   5 files changed, 268 insertions(+), 185 deletions(-)
>>>
>>
> _______________________________________________
> iommu mailing list
> iommu at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
> .
>

[PATCH 0/8] io-pgtable lock removal

[Robin Murphy]

On 15/06/17 01:40, Ray Jui wrote:
> Hi Robin,
> 
> I have applied this patch series on top of v4.12-rc4, and ran various
> Ethernet and NVMf target throughput tests on it.
> 
> To give you some background of my setup:
> 
> The system is a ARMv8 based system with 8 cores. It has various PCIe
> root complexes that can be used to connect to PCIe endpoint devices
> including NIC cards and NVMe SSDs.
> 
> I'm particularly interested in the performance of the PCIe root complex
> that connects to the NIC card, and during my test, IOMMU is
> enabled/disabled against that particular PCIe root complex. The root
> complexes connected to NVMe SSDs remain unchanged (i.e., without IOMMU).
> 
> For the Ethernet throughput out of 50G link:
> 
> Note during the multiple TCP session test, each session will be spread
> to different CPU cores for optimized performance
> 
> Without IOMMU:
> 
> TX TCP x1 - 29.7 Gbps
> TX TCP x4 - 30.5 Gbps
> TX TCP x8 - 28 Gbps
> 
> RX TCP x1 - 15 Gbps
> RX TCP x4 - 33.7 Gbps
> RX TCP x8 - 36 Gbps
> 
> With IOMMU, but without your latest patch:
> 
> TX TCP x1 - 15.2 Gbps
> TX TCP x4 - 14.3 Gbps
> TX TCP x8 - 13 Gbps
> 
> RX TCP x1 - 7.88 Gbps
> RX TCP x4 - 13.2 Gbps
> RX TCP x8 - 12.6 Gbps
> 
> With IOMMU and your latest patch:
> 
> TX TCP x1 - 21.4 Gbps
> TX TCP x4 - 30.5 Gbps
> TX TCP x8 - 21.3 Gbps
> 
> RX TCP x1 - 7.7 Gbps
> RX TCP x4 - 20.1 Gbps
> RX TCP x8 - 27.1 Gbps

Cool, those seem more or less in line with expectations. Nate's
currently cooking a patch to further reduce the overhead when unmapping
multi-page buffers, which we believe should make up most of the rest of
the difference.

> With the NVMf target test with 4 SSDs, fio based test, random read, 4k,
> 8 jobs:
> 
> Without IOMMU:
> 
> IOPS = 1080K
> 
> With IOMMU, but without your latest patch:
> 
> IOPS = 520K
> 
> With IOMMU and your latest patch:
> 
> IOPS = 500K ~ 850K (a lot of variation observed during the same test run)

That does seem a bit off - are you able to try some perf profiling to
get a better idea of where the overhead appears to be?

> As you can see, performance has improved significantly with this patch
> series! That is very impressive!
> 
> However, it is still off, compared to the test runs without the IOMMU.
> I'm wondering if more improvement is expected.
> 
> In addition, a much larger throughput variation is observed in the tests
> with these latest patches, when multiple CPUs are involved. I'm
> wondering if that is caused by some remaining lock in the driver?

Assuming this is the platform with MMU-500, there shouldn't be any locks
left, since that shouldn't have the hardware ATOS registers for
iova_to_phys().

> Also, in a few occasions, I observed the following message during the
> test, when multiple cores are involved:
> 
> arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked

That's particularly worrying, because it means we spent over a second
waiting for something that normally shouldn't take more than a few
hundred cycles. The only time I've ever actually seen that happen is if
TLBSYNC is issued while a context fault is pending - on MMU-500 it seems
that the sync just doesn't proceed until the fault is cleared - but that
stemmed from interrupts not being wired up correctly (on FPGAs) such
that we never saw the fault reported in the first place :/

Robin.

> 
> Thanks,
> 
> Ray
> 
> On 6/9/17 12:28 PM, Nate Watterson wrote:
>> Hi Robin,
>>
>> On 6/8/2017 7:51 AM, Robin Murphy wrote:
>>> Hi all,
>>>
>>> Here's the cleaned up nominally-final version of the patches everybody's
>>> keen to see. #1 is just a non-critical thing-I-spotted-in-passing fix,
>>> #2-#4 do some preparatory work (and bid farewell to everyone's least
>>> favourite bit of code, hooray!), and #5-#8 do the dirty deed itself.
>>>
>>> The branch I've previously shared has been updated too:
>>>
>>>    git://linux-arm.org/linux-rm  iommu/pgtable
>>>
>>> All feedback welcome, as I'd really like to land this for 4.13.
>>>
>>
>> I tested the series on a QDF2400 development platform and see notable
>> performance improvements particularly in workloads that make concurrent
>> accesses to a single iommu_domain.
>>
>>> Robin.
>>>
>>>
>>> Robin Murphy (8):
>>>    iommu/io-pgtable-arm-v7s: Check table PTEs more precisely
>>>    iommu/io-pgtable-arm: Improve split_blk_unmap
>>>    iommu/io-pgtable-arm-v7s: Refactor split_blk_unmap
>>>    iommu/io-pgtable: Introduce explicit coherency
>>>    iommu/io-pgtable-arm: Support lockless operation
>>>    iommu/io-pgtable-arm-v7s: Support lockless operation
>>>    iommu/arm-smmu: Remove io-pgtable spinlock
>>>    iommu/arm-smmu-v3: Remove io-pgtable spinlock
>>>
>>>   drivers/iommu/arm-smmu-v3.c        |  36 ++-----
>>>   drivers/iommu/arm-smmu.c           |  48 ++++------
>>>   drivers/iommu/io-pgtable-arm-v7s.c | 173
>>> +++++++++++++++++++++------------
>>>   drivers/iommu/io-pgtable-arm.c     | 190
>>> ++++++++++++++++++++++++-------------
>>>   drivers/iommu/io-pgtable.h         |   6 ++
>>>   5 files changed, 268 insertions(+), 185 deletions(-)
>>>
>>

[PATCH 0/8] io-pgtable lock removal

[Ray Jui]

Hi Robin,

On 6/20/17 6:37 AM, Robin Murphy wrote:
> On 15/06/17 01:40, Ray Jui wrote:
>> Hi Robin,
>>
>> I have applied this patch series on top of v4.12-rc4, and ran various
>> Ethernet and NVMf target throughput tests on it.
>>
>> To give you some background of my setup:
>>
>> The system is a ARMv8 based system with 8 cores. It has various PCIe
>> root complexes that can be used to connect to PCIe endpoint devices
>> including NIC cards and NVMe SSDs.
>>
>> I'm particularly interested in the performance of the PCIe root complex
>> that connects to the NIC card, and during my test, IOMMU is
>> enabled/disabled against that particular PCIe root complex. The root
>> complexes connected to NVMe SSDs remain unchanged (i.e., without IOMMU).
>>
>> For the Ethernet throughput out of 50G link:
>>
>> Note during the multiple TCP session test, each session will be spread
>> to different CPU cores for optimized performance
>>
>> Without IOMMU:
>>
>> TX TCP x1 - 29.7 Gbps
>> TX TCP x4 - 30.5 Gbps
>> TX TCP x8 - 28 Gbps
>>
>> RX TCP x1 - 15 Gbps
>> RX TCP x4 - 33.7 Gbps
>> RX TCP x8 - 36 Gbps
>>
>> With IOMMU, but without your latest patch:
>>
>> TX TCP x1 - 15.2 Gbps
>> TX TCP x4 - 14.3 Gbps
>> TX TCP x8 - 13 Gbps
>>
>> RX TCP x1 - 7.88 Gbps
>> RX TCP x4 - 13.2 Gbps
>> RX TCP x8 - 12.6 Gbps
>>
>> With IOMMU and your latest patch:
>>
>> TX TCP x1 - 21.4 Gbps
>> TX TCP x4 - 30.5 Gbps
>> TX TCP x8 - 21.3 Gbps
>>
>> RX TCP x1 - 7.7 Gbps
>> RX TCP x4 - 20.1 Gbps
>> RX TCP x8 - 27.1 Gbps
> 
> Cool, those seem more or less in line with expectations. Nate's
> currently cooking a patch to further reduce the overhead when unmapping
> multi-page buffers, which we believe should make up most of the rest of
> the difference.
> 

That's great to hear!

>> With the NVMf target test with 4 SSDs, fio based test, random read, 4k,
>> 8 jobs:
>>
>> Without IOMMU:
>>
>> IOPS = 1080K
>>
>> With IOMMU, but without your latest patch:
>>
>> IOPS = 520K
>>
>> With IOMMU and your latest patch:
>>
>> IOPS = 500K ~ 850K (a lot of variation observed during the same test run)
> 
> That does seem a bit off - are you able to try some perf profiling to
> get a better idea of where the overhead appears to be?
> 

I haven't had any time to look into this closer. But when I have a
chance, I will take a look (but that will not be anytime soon).

>> As you can see, performance has improved significantly with this patch
>> series! That is very impressive!
>>
>> However, it is still off, compared to the test runs without the IOMMU.
>> I'm wondering if more improvement is expected.
>>
>> In addition, a much larger throughput variation is observed in the tests
>> with these latest patches, when multiple CPUs are involved. I'm
>> wondering if that is caused by some remaining lock in the driver?
> 
> Assuming this is the platform with MMU-500, there shouldn't be any locks
> left, since that shouldn't have the hardware ATOS registers for
> iova_to_phys().
> 

Yes, this is with MMU-500.

>> Also, in a few occasions, I observed the following message during the
>> test, when multiple cores are involved:
>>
>> arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked
> 
> That's particularly worrying, because it means we spent over a second
> waiting for something that normally shouldn't take more than a few
> hundred cycles. The only time I've ever actually seen that happen is if
> TLBSYNC is issued while a context fault is pending - on MMU-500 it seems
> that the sync just doesn't proceed until the fault is cleared - but that
> stemmed from interrupts not being wired up correctly (on FPGAs) such
> that we never saw the fault reported in the first place :/
> 
> Robin.
> 

Okay, note the above error is reproduced only when we have a lot of TCP
sessions spread across all 8 CPU cores. It's fairly easy to reproduce in
our system. But I haven't had any time to take a closer look.

I also saw that patchset v2 is out. Based on your reply to other people,
I assume I do not need to test v2 explicitly. If you think there's a
need for me to help test v2, don't hesitate to let me know.

Thanks,

Ray

[PATCH 0/8] io-pgtable lock removal

[Will Deacon]

Hi Ray,

Robin and I have been bashing our heads against the tlb_sync_pending flag
this morning, and we reckon it could have something to do with your timeouts
on MMU-500.

On Tue, Jun 27, 2017 at 09:43:19AM -0700, Ray Jui wrote:
> >> Also, in a few occasions, I observed the following message during the
> >> test, when multiple cores are involved:
> >>
> >> arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked

The tlb_sync_pending logic was written under the assumption of a global
page-table lock, so it assumes that it only has to care about syncing
flushes from the current CPU/context. That's not true anymore, and the
current code can accidentally skip syncs and (what I think is happening in
your case) allow concurrent syncs, which will potentially lead to timeouts
if a CPU is unlucky enough to keep missing the Ack.

Please can you try the diff below and see if it fixes things for you?
This applies on top of my for-joerg/arm-smmu/updates branch, but note
that I've only shown it to the compiler. Not tested at all.

Will

--->8

diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
index 127558d83667..cd8d7aaec161 100644
--- a/drivers/iommu/io-pgtable.c
+++ b/drivers/iommu/io-pgtable.c
@@ -59,6 +59,7 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
 	iop->cookie	= cookie;
 	iop->cfg	= *cfg;
 
+	atomic_set(&iop->tlb_sync_pending, 0);
 	return &iop->ops;
 }
 
diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
index 524263a7ae6f..b64580c9d03d 100644
--- a/drivers/iommu/io-pgtable.h
+++ b/drivers/iommu/io-pgtable.h
@@ -1,5 +1,7 @@
 #ifndef __IO_PGTABLE_H
 #define __IO_PGTABLE_H
+
+#include <linux/atomic.h>
 #include <linux/bitops.h>
 
 /*
@@ -165,7 +167,7 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops);
 struct io_pgtable {
 	enum io_pgtable_fmt	fmt;
 	void			*cookie;
-	bool			tlb_sync_pending;
+	atomic_t		tlb_sync_pending;
 	struct io_pgtable_cfg	cfg;
 	struct io_pgtable_ops	ops;
 };
@@ -175,22 +177,20 @@ struct io_pgtable {
 static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop)
 {
 	iop->cfg.tlb->tlb_flush_all(iop->cookie);
-	iop->tlb_sync_pending = true;
+	atomic_set_release(&iop->tlb_sync_pending, 1);
 }
 
 static inline void io_pgtable_tlb_add_flush(struct io_pgtable *iop,
 		unsigned long iova, size_t size, size_t granule, bool leaf)
 {
 	iop->cfg.tlb->tlb_add_flush(iova, size, granule, leaf, iop->cookie);
-	iop->tlb_sync_pending = true;
+	atomic_set_release(&iop->tlb_sync_pending, 1);
 }
 
 static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
 {
-	if (iop->tlb_sync_pending) {
+	if (atomic_xchg_relaxed(&iop->tlb_sync_pending, 0))
 		iop->cfg.tlb->tlb_sync(iop->cookie);
-		iop->tlb_sync_pending = false;
-	}
 }
 
 /**

[PATCH 0/8] io-pgtable lock removal

[Ray Jui]

Hi Will/Robin,

On 6/28/17 4:46 AM, Will Deacon wrote:
> Hi Ray,
> 
> Robin and I have been bashing our heads against the tlb_sync_pending flag
> this morning, and we reckon it could have something to do with your timeouts
> on MMU-500.
> 
> On Tue, Jun 27, 2017 at 09:43:19AM -0700, Ray Jui wrote:
>>>> Also, in a few occasions, I observed the following message during the
>>>> test, when multiple cores are involved:
>>>>
>>>> arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked
> 
> The tlb_sync_pending logic was written under the assumption of a global
> page-table lock, so it assumes that it only has to care about syncing
> flushes from the current CPU/context. That's not true anymore, and the
> current code can accidentally skip syncs and (what I think is happening in
> your case) allow concurrent syncs, which will potentially lead to timeouts
> if a CPU is unlucky enough to keep missing the Ack.
> 
> Please can you try the diff below and see if it fixes things for you?
> This applies on top of my for-joerg/arm-smmu/updates branch, but note
> that I've only shown it to the compiler. Not tested at all.
> 
> Will
> 

Thanks for looking into this. I'm a bit busy at work but will certainly
find time to test the diff below. I hopefully will get to it later this
week.

Thanks,

Ray

> --->8
> 
> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
> index 127558d83667..cd8d7aaec161 100644
> --- a/drivers/iommu/io-pgtable.c
> +++ b/drivers/iommu/io-pgtable.c
> @@ -59,6 +59,7 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
>  	iop->cookie	= cookie;
>  	iop->cfg	= *cfg;
>  
> +	atomic_set(&iop->tlb_sync_pending, 0);
>  	return &iop->ops;
>  }
>  
> diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
> index 524263a7ae6f..b64580c9d03d 100644
> --- a/drivers/iommu/io-pgtable.h
> +++ b/drivers/iommu/io-pgtable.h
> @@ -1,5 +1,7 @@
>  #ifndef __IO_PGTABLE_H
>  #define __IO_PGTABLE_H
> +
> +#include <linux/atomic.h>
>  #include <linux/bitops.h>
>  
>  /*
> @@ -165,7 +167,7 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops);
>  struct io_pgtable {
>  	enum io_pgtable_fmt	fmt;
>  	void			*cookie;
> -	bool			tlb_sync_pending;
> +	atomic_t		tlb_sync_pending;
>  	struct io_pgtable_cfg	cfg;
>  	struct io_pgtable_ops	ops;
>  };
> @@ -175,22 +177,20 @@ struct io_pgtable {
>  static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop)
>  {
>  	iop->cfg.tlb->tlb_flush_all(iop->cookie);
> -	iop->tlb_sync_pending = true;
> +	atomic_set_release(&iop->tlb_sync_pending, 1);
>  }
>  
>  static inline void io_pgtable_tlb_add_flush(struct io_pgtable *iop,
>  		unsigned long iova, size_t size, size_t granule, bool leaf)
>  {
>  	iop->cfg.tlb->tlb_add_flush(iova, size, granule, leaf, iop->cookie);
> -	iop->tlb_sync_pending = true;
> +	atomic_set_release(&iop->tlb_sync_pending, 1);
>  }
>  
>  static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
>  {
> -	if (iop->tlb_sync_pending) {
> +	if (atomic_xchg_relaxed(&iop->tlb_sync_pending, 0))
>  		iop->cfg.tlb->tlb_sync(iop->cookie);
> -		iop->tlb_sync_pending = false;
> -	}
>  }
>  
>  /**
> 

[PATCH 0/8] io-pgtable lock removal

[Will Deacon]

Ray,

On Wed, Jun 28, 2017 at 10:02:35AM -0700, Ray Jui wrote:
> On 6/28/17 4:46 AM, Will Deacon wrote:
> > Robin and I have been bashing our heads against the tlb_sync_pending flag
> > this morning, and we reckon it could have something to do with your timeouts
> > on MMU-500.
> > 
> > On Tue, Jun 27, 2017 at 09:43:19AM -0700, Ray Jui wrote:
> >>>> Also, in a few occasions, I observed the following message during the
> >>>> test, when multiple cores are involved:
> >>>>
> >>>> arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked
> > 
> > The tlb_sync_pending logic was written under the assumption of a global
> > page-table lock, so it assumes that it only has to care about syncing
> > flushes from the current CPU/context. That's not true anymore, and the
> > current code can accidentally skip syncs and (what I think is happening in
> > your case) allow concurrent syncs, which will potentially lead to timeouts
> > if a CPU is unlucky enough to keep missing the Ack.
> > 
> > Please can you try the diff below and see if it fixes things for you?
> > This applies on top of my for-joerg/arm-smmu/updates branch, but note
> > that I've only shown it to the compiler. Not tested at all.
> > 
> > Will
> > 
> 
> Thanks for looking into this. I'm a bit busy at work but will certainly
> find time to test the diff below. I hopefully will get to it later this
> week.

It would be really handy if you could test this, since I think it could
cause some nasty problems if we don't get it fixed. Updated patch (with
commit message) below.

Will

--->8

>From eeb11dab63fcdd698b671a3a8c63516005caa9ec Mon Sep 17 00:00:00 2001
From: Will Deacon <will.deacon@arm.com>
Date: Thu, 29 Jun 2017 15:08:09 +0100
Subject: [PATCH] iommu/io-pgtable: Fix tlb_sync_pending flag access from
 concurrent CPUs

The tlb_sync_pending flag is used to elide back-to-back TLB sync operations
for two reasons:

  1. TLB sync operations can be expensive, and so avoiding them where we
     can is a good idea.

  2. Some hardware (mtk_iommu) locks up if it sees a TLB sync without an
     unsync'd TLB flush preceding it

The flag is set on an ->add_flush callback and cleared on a ->sync callback,
which worked nicely when all map/unmap operations where protected by a
global lock.

Unfortunately, moving to a lockless implementation means that we suddenly
have races on the flag: updates can go missing and we can end up with
back-to-back syncs once again.

This patch resolves the problem by making the tlb_sync_pending flag an
atomic_t and sorts out the ordering with respect to TLB callbacks.
Now, the flag is set with release semantics after adding a flush and
checked with an xchg operation (and subsequent control dependency) when
performing the sync. We could consider using a cmpxchg here, but we'll
likely just hit our local update to the flag anyway.

Cc: Ray Jui <ray.jui@broadcom.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Fixes: 2c3d273eabe8 ("iommu/io-pgtable-arm: Support lockless operation")
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 drivers/iommu/io-pgtable.c |  1 +
 drivers/iommu/io-pgtable.h | 12 ++++++------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
index 127558d83667..cd8d7aaec161 100644
--- a/drivers/iommu/io-pgtable.c
+++ b/drivers/iommu/io-pgtable.c
@@ -59,6 +59,7 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
 	iop->cookie	= cookie;
 	iop->cfg	= *cfg;
 
+	atomic_set(&iop->tlb_sync_pending, 0);
 	return &iop->ops;
 }
 
diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
index 524263a7ae6f..b64580c9d03d 100644
--- a/drivers/iommu/io-pgtable.h
+++ b/drivers/iommu/io-pgtable.h
@@ -1,5 +1,7 @@
 #ifndef __IO_PGTABLE_H
 #define __IO_PGTABLE_H
+
+#include <linux/atomic.h>
 #include <linux/bitops.h>
 
 /*
@@ -165,7 +167,7 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops);
 struct io_pgtable {
 	enum io_pgtable_fmt	fmt;
 	void			*cookie;
-	bool			tlb_sync_pending;
+	atomic_t		tlb_sync_pending;
 	struct io_pgtable_cfg	cfg;
 	struct io_pgtable_ops	ops;
 };
@@ -175,22 +177,20 @@ struct io_pgtable {
 static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop)
 {
 	iop->cfg.tlb->tlb_flush_all(iop->cookie);
-	iop->tlb_sync_pending = true;
+	atomic_set_release(&iop->tlb_sync_pending, 1);
 }
 
 static inline void io_pgtable_tlb_add_flush(struct io_pgtable *iop,
 		unsigned long iova, size_t size, size_t granule, bool leaf)
 {
 	iop->cfg.tlb->tlb_add_flush(iova, size, granule, leaf, iop->cookie);
-	iop->tlb_sync_pending = true;
+	atomic_set_release(&iop->tlb_sync_pending, 1);
 }
 
 static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
 {
-	if (iop->tlb_sync_pending) {
+	if (atomic_xchg_relaxed(&iop->tlb_sync_pending, 0))
 		iop->cfg.tlb->tlb_sync(iop->cookie);
-		iop->tlb_sync_pending = false;
-	}
 }
 
 /**
-- 
2.1.4

[PATCH 0/8] io-pgtable lock removal

[Ray Jui]

Hi Will,

On 7/4/17 10:31 AM, Will Deacon wrote:
> Ray,
> 
> On Wed, Jun 28, 2017 at 10:02:35AM -0700, Ray Jui wrote:
>> On 6/28/17 4:46 AM, Will Deacon wrote:
>>> Robin and I have been bashing our heads against the tlb_sync_pending flag
>>> this morning, and we reckon it could have something to do with your timeouts
>>> on MMU-500.
>>>
>>> On Tue, Jun 27, 2017 at 09:43:19AM -0700, Ray Jui wrote:
>>>>>> Also, in a few occasions, I observed the following message during the
>>>>>> test, when multiple cores are involved:
>>>>>>
>>>>>> arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked
>>>
>>> The tlb_sync_pending logic was written under the assumption of a global
>>> page-table lock, so it assumes that it only has to care about syncing
>>> flushes from the current CPU/context. That's not true anymore, and the
>>> current code can accidentally skip syncs and (what I think is happening in
>>> your case) allow concurrent syncs, which will potentially lead to timeouts
>>> if a CPU is unlucky enough to keep missing the Ack.
>>>
>>> Please can you try the diff below and see if it fixes things for you?
>>> This applies on top of my for-joerg/arm-smmu/updates branch, but note
>>> that I've only shown it to the compiler. Not tested at all.
>>>
>>> Will
>>>
>>
>> Thanks for looking into this. I'm a bit busy at work but will certainly
>> find time to test the diff below. I hopefully will get to it later this
>> week.
> 
> It would be really handy if you could test this, since I think it could
> cause some nasty problems if we don't get it fixed. Updated patch (with
> commit message) below.
> 
> Will

Yes I understand. Sorry I was way too busy last week and could not get
to it. Will definitely find time to test this ASAP.

Regards,

Ray

> 
> --->8
> 
> From eeb11dab63fcdd698b671a3a8c63516005caa9ec Mon Sep 17 00:00:00 2001
> From: Will Deacon <will.deacon@arm.com>
> Date: Thu, 29 Jun 2017 15:08:09 +0100
> Subject: [PATCH] iommu/io-pgtable: Fix tlb_sync_pending flag access from
>  concurrent CPUs
> 
> The tlb_sync_pending flag is used to elide back-to-back TLB sync operations
> for two reasons:
> 
>   1. TLB sync operations can be expensive, and so avoiding them where we
>      can is a good idea.
> 
>   2. Some hardware (mtk_iommu) locks up if it sees a TLB sync without an
>      unsync'd TLB flush preceding it
> 
> The flag is set on an ->add_flush callback and cleared on a ->sync callback,
> which worked nicely when all map/unmap operations where protected by a
> global lock.
> 
> Unfortunately, moving to a lockless implementation means that we suddenly
> have races on the flag: updates can go missing and we can end up with
> back-to-back syncs once again.
> 
> This patch resolves the problem by making the tlb_sync_pending flag an
> atomic_t and sorts out the ordering with respect to TLB callbacks.
> Now, the flag is set with release semantics after adding a flush and
> checked with an xchg operation (and subsequent control dependency) when
> performing the sync. We could consider using a cmpxchg here, but we'll
> likely just hit our local update to the flag anyway.
> 
> Cc: Ray Jui <ray.jui@broadcom.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Fixes: 2c3d273eabe8 ("iommu/io-pgtable-arm: Support lockless operation")
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  drivers/iommu/io-pgtable.c |  1 +
>  drivers/iommu/io-pgtable.h | 12 ++++++------
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
> index 127558d83667..cd8d7aaec161 100644
> --- a/drivers/iommu/io-pgtable.c
> +++ b/drivers/iommu/io-pgtable.c
> @@ -59,6 +59,7 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
>  	iop->cookie	= cookie;
>  	iop->cfg	= *cfg;
>  
> +	atomic_set(&iop->tlb_sync_pending, 0);
>  	return &iop->ops;
>  }
>  
> diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
> index 524263a7ae6f..b64580c9d03d 100644
> --- a/drivers/iommu/io-pgtable.h
> +++ b/drivers/iommu/io-pgtable.h
> @@ -1,5 +1,7 @@
>  #ifndef __IO_PGTABLE_H
>  #define __IO_PGTABLE_H
> +
> +#include <linux/atomic.h>
>  #include <linux/bitops.h>
>  
>  /*
> @@ -165,7 +167,7 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops);
>  struct io_pgtable {
>  	enum io_pgtable_fmt	fmt;
>  	void			*cookie;
> -	bool			tlb_sync_pending;
> +	atomic_t		tlb_sync_pending;
>  	struct io_pgtable_cfg	cfg;
>  	struct io_pgtable_ops	ops;
>  };
> @@ -175,22 +177,20 @@ struct io_pgtable {
>  static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop)
>  {
>  	iop->cfg.tlb->tlb_flush_all(iop->cookie);
> -	iop->tlb_sync_pending = true;
> +	atomic_set_release(&iop->tlb_sync_pending, 1);
>  }
>  
>  static inline void io_pgtable_tlb_add_flush(struct io_pgtable *iop,
>  		unsigned long iova, size_t size, size_t granule, bool leaf)
>  {
>  	iop->cfg.tlb->tlb_add_flush(iova, size, granule, leaf, iop->cookie);
> -	iop->tlb_sync_pending = true;
> +	atomic_set_release(&iop->tlb_sync_pending, 1);
>  }
>  
>  static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
>  {
> -	if (iop->tlb_sync_pending) {
> +	if (atomic_xchg_relaxed(&iop->tlb_sync_pending, 0))
>  		iop->cfg.tlb->tlb_sync(iop->cookie);
> -		iop->tlb_sync_pending = false;
> -	}
>  }
>  
>  /**
> 

[PATCH 0/8] io-pgtable lock removal

[Ray Jui]

Hi Will/Robin,

Has anything functionally changed between PATCH v2 and v1? I'm seeing a
very different L2 throughput with v2 (in general a lot worse with v2 vs.
v1); however, I'm currently unable to reproduce the TLB sync timed out
issue with v2 (without the patch from Will's email).

It could also be something else that has changed in my setup, but so far
I have not yet been able to spot anything wrong in the setup.

Thanks,

Ray

On 7/4/17 10:39 AM, Ray Jui wrote:
> Hi Will,
> 
> On 7/4/17 10:31 AM, Will Deacon wrote:
>> Ray,
>>
>> On Wed, Jun 28, 2017 at 10:02:35AM -0700, Ray Jui wrote:
>>> On 6/28/17 4:46 AM, Will Deacon wrote:
>>>> Robin and I have been bashing our heads against the tlb_sync_pending flag
>>>> this morning, and we reckon it could have something to do with your timeouts
>>>> on MMU-500.
>>>>
>>>> On Tue, Jun 27, 2017 at 09:43:19AM -0700, Ray Jui wrote:
>>>>>>> Also, in a few occasions, I observed the following message during the
>>>>>>> test, when multiple cores are involved:
>>>>>>>
>>>>>>> arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked
>>>>
>>>> The tlb_sync_pending logic was written under the assumption of a global
>>>> page-table lock, so it assumes that it only has to care about syncing
>>>> flushes from the current CPU/context. That's not true anymore, and the
>>>> current code can accidentally skip syncs and (what I think is happening in
>>>> your case) allow concurrent syncs, which will potentially lead to timeouts
>>>> if a CPU is unlucky enough to keep missing the Ack.
>>>>
>>>> Please can you try the diff below and see if it fixes things for you?
>>>> This applies on top of my for-joerg/arm-smmu/updates branch, but note
>>>> that I've only shown it to the compiler. Not tested at all.
>>>>
>>>> Will
>>>>
>>>
>>> Thanks for looking into this. I'm a bit busy at work but will certainly
>>> find time to test the diff below. I hopefully will get to it later this
>>> week.
>>
>> It would be really handy if you could test this, since I think it could
>> cause some nasty problems if we don't get it fixed. Updated patch (with
>> commit message) below.
>>
>> Will
> 
> Yes I understand. Sorry I was way too busy last week and could not get
> to it. Will definitely find time to test this ASAP.
> 
> Regards,
> 
> Ray
> 
>>
>> --->8
>>
>> From eeb11dab63fcdd698b671a3a8c63516005caa9ec Mon Sep 17 00:00:00 2001
>> From: Will Deacon <will.deacon@arm.com>
>> Date: Thu, 29 Jun 2017 15:08:09 +0100
>> Subject: [PATCH] iommu/io-pgtable: Fix tlb_sync_pending flag access from
>>  concurrent CPUs
>>
>> The tlb_sync_pending flag is used to elide back-to-back TLB sync operations
>> for two reasons:
>>
>>   1. TLB sync operations can be expensive, and so avoiding them where we
>>      can is a good idea.
>>
>>   2. Some hardware (mtk_iommu) locks up if it sees a TLB sync without an
>>      unsync'd TLB flush preceding it
>>
>> The flag is set on an ->add_flush callback and cleared on a ->sync callback,
>> which worked nicely when all map/unmap operations where protected by a
>> global lock.
>>
>> Unfortunately, moving to a lockless implementation means that we suddenly
>> have races on the flag: updates can go missing and we can end up with
>> back-to-back syncs once again.
>>
>> This patch resolves the problem by making the tlb_sync_pending flag an
>> atomic_t and sorts out the ordering with respect to TLB callbacks.
>> Now, the flag is set with release semantics after adding a flush and
>> checked with an xchg operation (and subsequent control dependency) when
>> performing the sync. We could consider using a cmpxchg here, but we'll
>> likely just hit our local update to the flag anyway.
>>
>> Cc: Ray Jui <ray.jui@broadcom.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Fixes: 2c3d273eabe8 ("iommu/io-pgtable-arm: Support lockless operation")
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>> ---
>>  drivers/iommu/io-pgtable.c |  1 +
>>  drivers/iommu/io-pgtable.h | 12 ++++++------
>>  2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
>> index 127558d83667..cd8d7aaec161 100644
>> --- a/drivers/iommu/io-pgtable.c
>> +++ b/drivers/iommu/io-pgtable.c
>> @@ -59,6 +59,7 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
>>  	iop->cookie	= cookie;
>>  	iop->cfg	= *cfg;
>>  
>> +	atomic_set(&iop->tlb_sync_pending, 0);
>>  	return &iop->ops;
>>  }
>>  
>> diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
>> index 524263a7ae6f..b64580c9d03d 100644
>> --- a/drivers/iommu/io-pgtable.h
>> +++ b/drivers/iommu/io-pgtable.h
>> @@ -1,5 +1,7 @@
>>  #ifndef __IO_PGTABLE_H
>>  #define __IO_PGTABLE_H
>> +
>> +#include <linux/atomic.h>
>>  #include <linux/bitops.h>
>>  
>>  /*
>> @@ -165,7 +167,7 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops);
>>  struct io_pgtable {
>>  	enum io_pgtable_fmt	fmt;
>>  	void			*cookie;
>> -	bool			tlb_sync_pending;
>> +	atomic_t		tlb_sync_pending;
>>  	struct io_pgtable_cfg	cfg;
>>  	struct io_pgtable_ops	ops;
>>  };
>> @@ -175,22 +177,20 @@ struct io_pgtable {
>>  static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop)
>>  {
>>  	iop->cfg.tlb->tlb_flush_all(iop->cookie);
>> -	iop->tlb_sync_pending = true;
>> +	atomic_set_release(&iop->tlb_sync_pending, 1);
>>  }
>>  
>>  static inline void io_pgtable_tlb_add_flush(struct io_pgtable *iop,
>>  		unsigned long iova, size_t size, size_t granule, bool leaf)
>>  {
>>  	iop->cfg.tlb->tlb_add_flush(iova, size, granule, leaf, iop->cookie);
>> -	iop->tlb_sync_pending = true;
>> +	atomic_set_release(&iop->tlb_sync_pending, 1);
>>  }
>>  
>>  static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
>>  {
>> -	if (iop->tlb_sync_pending) {
>> +	if (atomic_xchg_relaxed(&iop->tlb_sync_pending, 0))
>>  		iop->cfg.tlb->tlb_sync(iop->cookie);
>> -		iop->tlb_sync_pending = false;
>> -	}
>>  }
>>  
>>  /**
>>

[PATCH 0/8] io-pgtable lock removal

[Will Deacon]

On Tue, Jul 04, 2017 at 06:45:17PM -0700, Ray Jui wrote:
> Hi Will/Robin,
> 
> Has anything functionally changed between PATCH v2 and v1? I'm seeing a
> very different L2 throughput with v2 (in general a lot worse with v2 vs.
> v1); however, I'm currently unable to reproduce the TLB sync timed out
> issue with v2 (without the patch from Will's email).
> 
> It could also be something else that has changed in my setup, but so far
> I have not yet been able to spot anything wrong in the setup.

There were fixes, and that initially involved a DSB that was found to be
expensive. The patches queued in -next should have that addressed, so please
use those (or my for-joerg/arm-smmu/updates branch).

Will

[PATCH 0/8] io-pgtable lock removal

[Ray Jui]

Hi Will,

On 7/5/17 1:41 AM, Will Deacon wrote:
> On Tue, Jul 04, 2017 at 06:45:17PM -0700, Ray Jui wrote:
>> Hi Will/Robin,
>>
>> Has anything functionally changed between PATCH v2 and v1? I'm seeing a
>> very different L2 throughput with v2 (in general a lot worse with v2 vs.
>> v1); however, I'm currently unable to reproduce the TLB sync timed out
>> issue with v2 (without the patch from Will's email).
>>
>> It could also be something else that has changed in my setup, but so far
>> I have not yet been able to spot anything wrong in the setup.
> 
> There were fixes, and that initially involved a DSB that was found to be
> expensive. The patches queued in -next should have that addressed, so please
> use those (or my for-joerg/arm-smmu/updates branch).
> 
> Will
> 

That was my bad yesterday. I was in a rush and the setup was incorrect.

I redo my Ethernet performance test with both PATCH v1 and v2 today, and
can confirm the performance is consistent between v1 and v2 as expected.

I also made sure the following message can still be reproduced with
patch set v2:

arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked

Then I proceeded to apply your patch that attempt to fix the deadlock
issue. I also added a print to ensure I'm running the correct build with
your fix patch applied:

diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
index cd8d7aa..01a6fa8 100644
--- a/drivers/iommu/io-pgtable.c
+++ b/drivers/iommu/io-pgtable.c
@@ -60,6 +60,7 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum
io_pgtable_fmt fmt,
        iop->cfg        = *cfg;

        atomic_set(&iop->tlb_sync_pending, 0);
+       pr_err("tlb sync pending cleared\n");
        return &iop->ops;
 }

root at bcm958742k:~# dmesg | grep tlb
[    6.495754] tlb sync pending cleared
[    6.509934] tlb sync pending cleared
[    6.510067] tlb sync pending cleared
[    6.510207] tlb sync pending cleared
[    9.864543] tlb sync pending cleared
[    9.874019] tlb sync pending cleared
[    9.979311] tlb sync pending cleared
[   39.616465] tlb sync pending cleared


However, with the fix patch, I can still see the deadlock message when I
have > 32 iperf TX threads active in the system:

root at bcm958742k:~# iperf -c 192.168.1.20 -P64
------------------------------------------------------------
Client connecting to 192.168.1.20, TCP port 5001
TCP window size: 85.0 KByte (default)
------------------------------------------------------------
[ 66] local 192.168.1.10 port 48802 connected with 192.168.1.20 port 5001
[  6] local 192.168.1.10 port 48680 connected with 192.168.1.20 port 5001
[ 22] local 192.168.1.10 port 48710 connected with 192.168.1.20 port 5001
[ 50] local 192.168.1.10 port 48770 connected with 192.168.1.20 port 5001
[ 32] local 192.168.1.10 port 48734 connected with 192.168.1.20 port 5001
[ 23] local 192.168.1.10 port 48716 connected with 192.168.1.20 port 5001
[ 21] local 192.168.1.10 port 48712 connected with 192.168.1.20 port 5001
[ 10] local 192.168.1.10 port 48688 connected with 192.168.1.20 port 5001
[ 56] local 192.168.1.10 port 48782 connected with 192.168.1.20 port 5001
[ 31] local 192.168.1.10 port 48732 connected with 192.168.1.20 port 5001
[ 63] local 192.168.1.10 port 48796 connected with 192.168.1.20 port 5001
[ 58] local 192.168.1.10 port 48786 connected with 192.168.1.20 port 5001
[ 19] local 192.168.1.10 port 48706 connected with 192.168.1.20 port 5001
[ 47] local 192.168.1.10 port 48764 connected with 192.168.1.20 port 5001
[ 25] local 192.168.1.10 port 48720 connected with 192.168.1.20 port 5001
[ 34] local 192.168.1.10 port 48738 connected with 192.168.1.20 port 5001
[ 64] local 192.168.1.10 port 48798 connected with 192.168.1.20 port 5001
[ 52] local 192.168.1.10 port 48774 connected with 192.168.1.20 port 5001
[ 59] local 192.168.1.10 port 48788 connected with 192.168.1.20 port 5001
[ 30] local 192.168.1.10 port 48730 connected with 192.168.1.20 port 5001
[ 65] local 192.168.1.10 port 48800 connected with 192.168.1.20 port 5001
[ 17] local 192.168.1.10 port 48702 connected with 192.168.1.20 port 5001
[ 20] local 192.168.1.10 port 48708 connected with 192.168.1.20 port 5001
[ 44] local 192.168.1.10 port 48758 connected with 192.168.1.20 port 5001
[ 55] local 192.168.1.10 port 48780 connected with 192.168.1.20 port 5001
[ 33] local 192.168.1.10 port 48736 connected with 192.168.1.20 port 5001
[ 62] local 192.168.1.10 port 48794 connected with 192.168.1.20 port 5001
[ 60] local 192.168.1.10 port 48790 connected with 192.168.1.20 port 5001
[ 14] local 192.168.1.10 port 48696 connected with 192.168.1.20 port 5001
[ 28] local 192.168.1.10 port 48726 connected with 192.168.1.20 port 5001
[ 53] local 192.168.1.10 port 48776 connected with 192.168.1.20 port 5001
[ 42] local 192.168.1.10 port 48754 connected with 192.168.1.20 port 5001
[ 16] local 192.168.1.10 port 48700 connected with 192.168.1.20 port 5001
[  3] local 192.168.1.10 port 48678 connected with 192.168.1.20 port 5001
[ 29] local 192.168.1.10 port 48728 connected with 192.168.1.20 port 5001
[ 27] local 192.168.1.10 port 48724 connected with 192.168.1.20 port 5001
[ 38] local 192.168.1.10 port 48746 connected with 192.168.1.20 port 5001
[ 13] local 192.168.1.10 port 48694 connected with 192.168.1.20 port 5001
[ 12] local 192.168.1.10 port 48692 connected with 192.168.1.20 port 5001
[ 41] local 192.168.1.10 port 48752 connected with 192.168.1.20 port 5001
[ 26] local 192.168.1.10 port 48722 connected with 192.168.1.20 port 5001
[ 11] local 192.168.1.10 port 48690 connected with 192.168.1.20 port 5001
[ 24] local 192.168.1.10 port 48718 connected with 192.168.1.20 port 5001
[ 15] local 192.168.1.10 port 48698 connected with 192.168.1.20 port 5001
[ 37] local 192.168.1.10 port 48744 connected with 192.168.1.20 port 5001
[ 36] local 192.168.1.10 port 48742 connected with 192.168.1.20 port 5001
[ 43] local 192.168.1.10 port 48756 connected with 192.168.1.20 port 5001
[ 48] local 192.168.1.10 port 48766 connected with 192.168.1.20 port 5001
[ 45] local 192.168.1.10 port 48760 connected with 192.168.1.20 port 5001
[ 35] local 192.168.1.10 port 48740 connected with 192.168.1.20 port 5001
[  7] local 192.168.1.10 port 48672 connected with 192.168.1.20 port 5001
[ 39] local 192.168.1.10 port 48748 connected with 192.168.1.20 port 5001
[ 40] local 192.168.1.10 port 48750 connected with 192.168.1.20 port 5001
[  8] local 192.168.1.10 port 48682 connected with 192.168.1.20 port 5001
[ 18] local 192.168.1.10 port 48704 connected with 192.168.1.20 port 5001
[  4] local 192.168.1.10 port 48674 connected with 192.168.1.20 port 5001
[ 46] local 192.168.1.10 port 48762 connected with 192.168.1.20 port 5001
[  5] local 192.168.1.10 port 48676 connected with 192.168.1.20 port 5001
[ 49] local 192.168.1.10 port 48768 connected with 192.168.1.20 port 5001
[ 54] local 192.168.1.10 port 48778 connected with 192.168.1.20 port 5001
[ 57] local 192.168.1.10 port 48784 connected with 192.168.1.20 port 5001
[ 51] local 192.168.1.10 port 48772 connected with 192.168.1.20 port 5001
[  9] local 192.168.1.10 port 48686 connected with 192.168.1.20 port 5001
[ 61] local 192.168.1.10 port 48792 connected with 192.168.1.20 port 5001
[  698.284709] arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be
deadlocked
[  699.386010] arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be
deadlocked
[  702.064900] arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be
deadlocked
[ ID] Interval       Transfer     Bandwidth
[ 26]  0.0-10.0 sec   544 MBytes   456 Mbits/sec
[  6]  0.0-10.0 sec   382 MBytes   320 Mbits/sec
[ 22]  0.0-10.1 sec   667 MBytes   556 Mbits/sec
[ 50]  0.0-10.1 sec   245 MBytes   204 Mbits/sec
[ 21]  0.0-10.1 sec   291 MBytes   242 Mbits/sec
[ 56]  0.0-10.1 sec   256 MBytes   213 Mbits/sec
[ 19]  0.0-10.0 sec  17.0 MBytes  14.2 Mbits/sec
[ 47]  0.0-10.0 sec   357 MBytes   299 Mbits/sec
[ 52]  0.0-10.1 sec   121 MBytes   101 Mbits/sec
[ 59]  0.0-10.0 sec   364 MBytes   304 Mbits/sec
[ 30]  0.0-10.0 sec   469 MBytes   391 Mbits/sec
[ 20]  0.0-10.0 sec   435 MBytes   364 Mbits/sec
[ 44]  0.0-10.0 sec   379 MBytes   317 Mbits/sec
[ 33]  0.0-10.0 sec   468 MBytes   392 Mbits/sec
[ 60]  0.0-10.0 sec   178 MBytes   149 Mbits/sec
[ 14]  0.0-10.1 sec   539 MBytes   449 Mbits/sec
[ 28]  0.0-10.1 sec  60.6 MBytes  50.5 Mbits/sec
[ 42]  0.0-10.1 sec   365 MBytes   304 Mbits/sec
[  3]  0.0-10.1 sec   109 MBytes  90.5 Mbits/sec
[ 29]  0.0-10.1 sec   473 MBytes   395 Mbits/sec
[ 38]  0.0-10.0 sec   254 MBytes   212 Mbits/sec
[ 13]  0.0-10.0 sec   523 MBytes   438 Mbits/sec
[ 12]  0.0-10.1 sec   182 MBytes   152 Mbits/sec
[ 11]  0.0-10.1 sec   130 MBytes   109 Mbits/sec
[ 15]  0.0-10.1 sec   174 MBytes   145 Mbits/sec
[ 43]  0.0-10.1 sec   399 MBytes   333 Mbits/sec
[ 48]  0.0-10.1 sec   543 MBytes   452 Mbits/sec
[ 45]  0.0-10.1 sec  69.1 MBytes  57.6 Mbits/sec
[ 35]  0.0-10.1 sec  54.0 MBytes  45.0 Mbits/sec
[  4]  0.0-10.0 sec   116 MBytes  97.4 Mbits/sec
[ 46]  0.0-10.1 sec   300 MBytes   250 Mbits/sec
[ 51]  0.0-10.1 sec  49.8 MBytes  41.5 Mbits/sec
[ 61]  0.0-10.1 sec   102 MBytes  85.0 Mbits/sec
[ 23]  0.0-10.1 sec  1.64 GBytes  1.39 Gbits/sec
[ 10]  0.0-10.1 sec   210 MBytes   174 Mbits/sec
[ 31]  0.0-10.1 sec  1.16 GBytes   988 Mbits/sec
[ 63]  0.0-10.1 sec   468 MBytes   389 Mbits/sec
[ 25]  0.0-10.1 sec   457 MBytes   381 Mbits/sec
[ 34]  0.0-10.1 sec   332 MBytes   276 Mbits/sec
[ 64]  0.0-10.1 sec   280 MBytes   233 Mbits/sec
[ 17]  0.0-10.1 sec   425 MBytes   354 Mbits/sec
[ 62]  0.0-10.1 sec   616 MBytes   513 Mbits/sec
[ 53]  0.0-10.1 sec   289 MBytes   241 Mbits/sec
[ 16]  0.0-10.1 sec   661 MBytes   550 Mbits/sec
[ 27]  0.0-10.1 sec   298 MBytes   249 Mbits/sec
[ 41]  0.0-10.1 sec  11.5 MBytes  9.57 Mbits/sec
[ 37]  0.0-10.1 sec   945 MBytes   786 Mbits/sec
[ 36]  0.0-10.1 sec   164 MBytes   136 Mbits/sec
[ 40]  0.0-10.1 sec   782 MBytes   650 Mbits/sec
[  8]  0.0-10.1 sec   883 MBytes   734 Mbits/sec
[ 18]  0.0-10.1 sec   140 MBytes   117 Mbits/sec
[  5]  0.0-10.1 sec   366 MBytes   305 Mbits/sec
[ 49]  0.0-10.1 sec   229 MBytes   191 Mbits/sec
[ 54]  0.0-10.1 sec   884 MBytes   736 Mbits/sec
[ 57]  0.0-10.1 sec  56.6 MBytes  47.1 Mbits/sec
[  9]  0.0-10.1 sec  72.8 MBytes  60.4 Mbits/sec
[ 66]  0.0-10.1 sec   170 MBytes   141 Mbits/sec
[ 32]  0.0-10.1 sec   201 MBytes   167 Mbits/sec
[ 58]  0.0-10.1 sec   381 MBytes   317 Mbits/sec
[ 65]  0.0-10.1 sec   373 MBytes   310 Mbits/sec
[ 55]  0.0-10.1 sec  98.0 MBytes  81.5 Mbits/sec
[ 24]  0.0-10.1 sec   292 MBytes   243 Mbits/sec
[  7]  0.0-10.1 sec  1.08 GBytes   918 Mbits/sec
[ 39]  0.0-10.1 sec  95.8 MBytes  79.6 Mbits/sec
[SUM]  0.0-10.1 sec  23.2 GBytes  19.7 Gbits/sec


I played with it a bit and can confirm if I have all interrupt affinity
set to CPU0, I then do not see this issue. This tells us that there
still seem to be a race somewhere, when multiple CPUs are involved?

Regards,

Ray

[PATCH 0/8] io-pgtable lock removal

[Will Deacon]

Hi Ray,

Thanks for testing this, and sorry it didn't help.

On Wed, Jul 05, 2017 at 04:24:22PM -0700, Ray Jui wrote:
> On 7/5/17 1:41 AM, Will Deacon wrote:
> > On Tue, Jul 04, 2017 at 06:45:17PM -0700, Ray Jui wrote:
> >> Has anything functionally changed between PATCH v2 and v1? I'm seeing a
> >> very different L2 throughput with v2 (in general a lot worse with v2 vs.
> >> v1); however, I'm currently unable to reproduce the TLB sync timed out
> >> issue with v2 (without the patch from Will's email).
> >>
> >> It could also be something else that has changed in my setup, but so far
> >> I have not yet been able to spot anything wrong in the setup.
> > 
> > There were fixes, and that initially involved a DSB that was found to be
> > expensive. The patches queued in -next should have that addressed, so please
> > use those (or my for-joerg/arm-smmu/updates branch).
> > 
> 
> That was my bad yesterday. I was in a rush and the setup was incorrect.
> 
> I redo my Ethernet performance test with both PATCH v1 and v2 today, and
> can confirm the performance is consistent between v1 and v2 as expected.
> 
> I also made sure the following message can still be reproduced with
> patch set v2:
> 
> arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked
> 
> Then I proceeded to apply your patch that attempt to fix the deadlock
> issue.

[...]

> However, with the fix patch, I can still see the deadlock message when I
> have > 32 iperf TX threads active in the system:

Damn. We've been going over this today and the only plausible theory seems
to be that concurrent TLB syncs are causing the completion to be pushed out,
resulting in timeouts.

Can you try this patch below, instead of the one I sent before, please?

Thanks,

Will

--->8

>From bbf3737c29e3d18f539998a66f42878ac91cde97 Mon Sep 17 00:00:00 2001
From: Will Deacon <will.deacon@arm.com>
Date: Thu, 6 Jul 2017 15:55:48 +0100
Subject: [PATCH] iommu/arm-smmu: Reintroduce locking around TLB sync
 operations

Commit 523d7423e21b ("iommu/arm-smmu: Remove io-pgtable spinlock")
removed the locking used to serialise map/unmap calls into the io-pgtable
code from the ARM SMMU driver. This is good for performance, but opens
us up to a nasty race with TLB syncs because the TLB sync register is
shared within a context bank (or even globally for stage-2 on SMMUv1).

There are two cases to consider:

  1. A CPU can be spinning on the completion of a TLB sync, take an
     interrupt which issues a subsequent TLB sync, and then report a
     timeout on return from the interrupt.

  2. A CPU can be spinning on the completion of a TLB sync, but other
     CPUs can continuously issue additional TLB syncs in such a way that
     the backoff logic reports a timeout.

Rather than fix this by spinning for completion of prior TLB syncs before
issuing a new one (which may suffer from fairness issues on large systems),
instead reintroduce locking around TLB sync operations in the ARM SMMU
driver.

Fixes: 523d7423e21b ("iommu/arm-smmu: Remove io-pgtable spinlock")
Cc: Robin Murphy <robin.murphy@arm.com>
Reported-by: Ray Jui <ray.jui@broadcom.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 drivers/iommu/arm-smmu.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index b446183b3015..770abd247f40 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -400,6 +400,8 @@ struct arm_smmu_device {
 
 	u32				cavium_id_base; /* Specific to Cavium */
 
+	spinlock_t			global_sync_lock;
+
 	/* IOMMU core code handle */
 	struct iommu_device		iommu;
 };
@@ -436,7 +438,7 @@ struct arm_smmu_domain {
 	struct arm_smmu_cfg		cfg;
 	enum arm_smmu_domain_stage	stage;
 	struct mutex			init_mutex; /* Protects smmu pointer */
-	spinlock_t			cb_lock; /* Serialises ATS1* ops */
+	spinlock_t			cb_lock; /* Serialises ATS1* ops and TLB syncs */
 	struct iommu_domain		domain;
 };
 
@@ -602,9 +604,12 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu,
 static void arm_smmu_tlb_sync_global(struct arm_smmu_device *smmu)
 {
 	void __iomem *base = ARM_SMMU_GR0(smmu);
+	unsigned long flags;
 
+	spin_lock_irqsave(&smmu->global_sync_lock, flags);
 	__arm_smmu_tlb_sync(smmu, base + ARM_SMMU_GR0_sTLBGSYNC,
 			    base + ARM_SMMU_GR0_sTLBGSTATUS);
+	spin_unlock_irqrestore(&smmu->global_sync_lock, flags);
 }
 
 static void arm_smmu_tlb_sync_context(void *cookie)
@@ -612,9 +617,12 @@ static void arm_smmu_tlb_sync_context(void *cookie)
 	struct arm_smmu_domain *smmu_domain = cookie;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	void __iomem *base = ARM_SMMU_CB(smmu, smmu_domain->cfg.cbndx);
+	unsigned long flags;
 
+	spin_lock_irqsave(&smmu_domain->cb_lock, flags);
 	__arm_smmu_tlb_sync(smmu, base + ARM_SMMU_CB_TLBSYNC,
 			    base + ARM_SMMU_CB_TLBSTATUS);
+	spin_unlock_irqrestore(&smmu_domain->cb_lock, flags);
 }
 
 static void arm_smmu_tlb_sync_vmid(void *cookie)
@@ -1925,6 +1933,7 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 
 	smmu->num_mapping_groups = size;
 	mutex_init(&smmu->stream_map_mutex);
+	spin_lock_init(&smmu->global_sync_lock);
 
 	if (smmu->version < ARM_SMMU_V2 || !(id & ID0_PTFS_NO_AARCH32)) {
 		smmu->features |= ARM_SMMU_FEAT_FMT_AARCH32_L;
-- 
2.1.4

[PATCH 0/8] io-pgtable lock removal

[Ray Jui]

Hi Will,

On 7/6/17 8:08 AM, Will Deacon wrote:
> Hi Ray,
> 
> Thanks for testing this, and sorry it didn't help.
> 
> On Wed, Jul 05, 2017 at 04:24:22PM -0700, Ray Jui wrote:
>> On 7/5/17 1:41 AM, Will Deacon wrote:
>>> On Tue, Jul 04, 2017 at 06:45:17PM -0700, Ray Jui wrote:
>>>> Has anything functionally changed between PATCH v2 and v1? I'm seeing a
>>>> very different L2 throughput with v2 (in general a lot worse with v2 vs.
>>>> v1); however, I'm currently unable to reproduce the TLB sync timed out
>>>> issue with v2 (without the patch from Will's email).
>>>>
>>>> It could also be something else that has changed in my setup, but so far
>>>> I have not yet been able to spot anything wrong in the setup.
>>>
>>> There were fixes, and that initially involved a DSB that was found to be
>>> expensive. The patches queued in -next should have that addressed, so please
>>> use those (or my for-joerg/arm-smmu/updates branch).
>>>
>>
>> That was my bad yesterday. I was in a rush and the setup was incorrect.
>>
>> I redo my Ethernet performance test with both PATCH v1 and v2 today, and
>> can confirm the performance is consistent between v1 and v2 as expected.
>>
>> I also made sure the following message can still be reproduced with
>> patch set v2:
>>
>> arm-smmu 64000000.mmu: TLB sync timed out -- SMMU may be deadlocked
>>
>> Then I proceeded to apply your patch that attempt to fix the deadlock
>> issue.
> 
> [...]
> 
>> However, with the fix patch, I can still see the deadlock message when I
>> have > 32 iperf TX threads active in the system:
> 
> Damn. We've been going over this today and the only plausible theory seems
> to be that concurrent TLB syncs are causing the completion to be pushed out,
> resulting in timeouts.
> 
> Can you try this patch below, instead of the one I sent before, please?
> 
> Thanks,
> 
> Will
> 
> --->8

Good news! I can confirm that with the new patch below, the error log of
"TLB sync timed out" is now gone. I ran 20 iterations of the iperf
client test with 64 threads spread across 8 CPUs. The error message used
to coming out very quickly with just one iteration. But now it's
completely gone.

At the same time, I do seem to observe a slight impact on performance
when multiple threads are used, but I guess that is expected, given that
a spin lock is added to protect the TLB sync.

Great work and thanks for the fix!

Ray

> 
> From bbf3737c29e3d18f539998a66f42878ac91cde97 Mon Sep 17 00:00:00 2001
> From: Will Deacon <will.deacon@arm.com>
> Date: Thu, 6 Jul 2017 15:55:48 +0100
> Subject: [PATCH] iommu/arm-smmu: Reintroduce locking around TLB sync
>  operations
> 
> Commit 523d7423e21b ("iommu/arm-smmu: Remove io-pgtable spinlock")
> removed the locking used to serialise map/unmap calls into the io-pgtable
> code from the ARM SMMU driver. This is good for performance, but opens
> us up to a nasty race with TLB syncs because the TLB sync register is
> shared within a context bank (or even globally for stage-2 on SMMUv1).
> 
> There are two cases to consider:
> 
>   1. A CPU can be spinning on the completion of a TLB sync, take an
>      interrupt which issues a subsequent TLB sync, and then report a
>      timeout on return from the interrupt.
> 
>   2. A CPU can be spinning on the completion of a TLB sync, but other
>      CPUs can continuously issue additional TLB syncs in such a way that
>      the backoff logic reports a timeout.
> 
> Rather than fix this by spinning for completion of prior TLB syncs before
> issuing a new one (which may suffer from fairness issues on large systems),
> instead reintroduce locking around TLB sync operations in the ARM SMMU
> driver.
> 
> Fixes: 523d7423e21b ("iommu/arm-smmu: Remove io-pgtable spinlock")
> Cc: Robin Murphy <robin.murphy@arm.com>
> Reported-by: Ray Jui <ray.jui@broadcom.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  drivers/iommu/arm-smmu.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index b446183b3015..770abd247f40 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -400,6 +400,8 @@ struct arm_smmu_device {
>  
>  	u32				cavium_id_base; /* Specific to Cavium */
>  
> +	spinlock_t			global_sync_lock;
> +
>  	/* IOMMU core code handle */
>  	struct iommu_device		iommu;
>  };
> @@ -436,7 +438,7 @@ struct arm_smmu_domain {
>  	struct arm_smmu_cfg		cfg;
>  	enum arm_smmu_domain_stage	stage;
>  	struct mutex			init_mutex; /* Protects smmu pointer */
> -	spinlock_t			cb_lock; /* Serialises ATS1* ops */
> +	spinlock_t			cb_lock; /* Serialises ATS1* ops and TLB syncs */
>  	struct iommu_domain		domain;
>  };
>  
> @@ -602,9 +604,12 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu,
>  static void arm_smmu_tlb_sync_global(struct arm_smmu_device *smmu)
>  {
>  	void __iomem *base = ARM_SMMU_GR0(smmu);
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&smmu->global_sync_lock, flags);
>  	__arm_smmu_tlb_sync(smmu, base + ARM_SMMU_GR0_sTLBGSYNC,
>  			    base + ARM_SMMU_GR0_sTLBGSTATUS);
> +	spin_unlock_irqrestore(&smmu->global_sync_lock, flags);
>  }
>  
>  static void arm_smmu_tlb_sync_context(void *cookie)
> @@ -612,9 +617,12 @@ static void arm_smmu_tlb_sync_context(void *cookie)
>  	struct arm_smmu_domain *smmu_domain = cookie;
>  	struct arm_smmu_device *smmu = smmu_domain->smmu;
>  	void __iomem *base = ARM_SMMU_CB(smmu, smmu_domain->cfg.cbndx);
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&smmu_domain->cb_lock, flags);
>  	__arm_smmu_tlb_sync(smmu, base + ARM_SMMU_CB_TLBSYNC,
>  			    base + ARM_SMMU_CB_TLBSTATUS);
> +	spin_unlock_irqrestore(&smmu_domain->cb_lock, flags);
>  }
>  
>  static void arm_smmu_tlb_sync_vmid(void *cookie)
> @@ -1925,6 +1933,7 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  
>  	smmu->num_mapping_groups = size;
>  	mutex_init(&smmu->stream_map_mutex);
> +	spin_lock_init(&smmu->global_sync_lock);
>  
>  	if (smmu->version < ARM_SMMU_V2 || !(id & ID0_PTFS_NO_AARCH32)) {
>  		smmu->features |= ARM_SMMU_FEAT_FMT_AARCH32_L;
> 

[PATCH 0/8] io-pgtable lock removal

[Will Deacon]

On Thu, Jul 06, 2017 at 11:14:00AM -0700, Ray Jui wrote:
> On 7/6/17 8:08 AM, Will Deacon wrote:
> > On Wed, Jul 05, 2017 at 04:24:22PM -0700, Ray Jui wrote:
> >> However, with the fix patch, I can still see the deadlock message when I
> >> have > 32 iperf TX threads active in the system:
> > 
> > Damn. We've been going over this today and the only plausible theory seems
> > to be that concurrent TLB syncs are causing the completion to be pushed out,
> > resulting in timeouts.
> > 
> > Can you try this patch below, instead of the one I sent before, please?
> > 
> > Thanks,
> > 
> > Will
> > 
> > --->8
> 
> Good news! I can confirm that with the new patch below, the error log of
> "TLB sync timed out" is now gone. I ran 20 iterations of the iperf
> client test with 64 threads spread across 8 CPUs. The error message used
> to coming out very quickly with just one iteration. But now it's
> completely gone.

Good, I'll queue this is as a fix then. Thanks for testing!

> At the same time, I do seem to observe a slight impact on performance
> when multiple threads are used, but I guess that is expected, given that
> a spin lock is added to protect the TLB sync.

Hopefully that performance will pick up again once we defer the sync
to the end of iommu_unmap.

Will

[PATCH 0/8] io-pgtable lock removal

[Joerg Roedel]

On Wed, Jun 14, 2017 at 05:40:30PM -0700, Ray Jui wrote:
> With the NVMf target test with 4 SSDs, fio based test, random read, 4k,
> 8 jobs:
> 
> Without IOMMU:
> 
> IOPS = 1080K
> 
> With IOMMU, but without your latest patch:
> 
> IOPS = 520K
> 
> With IOMMU and your latest patch:
> 
> IOPS = 500K ~ 850K (a lot of variation observed during the same test run)

That variation might come from the RB-Tree used in the IOVA allocator.
For block-device workloads the allocation size of iova ranges might be
bigger than what is cached in the magazines, so that the fall-back to
the old (locked) allocator is used.


	Joerg