linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device
@ 2021-01-19 11:16 Lianbo Jiang
  2021-01-19 11:16 ` [PATCH 1/2 v2] dma-iommu: use static-key to minimize the impact in the fast-path Lianbo Jiang
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Lianbo Jiang @ 2021-01-19 11:16 UTC (permalink / raw)
  To: linux-kernel
  Cc: joro, will, iommu, jsnitsel, thomas.lendacky, robin.murphy, bhe

This patchset is to fix the failure of deferred attach for iommu attach
device, it includes the following two patches:

[1] [PATCH 1/2] dma-iommu: use static-key to minimize the impact in the fast-path
    This is a prepared patch for the second one, move out the is_kdump_kernel()
    check from iommu_dma_deferred_attach() to iommu_dma_init(), and use the
    static-key in the fast-path to minimize the impact in the normal case.

[2] [PATCH 2/2] iommu: use the __iommu_attach_device() directly for deferred attach
    Move the handling currently in iommu_dma_deferred_attach() into the
    iommu core code so that it can call the __iommu_attach_device()
    directly instead of the iommu_attach_device(). The external interface
    iommu_attach_device() is not suitable for handling this situation.

Changes since v1:
[1] use the __iommu_attach_device() directly for deferred attach
[2] use static-key to minimize the impact in the fast-path

Lianbo Jiang (2):
  dma-iommu: use static-key to minimize the impact in the fast-path
  iommu: use the __iommu_attach_device() directly for deferred attach

 drivers/iommu/dma-iommu.c | 29 +++++++++++------------------
 drivers/iommu/iommu.c     | 12 ++++++++++++
 include/linux/iommu.h     |  2 ++
 3 files changed, 25 insertions(+), 18 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2 v2] dma-iommu: use static-key to minimize the impact in the fast-path
  2021-01-19 11:16 [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device Lianbo Jiang
@ 2021-01-19 11:16 ` Lianbo Jiang
  2021-01-19 15:26   ` Christoph Hellwig
  2021-01-19 11:16 ` [PATCH 2/2 v2] iommu: use the __iommu_attach_device() directly for deferred attach Lianbo Jiang
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Lianbo Jiang @ 2021-01-19 11:16 UTC (permalink / raw)
  To: linux-kernel
  Cc: joro, will, iommu, jsnitsel, thomas.lendacky, robin.murphy, bhe

Let's move out the is_kdump_kernel() check from iommu_dma_deferred_attach()
to iommu_dma_init(), and use the static-key in the fast-path to minimize
the impact in the normal case.

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
Co-developed-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/dma-iommu.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index f0305e6aac1b..3711b4a6e4f9 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -51,6 +51,8 @@ struct iommu_dma_cookie {
 	struct iommu_domain		*fq_domain;
 };
 
