[PATCH] swiotlb: suppress warning when __GFP_NOWARN is set v2

[PATCH] swiotlb: suppress warning when __GFP_NOWARN is set v2

[Christian König]

TTM tries to allocate coherent memory in chunks of 2MB first to improve
TLB efficiency and falls back to allocating 4K pages if that fails.

Suppress the warning when the 2MB allocations fails since there is a
valid fall back path.

v2: suppress warnings from swiotlb_tbl_map_single as well

Signed-off-by: Christian König <christian.koenig@amd.com>
Reported-by: Mike Galbraith <efault@gmx.de>
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=104082
CC: stable@vger.kernel.org
---
 lib/swiotlb.c | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index cea19aaf303c..ed443d65a8e2 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -490,11 +490,11 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr,
 	}
 }
 
-phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
-				   dma_addr_t tbl_dma_addr,
-				   phys_addr_t orig_addr, size_t size,
-				   enum dma_data_direction dir,
-				   unsigned long attrs)
+phys_addr_t _swiotlb_tbl_map_single(struct device *hwdev,
+				    dma_addr_t tbl_dma_addr,
+				    phys_addr_t orig_addr, size_t size,
+				    enum dma_data_direction dir,
+				    unsigned long attrs, bool warn)
 {
 	unsigned long flags;
 	phys_addr_t tlb_addr;
@@ -586,7 +586,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
 
 not_found:
 	spin_unlock_irqrestore(&io_tlb_lock, flags);
-	if (printk_ratelimit())
+	if (warn && printk_ratelimit())
 		dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes)\n", size);
 	return SWIOTLB_MAP_ERROR;
 found:
@@ -605,6 +605,16 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
 
 	return tlb_addr;
 }
+
+phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
+				   dma_addr_t tbl_dma_addr,
+				   phys_addr_t orig_addr, size_t size,
+				   enum dma_data_direction dir,
+				   unsigned long attrs)
+{
+	return _swiotlb_tbl_map_single(hwdev, tbl_dma_addr, orig_addr,
+				       size, dir, attrs, true);
+}
 EXPORT_SYMBOL_GPL(swiotlb_tbl_map_single);
 
 /*
@@ -613,7 +623,7 @@ EXPORT_SYMBOL_GPL(swiotlb_tbl_map_single);
 
 static phys_addr_t
 map_single(struct device *hwdev, phys_addr_t phys, size_t size,
-	   enum dma_data_direction dir, unsigned long attrs)
+	   enum dma_data_direction dir, unsigned long attrs, bool warn)
 {
 	dma_addr_t start_dma_addr;
 
@@ -624,8 +634,8 @@ map_single(struct device *hwdev, phys_addr_t phys, size_t size,
 	}
 
 	start_dma_addr = swiotlb_phys_to_dma(hwdev, io_tlb_start);
-	return swiotlb_tbl_map_single(hwdev, start_dma_addr, phys, size,
-				      dir, attrs);
+	return _swiotlb_tbl_map_single(hwdev, start_dma_addr, phys, size,
+				       dir, attrs, warn);
 }
 
 /*
@@ -713,6 +723,7 @@ void *
 swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 		       dma_addr_t *dma_handle, gfp_t flags)
 {
+	bool warn = !(flags & __GFP_NOWARN);
 	dma_addr_t dev_addr;
 	void *ret;
 	int order = get_order(size);
@@ -739,7 +750,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 		 * will grab memory from the lowest available address range.
 		 */
 		phys_addr_t paddr = map_single(hwdev, 0, size,
-					       DMA_FROM_DEVICE, 0);
+					       DMA_FROM_DEVICE, 0, warn);
 		if (paddr == SWIOTLB_MAP_ERROR)
 			goto err_warn;
 
@@ -769,9 +780,11 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 	return ret;
 
 err_warn:
-	pr_warn("swiotlb: coherent allocation failed for device %s size=%zu\n",
-		dev_name(hwdev), size);
-	dump_stack();
+	if (warn) {
+		pr_warn("swiotlb: coherent allocation failed for device %s size=%zu\n",
+			dev_name(hwdev), size);
+		dump_stack();
+	}
 
 	return NULL;
 }
