io-pgtable lock removal
Subsystems (2)
Robin Murphy
reviewercover io-pgtable lock removal
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(-)
Signature · 1 line
2.12.2.dirty
Robin Murphy
reviewerPATCH 1/8 iommu/io-pgtable-arm-v7s: Check table PTEs more precisely
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.
--- drivers/iommu/io-pgtable-arm-v7s.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/iommu/io-pgtable-arm-v7s.c +2 -1
@@ -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)))
Signature · 1 line
2.12.2.dirty
Robin Murphy
reviewerPATCH 2/8 iommu/io-pgtable-arm: Improve split_blk_unmap
In reply to Robin MurphyThe 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.
--- drivers/iommu/io-pgtable-arm.c | 116 ++++++++++++++++++++++++----------------- 1 file changed, 68 insertions(+), 48 deletions(-)
drivers/iommu/io-pgtable-arm.c +68 -48
@@ -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' */
Signature · 1 line
2.12.2.dirty
Robin Murphy
reviewerPATCH 3/8 iommu/io-pgtable-arm-v7s: Refactor split_blk_unmap
In reply to Robin MurphyWhilst 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.
--- drivers/iommu/io-pgtable-arm-v7s.c | 85 ++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 40 deletions(-)
drivers/iommu/io-pgtable-arm-v7s.c +45 -40
@@ -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' */
Signature · 1 line
2.12.2.dirty
Robin Murphy
reviewerPATCH 4/8 iommu/io-pgtable: Introduce explicit coherency
In reply to Robin MurphyOnce 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.
--- 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(-)
drivers/iommu/arm-smmu-v3.c +3 -0
@@ -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 ;
drivers/iommu/arm-smmu.c +3 -0
@@ -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) {
drivers/iommu/io-pgtable-arm-v7s.c +10 -7
@@ -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;
drivers/iommu/io-pgtable-arm.c +6 -5
@@ -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) {
drivers/iommu/io-pgtable.h +6 -0
@@ -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;
Signature · 1 line
2.12.2.dirty
Robin Murphy
reviewerPATCH 5/8 iommu/io-pgtable-arm: Support lockless operation
In reply to Robin MurphyFor 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.
--- drivers/iommu/io-pgtable-arm.c | 75 ++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 17 deletions(-)
drivers/iommu/io-pgtable-arm.c +58 -17
@@ -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);
Signature · 1 line
2.12.2.dirty
Robin Murphy
reviewerPATCH 6/8 iommu/io-pgtable-arm-v7s: Support lockless operation
In reply to Robin MurphyMirroring 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.
--- drivers/iommu/io-pgtable-arm-v7s.c | 78 ++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 20 deletions(-)
drivers/iommu/io-pgtable-arm-v7s.c +58 -20
@@ -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),
Signature · 1 line
2.12.2.dirty
Robin Murphy
reviewerPATCH 7/8 iommu/arm-smmu: Remove io-pgtable spinlock
In reply to Robin MurphyWith 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.
--- drivers/iommu/arm-smmu.c | 45 ++++++++++++++------------------------------- 1 file changed, 14 insertions(+), 31 deletions(-)
drivers/iommu/arm-smmu.c +14 -31
@@ -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)
Signature · 1 line
2.12.2.dirty
Robin Murphy
reviewerPATCH 8/8 iommu/arm-smmu-v3: Remove io-pgtable spinlock
In reply to Robin MurphyAs 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.
--- drivers/iommu/arm-smmu-v3.c | 33 ++++++--------------------------- 1 file changed, 6 insertions(+), 27 deletions(-)
drivers/iommu/arm-smmu-v3.c +6 -27
@@ -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;
Signature · 1 line
2.12.2.dirty
cover io-pgtable lock removal
In reply to Robin MurphyHi Robin,
Quoted 7 lines · 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.
Quoted 6 lines
The branch I've previously shared has been updated too:
git://linux-arm.org/linux-rmiommu/pgtableAll 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.
Quoted 20 lines
Robin.
Robin Murphy (8):
iommu/io-pgtable-arm-v7s: Check table PTEs more precisely iommu/io-pgtable-arm: Improvesplit_blk_unmapiommu/io-pgtable-arm-v7s: Refactorsplit_blk_unmapiommu/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 spinlockdrivers/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(-)
Signature · 2 lines
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.
cover io-pgtable lock removal
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
Quoted 44 lines · Nate Watterson
Hi Robin,
Quoted 7 lines · 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.
Quoted 6 lines
The branch I've previously shared has been updated too:
git://linux-arm.org/linux-rmiommu/pgtableAll 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.Quoted 22 lines
Robin.
Robin Murphy (8):
iommu/io-pgtable-arm-v7s: Check table PTEs more precisely iommu/io-pgtable-arm: Improvesplit_blk_unmapiommu/io-pgtable-arm-v7s: Refactorsplit_blk_unmapiommu/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 spinlockdrivers/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(-)
cover io-pgtable lock removal
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
Quoted 137 lines
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 GbpsRX TCP x1 - 15 Gbps
RX TCP x4 - 33.7 Gbps
RX TCP x8 - 36 GbpsWith IOMMU, but without your latest patch:
TX TCP x1 - 15.2 Gbps
TX TCP x4 - 14.3 Gbps
TX TCP x8 - 13 GbpsRX TCP x1 - 7.88 Gbps
RX TCP x4 - 13.2 Gbps
RX TCP x8 - 12.6 GbpsWith IOMMU and your latest patch:
TX TCP x1 - 21.4 Gbps
TX TCP x4 - 30.5 Gbps
TX TCP x8 - 21.3 GbpsRX TCP x1 - 7.7 Gbps
RX TCP x4 - 20.1 Gbps
RX TCP x8 - 27.1 GbpsWith 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
Quoted 12 lines · Nate Watterson
Hi Robin,
Quoted 9 lines · 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:
Quoted 32 lines
Quoted 4 lines
git://linux-arm.org/linux-rmiommu/pgtableAll 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.Quoted 22 lines
Robin.
Robin Murphy (8):
iommu/io-pgtable-arm-v7s: Check table PTEs more precisely iommu/io-pgtable-arm: Improvesplit_blk_unmapiommu/io-pgtable-arm-v7s: Refactorsplit_blk_unmapiommu/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 spinlockdrivers/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.
Robin Murphy
reviewercover io-pgtable lock removal
In reply to Ray JuiQuoted 43 lines · 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 GbpsRX TCP x1 - 15 Gbps
RX TCP x4 - 33.7 Gbps
RX TCP x8 - 36 GbpsWith IOMMU, but without your latest patch:
TX TCP x1 - 15.2 Gbps
TX TCP x4 - 14.3 Gbps
TX TCP x8 - 13 GbpsRX TCP x1 - 7.88 Gbps
RX TCP x4 - 13.2 Gbps
RX TCP x8 - 12.6 GbpsWith IOMMU and your latest patch:
Quoted 7 lines
TX TCP x1 - 21.4 Gbps
TX TCP x4 - 30.5 Gbps
TX TCP x8 - 21.3 GbpsRX 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.
Quoted 7 lines
With the NVMf target test with 4 SSDs, fio based test, random read, 4k, 8 jobs:
Without IOMMU:
IOPS = 1080K
Quoted 7 lines
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?
Quoted 9 lines
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().
Quoted 4 lines
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.
Quoted 50 lines
Thanks,
Ray
Quoted 44 lines · Nate Watterson
Hi Robin,
Quoted 7 lines · 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.
Quoted 6 lines
The branch I've previously shared has been updated too:
git://linux-arm.org/linux-rmiommu/pgtableAll 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.Quoted 22 lines
Robin.
Robin Murphy (8):
iommu/io-pgtable-arm-v7s: Check table PTEs more precisely iommu/io-pgtable-arm: Improvesplit_blk_unmapiommu/io-pgtable-arm-v7s: Refactorsplit_blk_unmapiommu/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 spinlockdrivers/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(-)
cover io-pgtable lock removal
Hi Robin,
Quoted 48 lines · Robin Murphy
Quoted 47 lines · 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 GbpsRX TCP x1 - 15 Gbps
RX TCP x4 - 33.7 Gbps
RX TCP x8 - 36 GbpsWith IOMMU, but without your latest patch:
TX TCP x1 - 15.2 Gbps
TX TCP x4 - 14.3 Gbps
TX TCP x8 - 13 GbpsRX TCP x1 - 7.88 Gbps
RX TCP x4 - 13.2 Gbps
RX TCP x8 - 12.6 GbpsWith IOMMU and your latest patch:
TX TCP x1 - 21.4 Gbps
TX TCP x4 - 30.5 Gbps
TX TCP x8 - 21.3 Gbps
Quoted 9 lines
Quoted 3 lines
RX TCP x1 - 7.7 Gbps
RX TCP x4 - 20.1 Gbps
RX TCP x8 - 27.1 GbpsCool, 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!
Quoted 9 lines
Quoted 9 lines
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:
Quoted 9 lines
Quoted 5 lines
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).
Quoted 6 lines
Quoted 6 lines
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 13 lines
Quoted 4 lines
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 :/
Quoted 2 lines
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
cover io-pgtable lock removal
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.
Quoted 4 lines · Ray Jui
Quoted 4 lines
Quoted 4 lines
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
drivers/iommu/io-pgtable.c +1 -0
@@ -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; }
drivers/iommu/io-pgtable.h +6 -6
@@ -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; - } }/**
cover io-pgtable lock removal
Hi Will/Robin,
Quoted 19 lines · Will Deacon
Hi Ray,
Robin and I have been bashing our heads against the
tlb_sync_pendingflag this morning, and we reckon it could have something to do with your timeouts on MMU-500.Quoted 4 lines · Ray Jui
Quoted 4 lines
Quoted 4 lines
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_pendinglogic 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.
Quoted 6 lines
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
Quoted 62 lines
--->8
drivers/iommu/io-pgtable.c +1
-0@@ -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; }drivers/iommu/io-pgtable.h +6
-6@@ -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; - } }/**
cover io-pgtable lock removal
Ray,
Quoted 22 lines · Ray Jui
Quoted 21 lines · Will Deacon
Robin and I have been bashing our heads against the
tlb_sync_pendingflag this morning, and we reckon it could have something to do with your timeouts on MMU-500.Quoted 4 lines · Ray Jui
Quoted 4 lines
Quoted 4 lines
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_pendinglogic 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.
Quoted 6 lines
Quoted 2 lines
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
Quoted 1 line
From
eeb11dab63fcdd698b671a3a8c63516005caa9ecMon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
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.
--- drivers/iommu/io-pgtable.c | 1 + drivers/iommu/io-pgtable.h | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-)
drivers/iommu/io-pgtable.c +1 -0
@@ -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; }
drivers/iommu/io-pgtable.h +6 -6
@@ -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; - } }/**
Signature · 1 line
2.1.4
cover io-pgtable lock removal
Hi Will,
Quoted 32 lines · Will Deacon
Ray,
Quoted 22 lines · Ray Jui
Quoted 21 lines · Will Deacon
Robin and I have been bashing our heads against the
tlb_sync_pendingflag this morning, and we reckon it could have something to do with your timeouts on MMU-500.Quoted 4 lines · Ray Jui
Quoted 4 lines
Quoted 4 lines
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_pendinglogic 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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 5 lines
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
Quoted 102 lines
--->8
From
eeb11dab63fcdd698b671a3a8c63516005caa9ecMon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Thu, 29 Jun 2017 15:08:09 +0100
Subject: [PATCH] iommu/io-pgtable: Fixtlb_sync_pendingflag access from
concurrent CPUsThe
tlb_sync_pendingflag 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 itThe 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_pendingflag anatomic_tand 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 <[email protected]>Cc Robin Murphy <[email protected]>Fixes 2c3d273eabe8 ("iommu/io-pgtable-arm: Support lockless operation")Signed-off-by Will Deacon <[email protected]>--- drivers/iommu/io-pgtable.c | 1 + drivers/iommu/io-pgtable.h | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-)drivers/iommu/io-pgtable.c +1
-0@@ -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; }drivers/iommu/io-pgtable.h +6
-6@@ -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; - } }/**
cover io-pgtable lock removal
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
Quoted 150 lines · Ray Jui
Hi Will,
Quoted 32 lines · Will Deacon
Ray,
Quoted 22 lines · Ray Jui
Quoted 21 lines · Will Deacon
Robin and I have been bashing our heads against the
tlb_sync_pendingflag this morning, and we reckon it could have something to do with your timeouts on MMU-500.Quoted 4 lines · Ray Jui
Quoted 4 lines
Quoted 4 lines
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_pendinglogic 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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 5 lines
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
Quoted 102 lines
--->8
From
eeb11dab63fcdd698b671a3a8c63516005caa9ecMon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Thu, 29 Jun 2017 15:08:09 +0100
Subject: [PATCH] iommu/io-pgtable: Fixtlb_sync_pendingflag access from
concurrent CPUsThe
tlb_sync_pendingflag 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 itThe 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_pendingflag anatomic_tand 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 <[email protected]>Cc Robin Murphy <[email protected]>Fixes 2c3d273eabe8 ("iommu/io-pgtable-arm: Support lockless operation")Signed-off-by Will Deacon <[email protected]>--- drivers/iommu/io-pgtable.c | 1 + drivers/iommu/io-pgtable.h | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-)drivers/iommu/io-pgtable.c +1
-0@@ -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; }drivers/iommu/io-pgtable.h +6
-6@@ -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; - } }/**
cover io-pgtable lock removal
Quoted 9 lines · 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.
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
cover io-pgtable lock removal
Hi Will,
Quoted 8 lines · Will Deacon
Quoted 7 lines · 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).
Quoted 9 lines
Quoted 2 lines
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:
drivers/iommu/io-pgtable.c +0 -0
@@ -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
cover io-pgtable lock removal
Hi Ray,
Thanks for testing this, and sorry it didn't help.
Quoted 27 lines · Ray Jui
Quoted 6 lines · Will Deacon
Quoted 5 lines · Ray Jui
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).
Quoted 7 lines
Quoted 2 lines
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.
[...]
Quoted 2 lines
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
Quoted 1 line
From
bbf3737c29e3d18f539998a66f42878ac91cde97Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
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.
--- drivers/iommu/arm-smmu.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/iommu/arm-smmu.c +10 -1
@@ -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 ;
Signature · 1 line
2.1.4
cover io-pgtable lock removal
Hi Will,
Quoted 42 lines · Will Deacon
Hi Ray,
Thanks for testing this, and sorry it didn't help.
Quoted 27 lines · Ray Jui
Quoted 6 lines · Will Deacon
Quoted 5 lines · Ray Jui
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).
Quoted 7 lines
Quoted 2 lines
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.
[...]
Quoted 2 lines
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.
Quoted 7 lines
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
Quoted 93 lines
From
bbf3737c29e3d18f539998a66f42878ac91cde97Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Thu, 6 Jul 2017 15:55:48 +0100
Subject: [PATCH] iommu/arm-smmu: Reintroduce locking around TLB sync
operationsCommit
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 <[email protected]>Reported-by Ray Jui <[email protected]>Signed-off-by Will Deacon <[email protected]>--- drivers/iommu/arm-smmu.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/iommu/arm-smmu.c +10
-1@@ -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 ;
cover io-pgtable lock removal
Quoted 15 lines · Ray Jui
Quoted 14 lines · Will Deacon
Quoted 2 lines · Ray Jui
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
Quoted 7 lines
Quoted 1 line
--->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!
Quoted 3 lines
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
Joerg Roedel
maintainercover io-pgtable lock removal
In reply to Ray JuiQuoted 7 lines · Ray Jui
With the NVMf target test with 4 SSDs, fio based test, random read, 4k, 8 jobs:
Without IOMMU:
IOPS = 1080K
Quoted 7 lines
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