io-pgtable lock removal
Subsystems (2)
Robin Murphy
reviewerv2 cover io-pgtable lock removal
The feedback has been promising, so v2 is just a final update to cover a handful of memory ordering and cosmetic tweaks that came up when Will and I went through this offline.
Thanks,
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 | 179 +++++++++++++++++++++------------- drivers/iommu/io-pgtable-arm.c | 191 ++++++++++++++++++++++++------------- drivers/iommu/io-pgtable.h | 6 ++ 5 files changed, 274 insertions(+), 186 deletions(-)
Signature · 1 line
2.12.2.dirty
Robin Murphy
reviewerPATCH v2 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.
---
v2: No change
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 v2 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.
---
v2: Make the address increment in the loop nicer, which also allows the
factoring of the init_pte helpers to be a little cleaner too
drivers/iommu/io-pgtable-arm.c | 118 ++++++++++++++++++++++++----------------- 1 file changed, 69 insertions(+), 49 deletions(-)
drivers/iommu/io-pgtable-arm.c +69 -49
@@ -264,19 +264,38 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data, unsigned long iova, size_t size, int lvl, arm_lpae_iopte *ptep); +static void __arm_lpae_init_pte (struct arm_lpae_io_pgtable *data, +phys_addr_t paddr,arm_lpae_iopte prot, +int lvl,arm_lpae_iopte *ptep) +{ +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); + +__arm_lpae_set_pte (ptep, pte, &data->iop.cfg); +} +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 = *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.
@@ -289,21 +308,24 @@ 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); +__arm_lpae_init_pte (data, paddr, prot, lvl, ptep);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 +353,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 +471,43 @@ 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; +struct io_pgtable_cfg *cfg = &data->iop.cfg; +arm_lpae_iopte pte, *tablep;phys_addr_t blk_paddr; -arm_lpae_iopte table =0 ; +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); + + blk_paddr =iopte_to_pfn (blk_pte, data) << data->pg_shift; + pte =iopte_prot (blk_pte); + +for (i =0 ; i < tablesz /sizeof (pte); i++, blk_paddr += split_sz) {/* 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_init_pte (data, blk_paddr, pte, lvl, &tablep[i]); } -__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 v2 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.
---
v2: One trivial cosmetic tweak to the loop increment
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; i += num_entries, pte += size) {/* 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 v2 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.
---
v2: No change
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 v2 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.
---
v2:
- Fix barriers in install_table
- Make a few more PTE accesses with {READ,WRITE}_ONCE() just in case
- Minor cosmetics
drivers/iommu/io-pgtable-arm.c | 72 +++++++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 15 deletions(-)
drivers/iommu/io-pgtable-arm.c +57 -15
@@ -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,
@@ -314,16 +322,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; +/* Ensure the table itself is visible before its PTE can be */ +wmb (); + + old =cmpxchg64_relaxed (ptep, curr, new); + +if ((cfg->quirks &IO_PGTABLE_QUIRK_NO_DMA ) || + (old &ARM_LPAE_PTE_SW_SYNC )) +return old; + +/* Even if it's not ours, there's no point waiting; just kick it */ +__arm_lpae_sync_pte (ptep, cfg); +if (old == curr) +WRITE_ONCE (*ptep, new |ARM_LPAE_PTE_SW_SYNC ); + +return old; } static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
@@ -332,6 +354,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 */
@@ -346,17 +369,23 @@ 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)) { + 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 (pte && !iopte_leaf (pte, lvl)) { cptep =iopte_deref (pte, data); - }else { + }else if (pte) {/* We require an unmap first */ WARN_ON (!selftest_running);return -EEXIST ;
@@ -502,7 +531,19 @@ static int arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,__arm_lpae_init_pte (data, blk_paddr, pte, lvl, &tablep[i]); } -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 -1 ) !=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);
@@ -523,7 +564,7 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,return 0 ; ptep +=ARM_LPAE_LVL_IDX (iova, lvl, data); - pte = *ptep; + pte =READ_ONCE (*ptep);if (WARN_ON (!pte))return 0 ;
@@ -585,7 +626,8 @@ static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,return 0 ;/* Grab the IOPTE we're interested in */ - pte = *(ptep +ARM_LPAE_LVL_IDX (iova, lvl, data)); + ptep +=ARM_LPAE_LVL_IDX (iova, lvl, data); + pte =READ_ONCE (*ptep);/* Valid entry? */ if (!pte)
Signature · 1 line
2.12.2.dirty
PATCH v2 5/8 iommu/io-pgtable-arm: Support lockless operation
Robin,
Was trying to understand the new changes. Had few questions on arm_lpae_install_table.
Quoted 83 lines · Robin Murphy
For parallel I/O with multiple concurrent threads servicing the same device (or devices, if several share a domain), serialising page table updates becomes a massive bottleneck. On reflection, though, we don't strictly need to do that - for valid IOMMU API usage, there are in fact only two races that we need to guard against: multiple map requests for different blocks within the same region, when the intermediate-level table for that region does not yet exist; and multiple unmaps of different parts of the same block entry. Both of those are fairly easily solved by using a cmpxchg to install the new table, such that if we then find that someone else's table got there first, we can simply free ours and continue.
Make the requisite changes such that we can withstand being called without the caller maintaining a lock. In theory, this opens up a few corners in which wildly misbehaving callers making nonsensical overlapping requests might lead to crashes instead of just unpredictable results, but correct code really does not deserve to pay a significant performance cost for the sake of masking bugs in theoretical broken code.
Signed-off-by Robin Murphy <[email protected]>---
v2:
- Fix barriers ininstall_table
- Make a few more PTE accesses with {READ,WRITE}_ONCE() just in case
- Minor cosmeticsdrivers/iommu/io-pgtable-arm.c | 72 +++++++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 15 deletions(-)drivers/iommu/io-pgtable-arm.c +12
-4@@ -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,@@ -314,16 +322,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;
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 335
Quoted 10 lines
drivers/iommu/io-pgtable-arm.c +3
-2+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; +/* Ensure the table itself is visible before its PTE can be */ +wmb ();
Could you please give more hints on why this is required.
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 346
Quoted 11 lines
drivers/iommu/io-pgtable-arm.c +11
-0+ + old =cmpxchg64_relaxed (ptep, curr, new); + +if ((cfg->quirks &IO_PGTABLE_QUIRK_NO_DMA ) || + (old &ARM_LPAE_PTE_SW_SYNC )) +return old; + +/* Even if it's not ours, there's no point waiting; just kick it */ +__arm_lpae_sync_pte (ptep, cfg); +if (old == curr) +WRITE_ONCE (*ptep, new |ARM_LPAE_PTE_SW_SYNC );
How is it ensured that the cache operations are completed before we flag them with
ARM_LPAE_PTE_SW_SYNC. The previous version had a wmb() after the sync operation.
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 350
Quoted 4 lines
drivers/iommu/io-pgtable-arm.c +2
-0+ +return old; }
Thanks.
Signature · 1 line
Linu cherian
PATCH v2 5/8 iommu/io-pgtable-arm: Support lockless operation
Quoted 119 lines · Linu Cherian
Robin,
Was trying to understand the new changes. Had few questions onarm_lpae_install_table.Quoted 83 lines · Robin Murphy
For parallel I/O with multiple concurrent threads servicing the same device (or devices, if several share a domain), serialising page table updates becomes a massive bottleneck. On reflection, though, we don't strictly need to do that - for valid IOMMU API usage, there are in fact only two races that we need to guard against: multiple map requests for different blocks within the same region, when the intermediate-level table for that region does not yet exist; and multiple unmaps of different parts of the same block entry. Both of those are fairly easily solved by using a cmpxchg to install the new table, such that if we then find that someone else's table got there first, we can simply free ours and continue.
Make the requisite changes such that we can withstand being called without the caller maintaining a lock. In theory, this opens up a few corners in which wildly misbehaving callers making nonsensical overlapping requests might lead to crashes instead of just unpredictable results, but correct code really does not deserve to pay a significant performance cost for the sake of masking bugs in theoretical broken code.
Signed-off-by Robin Murphy <[email protected]>---
v2:
- Fix barriers ininstall_table
- Make a few more PTE accesses with {READ,WRITE}_ONCE() just in case
- Minor cosmeticsdrivers/iommu/io-pgtable-arm.c | 72 +++++++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 15 deletions(-)drivers/iommu/io-pgtable-arm.c +12
-4@@ -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,@@ -314,16 +322,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;drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 335
Quoted 10 lines
drivers/iommu/io-pgtable-arm.c +3
-2+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; +/* Ensure the table itself is visible before its PTE can be */ +wmb ();Could you please give more hints on why this is required.
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 346
Quoted 11 lines
drivers/iommu/io-pgtable-arm.c +11
-0+ + old =cmpxchg64_relaxed (ptep, curr, new); + +if ((cfg->quirks &IO_PGTABLE_QUIRK_NO_DMA ) || + (old &ARM_LPAE_PTE_SW_SYNC )) +return old; + +/* Even if it's not ours, there's no point waiting; just kick it */ +__arm_lpae_sync_pte (ptep, cfg); +if (old == curr) +WRITE_ONCE (*ptep, new |ARM_LPAE_PTE_SW_SYNC );How is it ensured that the cache operations are completed before we flag them with
ARM_LPAE_PTE_SW_SYNC. The previous version had awmb()after the sync operation.
Quoted 5 lines
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 350
Quoted 4 lines
drivers/iommu/io-pgtable-arm.c +2
-0+ +return old; }
Observed a performance drop of close to 1G,
while testing on the 10G network interface with this patch series compared to v1. Moving the wmb() as in v1 restores it back.
@@ -332,7 +332,6 @@ static arm_lpae_iopte arm_lpae_install_table(arm_lpae_iopte *table,
new |= ARM_LPAE_PTE_NSTABLE;
/* Ensure the table itself is visible before its PTE can be */- wmb();
old = cmpxchg64_relaxed(ptep, curr, new);
@@ -342,6 +341,7 @@ static arm_lpae_iopte arm_lpae_install_table(arm_lpae_iopte *table,
/* Even if it's not ours, there's no point waiting; just kick it */
__arm_lpae_sync_pte(ptep, cfg);
+ wmb();
if (old == curr)
WRITE_ONCE(*ptep, new | ARM_LPAE_PTE_SW_SYNC);Signature · 1 line
Linu cherian
Robin Murphy
reviewerPATCH v2 5/8 iommu/io-pgtable-arm: Support lockless operation
Quoted 96 lines · Linu Cherian
Quoted 95 lines · Linu Cherian
Robin,
Was trying to understand the new changes. Had few questions onarm_lpae_install_table.Quoted 89 lines · Robin Murphy
For parallel I/O with multiple concurrent threads servicing the same device (or devices, if several share a domain), serialising page table updates becomes a massive bottleneck. On reflection, though, we don't strictly need to do that - for valid IOMMU API usage, there are in fact only two races that we need to guard against: multiple map requests for different blocks within the same region, when the intermediate-level table for that region does not yet exist; and multiple unmaps of different parts of the same block entry. Both of those are fairly easily solved by using a cmpxchg to install the new table, such that if we then find that someone else's table got there first, we can simply free ours and continue.
Make the requisite changes such that we can withstand being called without the caller maintaining a lock. In theory, this opens up a few corners in which wildly misbehaving callers making nonsensical overlapping requests might lead to crashes instead of just unpredictable results, but correct code really does not deserve to pay a significant performance cost for the sake of masking bugs in theoretical broken code.
Signed-off-by Robin Murphy <[email protected]>---
v2:
- Fix barriers ininstall_table
- Make a few more PTE accesses with {READ,WRITE}_ONCE() just in case
- Minor cosmeticsdrivers/iommu/io-pgtable-arm.c | 72 +++++++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 15 deletions(-)drivers/iommu/io-pgtable-arm.c +13
-4@@ -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,@@ -314,16 +322,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 ;
Quoted 6 lines
Quoted 6 lines
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 335
Quoted 4 lines
drivers/iommu/io-pgtable-arm.c +2
-2-__arm_lpae_set_pte (ptep, new, cfg); -return new; +/* Ensure the table itself is visible before its PTE can be */ +wmb ();Could you please give more hints on why this is required.
In theory it's possible for the write to ptep to become visible before the previous writes filling out the PTEs in table - if the IOMMU happened to speculatively walk ptep while parts of table were still sat in a write buffer somewhere, it could see the old contents of that page and potentially allocate bogus TLB entries if the stale data happened to look like valid PTEs. In the non-coherent case the DMA cache maintenance is sufficient, but otherwise we need a barrier to order writing the next-level PTEs strictly before writing the table PTE pointing to them, such that the IOMMU cannot at any point see a partially-initialised table.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/iommu/io-pgtable-arm.c +6
-0+ + old =cmpxchg64_relaxed (ptep, curr, new); + +if ((cfg->quirks &IO_PGTABLE_QUIRK_NO_DMA ) || + (old &ARM_LPAE_PTE_SW_SYNC )) +return old;
Quoted 8 lines
Quoted 8 lines
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 346
Quoted 5 lines
drivers/iommu/io-pgtable-arm.c +5
-0+ +/* Even if it's not ours, there's no point waiting; just kick it */ +__arm_lpae_sync_pte (ptep, cfg); +if (old == curr) +WRITE_ONCE (*ptep, new |ARM_LPAE_PTE_SW_SYNC );How is it ensured that the cache operations are completed before we flag them with
ARM_LPAE_PTE_SW_SYNC. The previous version had awmb()after the sync operation.
The wmb() here was a silly oversight on my part - as Will reminded me, dma_sync_*() already has its own barrier to ensure completion, which is pretty obvious in retrospect because the entire streaming DMA would be totally broken otherwise.
Quoted 9 lines
Quoted 5 lines
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 350
Quoted 4 lines
drivers/iommu/io-pgtable-arm.c +2
-0+ +return old; }Observed a performance drop of close to 1G,
while testing on the 10G network interface with this patch series compared to v1.
Is that consistent over multiple runs? I wouldn't expect many workloads to be thrashing table and hugepage mappings in the same IOVA region, so after a point we should tend to reach a fairly steady state where we're only changing leaf PTEs.
Quoted 1 line
Moving the
wmb()as in v1 restores it back.
Note that on a coherent platform like ThunderX that's as good as just deleting it, because you'll never execute the case below. However, on reflection I think it can at least safely be downgraded to dma_wmb() (i.e. DMB) rather than a full DSB - would you be able to test what difference that makes?
Robin.
Quoted 18 lines
drivers/iommu/io-pgtable-arm.c +1
-1@@ -332,7 +332,6 @@ static arm_lpae_iopte arm_lpae_install_table(arm_lpae_iopte *table, new |=ARM_LPAE_PTE_NSTABLE ;/* Ensure the table itself is visible before its PTE can be */ -wmb ()> old =cmpxchg64_relaxed (ptep, curr, new);@@ -342,6 +341,7 @@ static arm_lpae_iopte arm_lpae_install_table(arm_lpae_iopte *table,/* Even if it's not ours, there's no point waiting; just kick it */ __arm_lpae_sync_pte (ptep, cfg); +wmb ();if (old == curr)WRITE_ONCE (*ptep, new |ARM_LPAE_PTE_SW_SYNC );
PATCH v2 5/8 iommu/io-pgtable-arm: Support lockless operation
Quoted 104 lines · Robin Murphy
Quoted 96 lines · Linu Cherian
Quoted 95 lines · Linu Cherian
Robin,
Was trying to understand the new changes. Had few questions onarm_lpae_install_table.Quoted 89 lines · Robin Murphy
For parallel I/O with multiple concurrent threads servicing the same device (or devices, if several share a domain), serialising page table updates becomes a massive bottleneck. On reflection, though, we don't strictly need to do that - for valid IOMMU API usage, there are in fact only two races that we need to guard against: multiple map requests for different blocks within the same region, when the intermediate-level table for that region does not yet exist; and multiple unmaps of different parts of the same block entry. Both of those are fairly easily solved by using a cmpxchg to install the new table, such that if we then find that someone else's table got there first, we can simply free ours and continue.
Make the requisite changes such that we can withstand being called without the caller maintaining a lock. In theory, this opens up a few corners in which wildly misbehaving callers making nonsensical overlapping requests might lead to crashes instead of just unpredictable results, but correct code really does not deserve to pay a significant performance cost for the sake of masking bugs in theoretical broken code.
Signed-off-by Robin Murphy <[email protected]>---
v2:
- Fix barriers ininstall_table
- Make a few more PTE accesses with {READ,WRITE}_ONCE() just in case
- Minor cosmeticsdrivers/iommu/io-pgtable-arm.c | 72 +++++++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 15 deletions(-)drivers/iommu/io-pgtable-arm.c +13
-4@@ -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,@@ -314,16 +322,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 ;Quoted 6 lines
Quoted 6 lines
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 335
Quoted 4 lines
drivers/iommu/io-pgtable-arm.c +2
-2-__arm_lpae_set_pte (ptep, new, cfg); -return new; +/* Ensure the table itself is visible before its PTE can be */ +wmb ();Could you please give more hints on why this is required.
Quoted 9 lines
In theory it's possible for the write to ptep to become visible before the previous writes filling out the PTEs in table - if the IOMMU happened to speculatively walk ptep while parts of table were still sat in a write buffer somewhere, it could see the old contents of that page and potentially allocate bogus TLB entries if the stale data happened to look like valid PTEs. In the non-coherent case the DMA cache maintenance is sufficient, but otherwise we need a barrier to order writing the next-level PTEs strictly before writing the table PTE pointing to them, such that the IOMMU cannot at any point see a partially-initialised table.
Got it. Thanks a lot for explainig that.
Quoted 36 lines
Quoted 15 lines
Quoted 15 lines
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 346
Quoted 12 lines
drivers/iommu/io-pgtable-arm.c +11
-0+ + old =cmpxchg64_relaxed (ptep, curr, new); + +if ((cfg->quirks &IO_PGTABLE_QUIRK_NO_DMA ) || + (old &ARM_LPAE_PTE_SW_SYNC )) +return old; + +/* Even if it's not ours, there's no point waiting; just kick it */ +__arm_lpae_sync_pte (ptep, cfg); +if (old == curr) +WRITE_ONCE (*ptep, new |ARM_LPAE_PTE_SW_SYNC );How is it ensured that the cache operations are completed before we flag them with
ARM_LPAE_PTE_SW_SYNC. The previous version had awmb()after the sync operation.The
wmb()here was a silly oversight on my part - as Will reminded me, dma_sync_*() already has its own barrier to ensure completion, which is pretty obvious in retrospect because the entire streaming DMA would be totally broken otherwise.Quoted 9 lines
Quoted 5 lines
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 350
Quoted 4 lines
drivers/iommu/io-pgtable-arm.c +2
-0+ +return old; }Observed a performance drop of close to 1G,
while testing on the 10G network interface with this patch series compared to v1.Is that consistent over multiple runs? I wouldn't expect many workloads to be thrashing table and hugepage mappings in the same IOVA region, so after a point we should tend to reach a fairly steady state where we're only changing leaf PTEs.
Quoted 7 lines
Quoted 1 line
Moving the
wmb()as in v1 restores it back.Note that on a coherent platform like ThunderX that's as good as just deleting it, because you'll never execute the case below. However, on reflection I think it can at least safely be downgraded to
dma_wmb()(i.e. DMB) rather than a full DSB - would you be able to test what difference that makes?
The testing was done on Thunderx 1, which has a non coherent page table walk. Yes, downgrading to dma_wmb() helps. With this change, performance is back to v1.
Thanks.
Signature · 1 line
Linu cherian
PATCH v2 5/8 iommu/io-pgtable-arm: Support lockless operation
Quoted 155 lines · Linu Cherian
Quoted 104 lines · Robin Murphy
Quoted 96 lines · Linu Cherian
Quoted 95 lines · Linu Cherian
Robin,
Was trying to understand the new changes. Had few questions onarm_lpae_install_table.Quoted 89 lines · Robin Murphy
For parallel I/O with multiple concurrent threads servicing the same device (or devices, if several share a domain), serialising page table updates becomes a massive bottleneck. On reflection, though, we don't strictly need to do that - for valid IOMMU API usage, there are in fact only two races that we need to guard against: multiple map requests for different blocks within the same region, when the intermediate-level table for that region does not yet exist; and multiple unmaps of different parts of the same block entry. Both of those are fairly easily solved by using a cmpxchg to install the new table, such that if we then find that someone else's table got there first, we can simply free ours and continue.
Make the requisite changes such that we can withstand being called without the caller maintaining a lock. In theory, this opens up a few corners in which wildly misbehaving callers making nonsensical overlapping requests might lead to crashes instead of just unpredictable results, but correct code really does not deserve to pay a significant performance cost for the sake of masking bugs in theoretical broken code.
Signed-off-by Robin Murphy <[email protected]>---
v2:
- Fix barriers ininstall_table
- Make a few more PTE accesses with {READ,WRITE}_ONCE() just in case
- Minor cosmeticsdrivers/iommu/io-pgtable-arm.c | 72 +++++++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 15 deletions(-)drivers/iommu/io-pgtable-arm.c +13
-4@@ -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,@@ -314,16 +322,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 ;Quoted 6 lines
Quoted 6 lines
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 335
Quoted 4 lines
drivers/iommu/io-pgtable-arm.c +2
-2-__arm_lpae_set_pte (ptep, new, cfg); -return new; +/* Ensure the table itself is visible before its PTE can be */ +wmb ();Could you please give more hints on why this is required.
Quoted 9 lines
In theory it's possible for the write to ptep to become visible before the previous writes filling out the PTEs in table - if the IOMMU happened to speculatively walk ptep while parts of table were still sat in a write buffer somewhere, it could see the old contents of that page and potentially allocate bogus TLB entries if the stale data happened to look like valid PTEs. In the non-coherent case the DMA cache maintenance is sufficient, but otherwise we need a barrier to order writing the next-level PTEs strictly before writing the table PTE pointing to them, such that the IOMMU cannot at any point see a partially-initialised table.
Got it. Thanks a lot for explainig that.
Quoted 38 lines
Quoted 15 lines
Quoted 15 lines
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 346
Quoted 12 lines
drivers/iommu/io-pgtable-arm.c +11
-0+ + old =cmpxchg64_relaxed (ptep, curr, new); + +if ((cfg->quirks &IO_PGTABLE_QUIRK_NO_DMA ) || + (old &ARM_LPAE_PTE_SW_SYNC )) +return old; + +/* Even if it's not ours, there's no point waiting; just kick it */ +__arm_lpae_sync_pte (ptep, cfg); +if (old == curr) +WRITE_ONCE (*ptep, new |ARM_LPAE_PTE_SW_SYNC );How is it ensured that the cache operations are completed before we flag them with
ARM_LPAE_PTE_SW_SYNC. The previous version had awmb()after the sync operation.The
wmb()here was a silly oversight on my part - as Will reminded me, dma_sync_*() already has its own barrier to ensure completion, which is pretty obvious in retrospect because the entire streaming DMA would be totally broken otherwise.Quoted 9 lines
Quoted 5 lines
drivers/iommu/io-pgtable-arm.c static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, · line 350
Quoted 4 lines
drivers/iommu/io-pgtable-arm.c +2
-0+ +return old; }Observed a performance drop of close to 1G,
while testing on the 10G network interface with this patch series compared to v1.Is that consistent over multiple runs? I wouldn't expect many workloads to be thrashing table and hugepage mappings in the same IOVA region, so after a point we should tend to reach a fairly steady state where we're only changing leaf PTEs.
Quoted 1 line
Moving the
wmb()as in v1 restores it back.
Quoted 11 lines
Quoted 5 lines
Note that on a coherent platform like ThunderX that's as good as just deleting it, because you'll never execute the case below. However, on reflection I think it can at least safely be downgraded to
dma_wmb()(i.e. DMB) rather than a full DSB - would you be able to test what difference that makes?The testing was done on Thunderx 1, which has a non coherent page table walk. Yes, downgrading to
dma_wmb()helps. With this change, performance is back to v1.
Should i send a patch for this ?
Thanks.
Signature · 1 line
Linu cherian
PATCH v2 5/8 iommu/io-pgtable-arm: Support lockless operation
Quoted 8 lines · Linu Cherian
Quoted 7 lines · Linu Cherian
Quoted 5 lines · Robin Murphy
Note that on a coherent platform like ThunderX that's as good as just deleting it, because you'll never execute the case below. However, on reflection I think it can at least safely be downgraded to
dma_wmb()(i.e. DMB) rather than a full DSB - would you be able to test what difference that makes?
Quoted 5 lines
Quoted 3 lines
The testing was done on Thunderx 1, which has a non coherent page table walk. Yes, downgrading to
dma_wmb()helps. With this change, performance is back to v1.Should i send a patch for this ?
I already did it:
Will
PATCH v2 5/8 iommu/io-pgtable-arm: Support lockless operation
Quoted 17 lines · Will Deacon
Quoted 8 lines · Linu Cherian
Quoted 7 lines · Linu Cherian
Quoted 5 lines · Robin Murphy
Note that on a coherent platform like ThunderX that's as good as just deleting it, because you'll never execute the case below. However, on reflection I think it can at least safely be downgraded to
dma_wmb()(i.e. DMB) rather than a full DSB - would you be able to test what difference that makes?Quoted 5 lines
Quoted 3 lines
The testing was done on Thunderx 1, which has a non coherent page table walk. Yes, downgrading to
dma_wmb()helps. With this change, performance is back to v1.Should i send a patch for this ?
I already did it:
Quoted 8 lines
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Thanks Will.
Signature · 1 line
Linu cherian
Robin Murphy
reviewerPATCH v2 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.
---
v2:
- Fix barriers in install_table
- Use READ_ONCE() in iova_to_phys just in case
drivers/iommu/io-pgtable-arm-v7s.c | 84 ++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 21 deletions(-)
drivers/iommu/io-pgtable-arm-v7s.c +63 -21
@@ -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,22 @@ 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; +/* Ensure the table itself is visible before its PTE can be */ +wmb (); + + 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 +438,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 +507,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 +562,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 +592,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) {
@@ -631,7 +671,8 @@ static phys_addr_t arm_v7s_iova_to_phys(struct io_pgtable_ops *ops,u32 mask;do { - pte = ptep[ARM_V7S_LVL_IDX (iova, ++lvl)]; + ptep +=ARM_V7S_LVL_IDX (iova, ++lvl); + pte =READ_ONCE (*ptep); ptep =iopte_deref (pte, lvl); }while (ARM_V7S_PTE_IS_TABLE (pte, lvl));
@@ -672,6 +713,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 v2 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.
---
v2: No change
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 v2 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.
---
v2: No change
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
v2 cover io-pgtable lock removal
In reply to Robin MurphyQuoted 6 lines · Robin Murphy
The feedback has been promising, so v2 is just a final update to cover a handful of memory ordering and cosmetic tweaks that came up when Will and I went through this offline.
Thanks,
Robin.
Hi Robin,
Is it worth us retesting this patchset?
If yes, as for a branch (for convenience), does this one now match v2:git://linux-arm.org/linux-rm iommu/pgtable
I saw it was rewritten recently.
Thanks,
John
Quoted 19 lines
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 | 179 +++++++++++++++++++++------------- drivers/iommu/io-pgtable-arm.c | 191 ++++++++++++++++++++++++------------- drivers/iommu/io-pgtable.h | 6 ++ 5 files changed, 274 insertions(+), 186 deletions(-)
Robin Murphy
reviewerv2 cover io-pgtable lock removal
Quoted 8 lines · John Garry
Quoted 6 lines · Robin Murphy
The feedback has been promising, so v2 is just a final update to cover a handful of memory ordering and cosmetic tweaks that came up when Will and I went through this offline.
Thanks,
Robin.
Quoted 8 lines
Hi Robin,
Is it worth us retesting this patchset?
If yes, as for a branch (for convenience), does this one now match v2:
git://linux-arm.org/linux-rmiommu/pgtableI saw it was rewritten recently.
Indeed, I pushed the branch out before posting. Functionally it ought to be identical to v1, so this was more just a heads-up - feel free to double-check I've not broken anything, but I wouldn't say you need to throw the full test lab at it again.
Thanks,
Robin.
Quoted 27 lines
Thanks,
JohnQuoted 21 lines
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 | 179 +++++++++++++++++++++------------- drivers/iommu/io-pgtable-arm.c | 191 ++++++++++++++++++++++++------------- drivers/iommu/io-pgtable.h | 6 ++ 5 files changed, 274 insertions(+), 186 deletions(-)
v2 cover io-pgtable lock removal
Quoted 16 lines · Robin Murphy
Quoted 15 lines · John Garry
Quoted 6 lines · Robin Murphy
The feedback has been promising, so v2 is just a final update to cover a handful of memory ordering and cosmetic tweaks that came up when Will and I went through this offline.
Thanks,
Robin.Hi Robin,
Is it worth us retesting this patchset?
If yes, as for a branch (for convenience), does this one now match v2:
git://linux-arm.org/linux-rmiommu/pgtable
Quoted 7 lines
Quoted 1 line
I saw it was rewritten recently.
Indeed, I pushed the branch out before posting. Functionally it ought to be identical to v1, so this was more just a heads-up - feel free to double-check I've not broken anything, but I wouldn't say you need to throw the full test lab at it again.
I saw Will has already sent the pull request. But, FWIW, we are seeing roughly the same performance as v1 patchset. For PCI NIC, Zhou again found performance drop goes from ~15->8% with SMMU enabled, and for integrated storage controller [platform device], we still see a drop of about 50%, depending on datarates (Leizhen has been working on fixing this).
John
Quoted 34 lines
Thanks,
Robin.Quoted 27 lines
Thanks,
JohnQuoted 21 lines
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 | 179 +++++++++++++++++++++------------- drivers/iommu/io-pgtable-arm.c | 191 ++++++++++++++++++++++++------------- drivers/iommu/io-pgtable.h | 6 ++ 5 files changed, 274 insertions(+), 186 deletions(-).
Robin Murphy
reviewerv2 cover io-pgtable lock removal
Quoted 25 lines · John Garry
Quoted 16 lines · Robin Murphy
Quoted 15 lines · John Garry
Quoted 6 lines · Robin Murphy
The feedback has been promising, so v2 is just a final update to cover a handful of memory ordering and cosmetic tweaks that came up when Will and I went through this offline.
Thanks,
Robin.Hi Robin,
Is it worth us retesting this patchset?
If yes, as for a branch (for convenience), does this one now match v2:
git://linux-arm.org/linux-rmiommu/pgtableQuoted 7 lines
Quoted 1 line
I saw it was rewritten recently.
Indeed, I pushed the branch out before posting. Functionally it ought to be identical to v1, so this was more just a heads-up - feel free to double-check I've not broken anything, but I wouldn't say you need to throw the full test lab at it again.
Quoted 6 lines
I saw Will has already sent the pull request. But, FWIW, we are seeing roughly the same performance as v1 patchset. For PCI NIC, Zhou again found performance drop goes from ~15->8% with SMMU enabled, and for integrated storage controller [platform device], we still see a drop of about 50%, depending on datarates (Leizhen has been working on fixing this).
Thanks for confirming. Following Joerg's suggestion that the storage workloads may still depend on rbtree performance - it had slipped my mind that even with small block sizes those could well be grouped into scatterlists large enough to trigger a >64-page IOVA allocation - I've taken the liberty of cooking up a simplified version of Leizhen's rbtree optimisation series in the iommu/iova branch of my tree. I'll follow up on that after the merge window, but if anyone wants to play with it in the meantime feel free.
Robin.
v2 cover io-pgtable lock removal
Quoted 9 lines
Quoted 8 lines
I saw Will has already sent the pull request. But, FWIW, we are seeing roughly the same performance as v1 patchset. For PCI NIC, Zhou again found performance drop goes from ~15->8% with SMMU enabled, and for integrated storage controller [platform device], we still see a drop of about 50%, depending on datarates (Leizhen has been working on fixing this).
Quoted 8 lines
Thanks for confirming. Following Joerg's suggestion that the storage workloads may still depend on rbtree performance - it had slipped my mind that even with small block sizes those could well be grouped into scatterlists large enough to trigger a >64-page IOVA allocation - I've taken the liberty of cooking up a simplified version of Leizhen's rbtree optimisation series in the iommu/iova branch of my tree. I'll follow up on that after the merge window, but if anyone wants to play with it in the meantime feel free.
Just a reminder that we did also see poor performance with our integrated NIC on your v1 patchset also (I can push for v2 patchset testing, but expect the same).
We might be able to now include a LSI 3108 PCI SAS card in our testing also to give a broader set of results.
John
Quoted 5 lines
Robin.
.
v2 cover io-pgtable lock removal
Quoted 9 lines · John Garry
Quoted 9 lines
Quoted 8 lines
I saw Will has already sent the pull request. But, FWIW, we are seeing roughly the same performance as v1 patchset. For PCI NIC, Zhou again found performance drop goes from ~15->8% with SMMU enabled, and for integrated storage controller [platform device], we still see a drop of about 50%, depending on datarates (Leizhen has been working on fixing this).
Quoted 8 lines
Quoted 8 lines
Thanks for confirming. Following Joerg's suggestion that the storage workloads may still depend on rbtree performance - it had slipped my mind that even with small block sizes those could well be grouped into scatterlists large enough to trigger a >64-page IOVA allocation - I've taken the liberty of cooking up a simplified version of Leizhen's rbtree optimisation series in the iommu/iova branch of my tree. I'll follow up on that after the merge window, but if anyone wants to play with it in the meantime feel free.
The main problem is lock confliction of cmd queue. I have prepared my patchset, I will send it later.
Quoted 17 lines
Just a reminder that we did also see poor performance with our integrated NIC on your v1 patchset also (I can push for v2 patchset testing, but expect the same).
We might be able to now include a LSI 3108 PCI SAS card in our testing also to give a broader set of results.
John
Quoted 5 lines
Robin.
.
.
Signature · 2 lines
Thanks! BestRegards