@@ -851,7 +864,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
 
 	/* Oh well, have to allocate and map a bounce buffer. */
-	map = map_single(dev, phys, size, dir, attrs);
+	map = map_single(dev, phys, size, dir, attrs, true);
 	if (map == SWIOTLB_MAP_ERROR) {
 		swiotlb_full(dev, size, dir, 1);
 		return swiotlb_phys_to_dma(dev, io_tlb_overflow_buffer);
@@ -989,7 +1002,8 @@ swiotlb_map_sg_attrs(struct device *hwdev, struct scatterlist *sgl, int nelems,
 		if (swiotlb_force == SWIOTLB_FORCE ||
 		    !dma_capable(hwdev, dev_addr, sg->length)) {
 			phys_addr_t map = map_single(hwdev, sg_phys(sg),
-						     sg->length, dir, attrs);
+						     sg->length, dir, attrs,
+						     true);
 			if (map == SWIOTLB_MAP_ERROR) {
 				/* Don't panic here, we expect map_sg users
 				   to do proper error handling. */
-- 
2.11.0

Re: [PATCH] swiotlb: suppress warning when __GFP_NOWARN is set v2

[Mike Galbraith]

On Tue, 2018-01-02 at 13:13 +0100, Christian König wrote:
> 
> v2: suppress warnings from swiotlb_tbl_map_single as well

Thanks, dmesg spam is history.

	-Mike

Re: [PATCH] swiotlb: suppress warning when __GFP_NOWARN is set v2

[Chris Wilson]

Quoting Christian König (2018-01-02 12:13:58)
> TTM tries to allocate coherent memory in chunks of 2MB first to improve
> TLB efficiency and falls back to allocating 4K pages if that fails.
> 
> Suppress the warning when the 2MB allocations fails since there is a
> valid fall back path.
> 
> v2: suppress warnings from swiotlb_tbl_map_single as well
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reported-by: Mike Galbraith <efault@gmx.de>
> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=104082
> CC: stable@vger.kernel.org
> ---
> @@ -989,7 +1002,8 @@ swiotlb_map_sg_attrs(struct device *hwdev, struct scatterlist *sgl, int nelems,
>                 if (swiotlb_force == SWIOTLB_FORCE ||
>                     !dma_capable(hwdev, dev_addr, sg->length)) {
>                         phys_addr_t map = map_single(hwdev, sg_phys(sg),
> -                                                    sg->length, dir, attrs);
> +                                                    sg->length, dir, attrs,
> +                                                    true);

This keeps the warning for dma_map_sg(), so seems quite ineffective.

>                         if (map == SWIOTLB_MAP_ERROR) {
>                                 /* Don't panic here, we expect map_sg users
>                                    to do proper error handling. */

And counter to the suggestion here?
-Chris

Re: [PATCH] swiotlb: suppress warning when __GFP_NOWARN is set v2

[Christian König]

Am 02.01.2018 um 16:39 schrieb Chris Wilson:
> Quoting Christian König (2018-01-02 12:13:58)
>> TTM tries to allocate coherent memory in chunks of 2MB first to improve
>> TLB efficiency and falls back to allocating 4K pages if that fails.
>>
>> Suppress the warning when the 2MB allocations fails since there is a
>> valid fall back path.
>>
>> v2: suppress warnings from swiotlb_tbl_map_single as well
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reported-by: Mike Galbraith <efault@gmx.de>
>> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=104082
>> CC: stable@vger.kernel.org
>> ---
>> @@ -989,7 +1002,8 @@ swiotlb_map_sg_attrs(struct device *hwdev, struct scatterlist *sgl, int nelems,
>>                  if (swiotlb_force == SWIOTLB_FORCE ||
>>                      !dma_capable(hwdev, dev_addr, sg->length)) {
>>                          phys_addr_t map = map_single(hwdev, sg_phys(sg),
>> -                                                    sg->length, dir, attrs);
>> +                                                    sg->length, dir, attrs,
>> +                                                    true);
> This keeps the warning for dma_map_sg(), so seems quite ineffective.

That is the desired result, dma_map_sg() should still print the warning.

We only want to disable it for swiotlb_alloc_coherent().

Regards,
Christian.

>
>>                          if (map == SWIOTLB_MAP_ERROR) {
>>                                  /* Don't panic here, we expect map_sg users
>>                                     to do proper error handling. */
> And counter to the suggestion here?

We don't use swiotlb_map_sg_attrs() in TTM, so that is irrelevant for my 
use case and I can't test any change regarding it.

Regards,
Christian.

> -Chris

Re: [PATCH] swiotlb: suppress warning when __GFP_NOWARN is set v2

[Konrad Rzeszutek Wilk]

On Tue, Jan 02, 2018 at 01:13:58PM +0100, Christian König wrote:
> TTM tries to allocate coherent memory in chunks of 2MB first to improve
> TLB efficiency and falls back to allocating 4K pages if that fails.
> 
> Suppress the warning when the 2MB allocations fails since there is a
> valid fall back path.
> 
> v2: suppress warnings from swiotlb_tbl_map_single as well
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reported-by: Mike Galbraith <efault@gmx.de>
> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=104082
> CC: stable@vger.kernel.org
> ---
>  lib/swiotlb.c | 44 +++++++++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index cea19aaf303c..ed443d65a8e2 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -490,11 +490,11 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr,
>  	}
>  }
>  
> -phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> -				   dma_addr_t tbl_dma_addr,
> -				   phys_addr_t orig_addr, size_t size,
> -				   enum dma_data_direction dir,
> -				   unsigned long attrs)
> +phys_addr_t _swiotlb_tbl_map_single(struct device *hwdev,

Just ditch the '_swiotlb' and make it 'tbl_map_single'
> +				    dma_addr_t tbl_dma_addr,
> +				    phys_addr_t orig_addr, size_t size,
> +				    enum dma_data_direction dir,
> +				    unsigned long attrs, bool warn)
>  {
>  	unsigned long flags;
>  	phys_addr_t tlb_addr;
> @@ -586,7 +586,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>  
>  not_found:
>  	spin_unlock_irqrestore(&io_tlb_lock, flags);
> -	if (printk_ratelimit())
> +	if (warn && printk_ratelimit())
>  		dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes)\n", size);
>  	return SWIOTLB_MAP_ERROR;
>  found:
> @@ -605,6 +605,16 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>  
>  	return tlb_addr;
>  }
> +
> +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> +				   dma_addr_t tbl_dma_addr,
> +				   phys_addr_t orig_addr, size_t size,
> +				   enum dma_data_direction dir,
> +				   unsigned long attrs)

Hm, could be my editor, but are the parameters on the same line as 'struct device *hwdev'?

> +{
> +	return _swiotlb_tbl_map_single(hwdev, tbl_dma_addr, orig_addr,
> +				       size, dir, attrs, true);
> +}
>  EXPORT_SYMBOL_GPL(swiotlb_tbl_map_single);
>  
>  /*
> @@ -613,7 +623,7 @@ EXPORT_SYMBOL_GPL(swiotlb_tbl_map_single);
>  
>  static phys_addr_t
>  map_single(struct device *hwdev, phys_addr_t phys, size_t size,
> -	   enum dma_data_direction dir, unsigned long attrs)
> +	   enum dma_data_direction dir, unsigned long attrs, bool warn)
>  {
>  	dma_addr_t start_dma_addr;
>  
> @@ -624,8 +634,8 @@ map_single(struct device *hwdev, phys_addr_t phys, size_t size,
>  	}
>  
>  	start_dma_addr = swiotlb_phys_to_dma(hwdev, io_tlb_start);
> -	return swiotlb_tbl_map_single(hwdev, start_dma_addr, phys, size,
> -				      dir, attrs);
> +	return _swiotlb_tbl_map_single(hwdev, start_dma_addr, phys, size,
> +				       dir, attrs, warn);
>  }
>  
>  /*
> @@ -713,6 +723,7 @@ void *
>  swiotlb_alloc_coherent(struct device *hwdev, size_t size,
>  		       dma_addr_t *dma_handle, gfp_t flags)
>  {
> +	bool warn = !(flags & __GFP_NOWARN);
>  	dma_addr_t dev_addr;
>  	void *ret;
>  	int order = get_order(size);
> @@ -739,7 +750,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
>  		 * will grab memory from the lowest available address range.
>  		 */
>  		phys_addr_t paddr = map_single(hwdev, 0, size,
> -					       DMA_FROM_DEVICE, 0);
> +					       DMA_FROM_DEVICE, 0, warn);
>  		if (paddr == SWIOTLB_MAP_ERROR)
>  			goto err_warn;
>  
> @@ -769,9 +780,11 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
>  	return ret;
>  
>  err_warn:
> -	pr_warn("swiotlb: coherent allocation failed for device %s size=%zu\n",
> -		dev_name(hwdev), size);
> -	dump_stack();
> +	if (warn) {

&& printk_ratelimit() ?

> +		pr_warn("swiotlb: coherent allocation failed for device %s size=%zu\n",
> +			dev_name(hwdev), size);
> +		dump_stack();
> +	}
>  
>  	return NULL;
>  }
> @@ -851,7 +864,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
>  	trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
>  
>  	/* Oh well, have to allocate and map a bounce buffer. */
> -	map = map_single(dev, phys, size, dir, attrs);
> +	map = map_single(dev, phys, size, dir, attrs, true);
>  	if (map == SWIOTLB_MAP_ERROR) {
>  		swiotlb_full(dev, size, dir, 1);
>  		return swiotlb_phys_to_dma(dev, io_tlb_overflow_buffer);
> @@ -989,7 +1002,8 @@ swiotlb_map_sg_attrs(struct device *hwdev, struct scatterlist *sgl, int nelems,
>  		if (swiotlb_force == SWIOTLB_FORCE ||
>  		    !dma_capable(hwdev, dev_addr, sg->length)) {
>  			phys_addr_t map = map_single(hwdev, sg_phys(sg),
> -						     sg->length, dir, attrs);
> +						     sg->length, dir, attrs,
> +						     true);

s/true/true /*Always warn.*/

>  			if (map == SWIOTLB_MAP_ERROR) {
>  				/* Don't panic here, we expect map_sg users
>  				   to do proper error handling. */
> -- 
> 2.11.0
> 

Re: [PATCH] swiotlb: suppress warning when __GFP_NOWARN is set v2

[Christian König]

Am 02.01.2018 um 21:51 schrieb Konrad Rzeszutek Wilk:
> On Tue, Jan 02, 2018 at 01:13:58PM +0100, Christian König wrote:
> [SNIP]
>> +
>> +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>> +				   dma_addr_t tbl_dma_addr,
>> +				   phys_addr_t orig_addr, size_t size,
>> +				   enum dma_data_direction dir,
>> +				   unsigned long attrs)
> Hm, could be my editor, but are the parameters on the same line as 'struct device *hwdev'?

Looks like just an issue in your editor.

I've fixed all other suggestions and going to send out v3 of the patch 
in a minute.

Thanks,
Christian.

Re: [PATCH] swiotlb: suppress warning when __GFP_NOWARN is set v2

[kbuild test robot]

Hi Christian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.15-rc5]
[also build test WARNING on next-20180104]
[cannot apply to swiotlb/linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christian-K-nig/swiotlb-suppress-warning-when-__GFP_NOWARN-is-set-v2/20180104-185406
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)


Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] swiotlb: _swiotlb_tbl_map_single() can be static

[kbuild test robot]

Fixes: bd4bb89b2f71 ("swiotlb: suppress warning when __GFP_NOWARN is set v2")
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 swiotlb.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index ed443d6..e253e80 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -490,11 +490,11 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr,
 	}
 }
 
-phys_addr_t _swiotlb_tbl_map_single(struct device *hwdev,
-				    dma_addr_t tbl_dma_addr,
-				    phys_addr_t orig_addr, size_t size,
-				    enum dma_data_direction dir,
-				    unsigned long attrs, bool warn)
+static phys_addr_t _swiotlb_tbl_map_single(struct device *hwdev,
+					   dma_addr_t tbl_dma_addr,
+					   phys_addr_t orig_addr, size_t size,
+					   enum dma_data_direction dir,
+					   unsigned long attrs, bool warn)
 {
 	unsigned long flags;
 	phys_addr_t tlb_addr;