+static DEFINE_STATIC_KEY_FALSE(__deferred_attach);
+
 void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
 		struct iommu_domain *domain)
 {
@@ -383,9 +385,6 @@ static int iommu_dma_deferred_attach(struct device *dev,
 {
 	const struct iommu_ops *ops = domain->ops;
 
-	if (!is_kdump_kernel())
-		return 0;
-
 	if (unlikely(ops->is_attach_deferred &&
 			ops->is_attach_deferred(domain, dev)))
 		return iommu_attach_device(domain, dev);
@@ -535,7 +534,8 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
 	size_t iova_off = iova_offset(iovad, phys);
 	dma_addr_t iova;
 
-	if (unlikely(iommu_dma_deferred_attach(dev, domain)))
+	if (static_branch_unlikely(&__deferred_attach) &&
+	    iommu_dma_deferred_attach(dev, domain))
 		return DMA_MAPPING_ERROR;
 
 	size = iova_align(iovad, size + iova_off);
@@ -693,7 +693,8 @@ static void *iommu_dma_alloc_remap(struct device *dev, size_t size,
 
 	*dma_handle = DMA_MAPPING_ERROR;
 
-	if (unlikely(iommu_dma_deferred_attach(dev, domain)))
+	if (static_branch_unlikely(&__deferred_attach) &&
+	    iommu_dma_deferred_attach(dev, domain))
 		return NULL;
 
 	min_size = alloc_sizes & -alloc_sizes;
@@ -1003,7 +1004,8 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	unsigned long mask = dma_get_seg_boundary(dev);
 	int i;
 
-	if (unlikely(iommu_dma_deferred_attach(dev, domain)))
+	if (static_branch_unlikely(&__deferred_attach) &&
+	    iommu_dma_deferred_attach(dev, domain))
 		return 0;
 
 	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
@@ -1451,6 +1453,9 @@ void iommu_dma_compose_msi_msg(struct msi_desc *desc,
 
 static int iommu_dma_init(void)
 {
+	if (is_kdump_kernel())
+		static_branch_enable(&__deferred_attach);
+
 	return iova_cache_get();
 }
 arch_initcall(iommu_dma_init);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2 v2] iommu: use the __iommu_attach_device() directly for deferred attach
  2021-01-19 11:16 [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device Lianbo Jiang
  2021-01-19 11:16 ` [PATCH 1/2 v2] dma-iommu: use static-key to minimize the impact in the fast-path Lianbo Jiang
@ 2021-01-19 11:16 ` Lianbo Jiang
  2021-01-19 13:40   ` Robin Murphy
  2021-01-19 15:29   ` Christoph Hellwig
  2021-01-19 13:46 ` [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device Lu Baolu
  2021-01-28 11:04 ` Joerg Roedel
  3 siblings, 2 replies; 11+ messages in thread
From: Lianbo Jiang @ 2021-01-19 11:16 UTC (permalink / raw)
  To: linux-kernel
  Cc: joro, will, iommu, jsnitsel, thomas.lendacky, robin.murphy, bhe

Currently, because domain attach allows to be deferred from iommu
driver to device driver, and when iommu initializes, the devices
on the bus will be scanned and the default groups will be allocated.

Due to the above changes, some devices could be added to the same
group as below:

[    3.859417] pci 0000:01:00.0: Adding to iommu group 16
[    3.864572] pci 0000:01:00.1: Adding to iommu group 16
[    3.869738] pci 0000:02:00.0: Adding to iommu group 17
[    3.874892] pci 0000:02:00.1: Adding to iommu group 17

But when attaching these devices, it doesn't allow that a group has
more than one device, otherwise it will return an error. This conflicts
with the deferred attaching. Unfortunately, it has two devices in the
same group for my side, for example:

[    9.627014] iommu_group_device_count(): device name[0]:0000:01:00.0
[    9.633545] iommu_group_device_count(): device name[1]:0000:01:00.1
...
[   10.255609] iommu_group_device_count(): device name[0]:0000:02:00.0
[   10.262144] iommu_group_device_count(): device name[1]:0000:02:00.1

Finally, which caused the failure of tg3 driver when tg3 driver calls
the dma_alloc_coherent() to allocate coherent memory in the tg3_test_dma().

[    9.660310] tg3 0000:01:00.0: DMA engine test failed, aborting
[    9.754085] tg3: probe of 0000:01:00.0 failed with error -12
[    9.997512] tg3 0000:01:00.1: DMA engine test failed, aborting
[   10.043053] tg3: probe of 0000:01:00.1 failed with error -12
[   10.288905] tg3 0000:02:00.0: DMA engine test failed, aborting
[   10.334070] tg3: probe of 0000:02:00.0 failed with error -12
[   10.578303] tg3 0000:02:00.1: DMA engine test failed, aborting
[   10.622629] tg3: probe of 0000:02:00.1 failed with error -12

In addition, the similar situations also occur in other drivers such
as the bnxt_en driver. That can be reproduced easily in kdump kernel
when SME is active

Let's move the handling currently in iommu_dma_deferred_attach() into
the iommu core code so that it can call the __iommu_attach_device()
directly instead of the iommu_attach_device(). The external interface
iommu_attach_device() is not suitable for handling this situation.

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
 drivers/iommu/dma-iommu.c | 18 +++---------------
 drivers/iommu/iommu.c     | 12 ++++++++++++
 include/linux/iommu.h     |  2 ++
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 3711b4a6e4f9..fa6f9098e77d 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -380,18 +380,6 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 	return iova_reserve_iommu_regions(dev, domain);
 }
 
-static int iommu_dma_deferred_attach(struct device *dev,
-		struct iommu_domain *domain)
-{
-	const struct iommu_ops *ops = domain->ops;
-
-	if (unlikely(ops->is_attach_deferred &&
-			ops->is_attach_deferred(domain, dev)))
-		return iommu_attach_device(domain, dev);
-
-	return 0;
-}
-
 /**
  * dma_info_to_prot - Translate DMA API directions and attributes to IOMMU API
  *                    page flags.
@@ -535,7 +523,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
 	dma_addr_t iova;
 
 	if (static_branch_unlikely(&__deferred_attach) &&
-	    iommu_dma_deferred_attach(dev, domain))
+	    iommu_do_deferred_attach(dev, domain))
 		return DMA_MAPPING_ERROR;
 
 	size = iova_align(iovad, size + iova_off);
@@ -694,7 +682,7 @@ static void *iommu_dma_alloc_remap(struct device *dev, size_t size,
 	*dma_handle = DMA_MAPPING_ERROR;
 
 	if (static_branch_unlikely(&__deferred_attach) &&
-	    iommu_dma_deferred_attach(dev, domain))
+	    iommu_do_deferred_attach(dev, domain))
 		return NULL;
 
 	min_size = alloc_sizes & -alloc_sizes;
@@ -1005,7 +993,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	int i;
 
 	if (static_branch_unlikely(&__deferred_attach) &&
-	    iommu_dma_deferred_attach(dev, domain))
+	    iommu_do_deferred_attach(dev, domain))
 		return 0;
 
 	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ffeebda8d6de..32164d355d2e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1980,6 +1980,18 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_attach_device);
 
+int iommu_do_deferred_attach(struct device *dev,
+			     struct iommu_domain *domain)
+{
+	const struct iommu_ops *ops = domain->ops;
+
+	if (unlikely(ops->is_attach_deferred &&
+		     ops->is_attach_deferred(domain, dev)))
+		return __iommu_attach_device(domain, dev);
+
+	return 0;
+}
+
 /*
  * Check flags and other user provided data for valid combinations. We also
  * make sure no reserved fields or unused flags are set. This is to ensure
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b3f0e2018c62..c04bbd30aa1d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -424,6 +424,8 @@ extern struct iommu_group *iommu_group_get_by_id(int id);
 extern void iommu_domain_free(struct iommu_domain *domain);
 extern int iommu_attach_device(struct iommu_domain *domain,
 			       struct device *dev);
+extern int iommu_do_deferred_attach(struct device *dev,
+				    struct iommu_domain *domain);
 extern void iommu_detach_device(struct iommu_domain *domain,
 				struct device *dev);
 extern int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2 v2] iommu: use the __iommu_attach_device() directly for deferred attach
  2021-01-19 11:16 ` [PATCH 2/2 v2] iommu: use the __iommu_attach_device() directly for deferred attach Lianbo Jiang
@ 2021-01-19 13:40   ` Robin Murphy
  2021-01-19 15:29   ` Christoph Hellwig
  1 sibling, 0 replies; 11+ messages in thread
From: Robin Murphy @ 2021-01-19 13:40 UTC (permalink / raw)
  To: Lianbo Jiang, linux-kernel
  Cc: joro, will, iommu, jsnitsel, thomas.lendacky, bhe

On 2021-01-19 11:16, Lianbo Jiang wrote:
> Currently, because domain attach allows to be deferred from iommu
> driver to device driver, and when iommu initializes, the devices
> on the bus will be scanned and the default groups will be allocated.
> 
> Due to the above changes, some devices could be added to the same
> group as below:
> 
> [    3.859417] pci 0000:01:00.0: Adding to iommu group 16
> [    3.864572] pci 0000:01:00.1: Adding to iommu group 16
> [    3.869738] pci 0000:02:00.0: Adding to iommu group 17
> [    3.874892] pci 0000:02:00.1: Adding to iommu group 17
> 
> But when attaching these devices, it doesn't allow that a group has
> more than one device, otherwise it will return an error. This conflicts
> with the deferred attaching. Unfortunately, it has two devices in the
> same group for my side, for example:
> 
> [    9.627014] iommu_group_device_count(): device name[0]:0000:01:00.0
> [    9.633545] iommu_group_device_count(): device name[1]:0000:01:00.1
> ...
> [   10.255609] iommu_group_device_count(): device name[0]:0000:02:00.0
> [   10.262144] iommu_group_device_count(): device name[1]:0000:02:00.1
> 
> Finally, which caused the failure of tg3 driver when tg3 driver calls
> the dma_alloc_coherent() to allocate coherent memory in the tg3_test_dma().
> 
> [    9.660310] tg3 0000:01:00.0: DMA engine test failed, aborting
> [    9.754085] tg3: probe of 0000:01:00.0 failed with error -12
> [    9.997512] tg3 0000:01:00.1: DMA engine test failed, aborting
> [   10.043053] tg3: probe of 0000:01:00.1 failed with error -12
> [   10.288905] tg3 0000:02:00.0: DMA engine test failed, aborting
> [   10.334070] tg3: probe of 0000:02:00.0 failed with error -12
> [   10.578303] tg3 0000:02:00.1: DMA engine test failed, aborting
> [   10.622629] tg3: probe of 0000:02:00.1 failed with error -12
> 
> In addition, the similar situations also occur in other drivers such
> as the bnxt_en driver. That can be reproduced easily in kdump kernel
> when SME is active
> 
> Let's move the handling currently in iommu_dma_deferred_attach() into
> the iommu core code so that it can call the __iommu_attach_device()
> directly instead of the iommu_attach_device(). The external interface
> iommu_attach_device() is not suitable for handling this situation.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> ---
>   drivers/iommu/dma-iommu.c | 18 +++---------------
>   drivers/iommu/iommu.c     | 12 ++++++++++++
>   include/linux/iommu.h     |  2 ++
>   3 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 3711b4a6e4f9..fa6f9098e77d 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -380,18 +380,6 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
>   	return iova_reserve_iommu_regions(dev, domain);
>   }
>   
> -static int iommu_dma_deferred_attach(struct device *dev,
> -		struct iommu_domain *domain)
> -{
> -	const struct iommu_ops *ops = domain->ops;
> -
> -	if (unlikely(ops->is_attach_deferred &&
> -			ops->is_attach_deferred(domain, dev)))
> -		return iommu_attach_device(domain, dev);
> -
> -	return 0;
> -}
> -
>   /**
>    * dma_info_to_prot - Translate DMA API directions and attributes to IOMMU API
>    *                    page flags.
> @@ -535,7 +523,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
>   	dma_addr_t iova;
>   
>   	if (static_branch_unlikely(&__deferred_attach) &&
> -	    iommu_dma_deferred_attach(dev, domain))
> +	    iommu_do_deferred_attach(dev, domain))
>   		return DMA_MAPPING_ERROR;
>   
>   	size = iova_align(iovad, size + iova_off);
> @@ -694,7 +682,7 @@ static void *iommu_dma_alloc_remap(struct device *dev, size_t size,
>   	*dma_handle = DMA_MAPPING_ERROR;
>   
>   	if (static_branch_unlikely(&__deferred_attach) &&
> -	    iommu_dma_deferred_attach(dev, domain))
> +	    iommu_do_deferred_attach(dev, domain))
>   		return NULL;
>   
>   	min_size = alloc_sizes & -alloc_sizes;
> @@ -1005,7 +993,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   	int i;
>   
>   	if (static_branch_unlikely(&__deferred_attach) &&
> -	    iommu_dma_deferred_attach(dev, domain))
> +	    iommu_do_deferred_attach(dev, domain))
>   		return 0;
>   
>   	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index ffeebda8d6de..32164d355d2e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1980,6 +1980,18 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(iommu_attach_device);
>   
> +int iommu_do_deferred_attach(struct device *dev,
> +			     struct iommu_domain *domain)
> +{
> +	const struct iommu_ops *ops = domain->ops;
> +
> +	if (unlikely(ops->is_attach_deferred &&
> +		     ops->is_attach_deferred(domain, dev)))
> +		return __iommu_attach_device(domain, dev);
> +
> +	return 0;
> +}
> +
>   /*
>    * Check flags and other user provided data for valid combinations. We also
>    * make sure no reserved fields or unused flags are set. This is to ensure
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index b3f0e2018c62..c04bbd30aa1d 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -424,6 +424,8 @@ extern struct iommu_group *iommu_group_get_by_id(int id);
>   extern void iommu_domain_free(struct iommu_domain *domain);
>   extern int iommu_attach_device(struct iommu_domain *domain,
>   			       struct device *dev);
> +extern int iommu_do_deferred_attach(struct device *dev,
> +				    struct iommu_domain *domain);
>   extern void iommu_detach_device(struct iommu_domain *domain,
>   				struct device *dev);
>   extern int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device
  2021-01-19 11:16 [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device Lianbo Jiang
  2021-01-19 11:16 ` [PATCH 1/2 v2] dma-iommu: use static-key to minimize the impact in the fast-path Lianbo Jiang
  2021-01-19 11:16 ` [PATCH 2/2 v2] iommu: use the __iommu_attach_device() directly for deferred attach Lianbo Jiang
@ 2021-01-19 13:46 ` Lu Baolu
  2021-01-28 11:04 ` Joerg Roedel
  3 siblings, 0 replies; 11+ messages in thread
From: Lu Baolu @ 2021-01-19 13:46 UTC (permalink / raw)
  To: Lianbo Jiang, linux-kernel
  Cc: baolu.lu, thomas.lendacky, will, iommu, robin.murphy

On 2021/1/19 19:16, Lianbo Jiang wrote:
> This patchset is to fix the failure of deferred attach for iommu attach
> device, it includes the following two patches:
> 
> [1] [PATCH 1/2] dma-iommu: use static-key to minimize the impact in the fast-path
>      This is a prepared patch for the second one, move out the is_kdump_kernel()
>      check from iommu_dma_deferred_attach() to iommu_dma_init(), and use the
>      static-key in the fast-path to minimize the impact in the normal case.
> 
> [2] [PATCH 2/2] iommu: use the __iommu_attach_device() directly for deferred attach
>      Move the handling currently in iommu_dma_deferred_attach() into the
>      iommu core code so that it can call the __iommu_attach_device()
>      directly instead of the iommu_attach_device(). The external interface
>      iommu_attach_device() is not suitable for handling this situation.
> 
> Changes since v1:
> [1] use the __iommu_attach_device() directly for deferred attach
> [2] use static-key to minimize the impact in the fast-path
> 
> Lianbo Jiang (2):
>    dma-iommu: use static-key to minimize the impact in the fast-path
>    iommu: use the __iommu_attach_device() directly for deferred attach
> 
>   drivers/iommu/dma-iommu.c | 29 +++++++++++------------------
>   drivers/iommu/iommu.c     | 12 ++++++++++++
>   include/linux/iommu.h     |  2 ++
>   3 files changed, 25 insertions(+), 18 deletions(-)
> 

Good fix and improvement. For the patches in this series:

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2 v2] dma-iommu: use static-key to minimize the impact in the fast-path
  2021-01-19 11:16 ` [PATCH 1/2 v2] dma-iommu: use static-key to minimize the impact in the fast-path Lianbo Jiang
@ 2021-01-19 15:26   ` Christoph Hellwig
  2021-01-19 15:52     ` Robin Murphy
  2021-01-21 12:21     ` lijiang
  0 siblings, 2 replies; 11+ messages in thread
From: Christoph Hellwig @ 2021-01-19 15:26 UTC (permalink / raw)
  To: Lianbo Jiang; +Cc: linux-kernel, thomas.lendacky, will, iommu, robin.murphy

On Tue, Jan 19, 2021 at 07:16:15PM +0800, Lianbo Jiang wrote:
> +static DEFINE_STATIC_KEY_FALSE(__deferred_attach);

Why the strange underscores?  Wouldn't iommu_deferred_attach_enabled
be a better name?

> -	if (unlikely(iommu_dma_deferred_attach(dev, domain)))
> +	if (static_branch_unlikely(&__deferred_attach) &&
> +	    iommu_dma_deferred_attach(dev, domain))

Also insted of duplicating this logic in three places, maybe rename
iommu_dma_deferred_attach to __iommu_dma_deferred_attach and create
a small inline wrapper for it?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2 v2] iommu: use the __iommu_attach_device() directly for deferred attach
  2021-01-19 11:16 ` [PATCH 2/2 v2] iommu: use the __iommu_attach_device() directly for deferred attach Lianbo Jiang
  2021-01-19 13:40   ` Robin Murphy
@ 2021-01-19 15:29   ` Christoph Hellwig
  2021-01-21 12:57     ` lijiang
  1 sibling, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2021-01-19 15:29 UTC (permalink / raw)
  To: Lianbo Jiang; +Cc: linux-kernel, thomas.lendacky, will, iommu, robin.murphy

> +int iommu_do_deferred_attach(struct device *dev,
> +			     struct iommu_domain *domain)

I'd remove the "do_" from the name, it doesn't really add any value.

> +{
> +	const struct iommu_ops *ops = domain->ops;
> +
> +	if (unlikely(ops->is_attach_deferred &&
> +		     ops->is_attach_deferred(domain, dev)))
> +		return __iommu_attach_device(domain, dev);
> +
> +	return 0;

Now that everyting is under the static key we don't really need to
bother with the unlikely micro optimization, do we?

> +extern int iommu_do_deferred_attach(struct device *dev,
> +				    struct iommu_domain *domain);

No need for the extern.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2 v2] dma-iommu: use static-key to minimize the impact in the fast-path
  2021-01-19 15:26   ` Christoph Hellwig
@ 2021-01-19 15:52     ` Robin Murphy
  2021-01-21 12:21     ` lijiang
  1 sibling, 0 replies; 11+ messages in thread
From: Robin Murphy @ 2021-01-19 15:52 UTC (permalink / raw)
  To: Christoph Hellwig, Lianbo Jiang
  Cc: linux-kernel, thomas.lendacky, will, iommu

On 2021-01-19 15:26, Christoph Hellwig wrote:
> On Tue, Jan 19, 2021 at 07:16:15PM +0800, Lianbo Jiang wrote:
>> +static DEFINE_STATIC_KEY_FALSE(__deferred_attach);
> 
> Why the strange underscores?  Wouldn't iommu_deferred_attach_enabled
> be a better name?
> 
>> -	if (unlikely(iommu_dma_deferred_attach(dev, domain)))
>> +	if (static_branch_unlikely(&__deferred_attach) &&
>> +	    iommu_dma_deferred_attach(dev, domain))
> 
> Also insted of duplicating this logic in three places, maybe rename
> iommu_dma_deferred_attach to __iommu_dma_deferred_attach and create
> a small inline wrapper for it?

Once patch #2 is in place, I really don't see any point. The "helper" 
would add a minimum of 5 lines to save at most 3, and would have to be 
annotated as always_inline - which a whole other camp of people would 
probably object to - in order for the static branch to be properly useful.

It's not as if this is a complex or hard-to-read expression, so IMO 
having 3 lines repeated 3 times is objectively better than having 2 
lines necessarily repeated 3 times plus having to scroll up and find 
several more lines to follow what it's doing.

Robin.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2 v2] dma-iommu: use static-key to minimize the impact in the fast-path
  2021-01-19 15:26   ` Christoph Hellwig
  2021-01-19 15:52     ` Robin Murphy
@ 2021-01-21 12:21     ` lijiang
  1 sibling, 0 replies; 11+ messages in thread
From: lijiang @ 2021-01-21 12:21 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-kernel, thomas.lendacky, will, iommu, robin.murphy

Hi, Christoph

Thanks for the comment.
在 2021年01月19日 23:26, Christoph Hellwig 写道:
> On Tue, Jan 19, 2021 at 07:16:15PM +0800, Lianbo Jiang wrote:
>> +static DEFINE_STATIC_KEY_FALSE(__deferred_attach);
> Why the strange underscores?  Wouldn't iommu_deferred_attach_enabled

The variable is defined with the static keyword, which indicates that the
variable is only used in the local module(file), and gives a hint explicitly
with the underscore prefix. Anyway, this is my personal opinion.

> be a better name?
> 
It could be a long name?

Thanks.
Lianbo


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2 v2] iommu: use the __iommu_attach_device() directly for deferred attach
  2021-01-19 15:29   ` Christoph Hellwig
@ 2021-01-21 12:57     ` lijiang
  0 siblings, 0 replies; 11+ messages in thread
From: lijiang @ 2021-01-21 12:57 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-kernel, thomas.lendacky, will, iommu, robin.murphy

Hi, Christoph

在 2021年01月19日 23:29, Christoph Hellwig 写道:
>> +int iommu_do_deferred_attach(struct device *dev,
>> +			     struct iommu_domain *domain)
> 
> I'd remove the "do_" from the name, it doesn't really add any value.
> 
OK.

>> +{
>> +	const struct iommu_ops *ops = domain->ops;
>> +
>> +	if (unlikely(ops->is_attach_deferred &&
>> +		     ops->is_attach_deferred(domain, dev)))
>> +		return __iommu_attach_device(domain, dev);
>> +
>> +	return 0;
> 
> Now that everyting is under the static key we don't really need to
> bother with the unlikely micro optimization, do we?
> 
Good understanding. I can remove it, otherwise it should use the likely().

>> +extern int iommu_do_deferred_attach(struct device *dev,
>> +				    struct iommu_domain *domain);
> 
> No need for the extern.
> 
Sounds good. I will remove the 'extern' when I post again.

Thanks.
Lianbo


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device
  2021-01-19 11:16 [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device Lianbo Jiang
                   ` (2 preceding siblings ...)
  2021-01-19 13:46 ` [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device Lu Baolu
@ 2021-01-28 11:04 ` Joerg Roedel
  3 siblings, 0 replies; 11+ messages in thread
From: Joerg Roedel @ 2021-01-28 11:04 UTC (permalink / raw)
  To: Lianbo Jiang
  Cc: linux-kernel, will, iommu, jsnitsel, thomas.lendacky, robin.murphy, bhe

On Tue, Jan 19, 2021 at 07:16:14PM +0800, Lianbo Jiang wrote:
> Lianbo Jiang (2):
>   dma-iommu: use static-key to minimize the impact in the fast-path
>   iommu: use the __iommu_attach_device() directly for deferred attach
> 
>  drivers/iommu/dma-iommu.c | 29 +++++++++++------------------
>  drivers/iommu/iommu.c     | 12 ++++++++++++
>  include/linux/iommu.h     |  2 ++
>  3 files changed, 25 insertions(+), 18 deletions(-)

Applied, thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-01-28 11:05 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-19 11:16 [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device Lianbo Jiang
2021-01-19 11:16 ` [PATCH 1/2 v2] dma-iommu: use static-key to minimize the impact in the fast-path Lianbo Jiang
2021-01-19 15:26   ` Christoph Hellwig
2021-01-19 15:52     ` Robin Murphy
2021-01-21 12:21     ` lijiang
2021-01-19 11:16 ` [PATCH 2/2 v2] iommu: use the __iommu_attach_device() directly for deferred attach Lianbo Jiang
2021-01-19 13:40   ` Robin Murphy
2021-01-19 15:29   ` Christoph Hellwig
2021-01-21 12:57     ` lijiang
2021-01-19 13:46 ` [PATCH 0/2 v2] iommu: fix the failure of deferred attach for iommu attach device Lu Baolu
2021-01-28 11:04 ` Joerg Roedel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).