[PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

The result was printing the warning only when we were explicitly asked
not to.

Cc: stable@vger.kernel.org
Fixes: 0176adb004065d6815a8e67946752df4cd947c5b "swiotlb: refactor
 coherent buffer allocation"
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 lib/swiotlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index c43ec2271469..e9ac21540628 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -750,7 +750,7 @@ swiotlb_alloc_buffer(struct device *dev, size_t size, dma_addr_t *dma_handle,
 	swiotlb_tbl_unmap_single(dev, phys_addr, size, DMA_TO_DEVICE,
 			DMA_ATTR_SKIP_CPU_SYNC);
 out_warn:
-	if ((attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) {
+	if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) {
 		dev_warn(dev,
 			"swiotlb: coherent allocation failed, size=%zu\n",
 			size);
-- 
2.17.0

Re: [PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Christian König]

Am 01.05.2018 um 15:24 schrieb Michel Dänzer:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> The result was printing the warning only when we were explicitly asked
> not to.
>
> Cc: stable@vger.kernel.org
> Fixes: 0176adb004065d6815a8e67946752df4cd947c5b "swiotlb: refactor
>   coherent buffer allocation"
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

Good catch, looked at the code multiple times and haven't seen that 
myself :)

Reviewed-by: Christian König <christian.koenig@amd.com>.

Christian.

> ---
>   lib/swiotlb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index c43ec2271469..e9ac21540628 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -750,7 +750,7 @@ swiotlb_alloc_buffer(struct device *dev, size_t size, dma_addr_t *dma_handle,
>   	swiotlb_tbl_unmap_single(dev, phys_addr, size, DMA_TO_DEVICE,
>   			DMA_ATTR_SKIP_CPU_SYNC);
>   out_warn:
> -	if ((attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) {
> +	if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) {
>   		dev_warn(dev,
>   			"swiotlb: coherent allocation failed, size=%zu\n",
>   			size);

Re: [PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Daniel Vetter]

On Wed, May 2, 2018 at 11:49 AM, Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
> Am 01.05.2018 um 15:24 schrieb Michel Dänzer:
>>
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> The result was printing the warning only when we were explicitly asked
>> not to.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 0176adb004065d6815a8e67946752df4cd947c5b "swiotlb: refactor
>>   coherent buffer allocation"
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>
>
> Good catch, looked at the code multiple times and haven't seen that myself
> :)
>
> Reviewed-by: Christian König <christian.koenig@amd.com>.

Other dma-api backends like cma just shut up when __GFP_NOWARN is
passed. And afaiui Christoph Hellwig has plans to nuke the DMA_ATTR
stuff (or at least clean it up) - should we just remove
DMA_ATTR_NO_WARN and instead only look at __GFP_NOWARN?

For context the CMA patch:

commit ef4650144e76ae361fe4b8c9a0afcd53074cd520
Author: Boris Brezillon <boris.brezillon@free-electrons.com>
Date:   Fri Oct 13 15:58:01 2017 -0700

    mm/cma.c: take __GFP_NOWARN into account in cma_alloc()

Or maybe we should at least enforce that both or none are set, for
consistency for now?

Cheers, Daniel


>
> Christian.
>
>> ---
>>   lib/swiotlb.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>> index c43ec2271469..e9ac21540628 100644
>> --- a/lib/swiotlb.c
>> +++ b/lib/swiotlb.c
>> @@ -750,7 +750,7 @@ swiotlb_alloc_buffer(struct device *dev, size_t size,
>> dma_addr_t *dma_handle,
>>         swiotlb_tbl_unmap_single(dev, phys_addr, size, DMA_TO_DEVICE,
>>                         DMA_ATTR_SKIP_CPU_SYNC);
>>   out_warn:
>> -       if ((attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) {
>> +       if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) {
>>                 dev_warn(dev,
>>                         "swiotlb: coherent allocation failed, size=%zu\n",
>>                         size);
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Christoph Hellwig]

On Wed, May 02, 2018 at 02:18:56PM +0200, Daniel Vetter wrote:
> Other dma-api backends like cma just shut up when __GFP_NOWARN is
> passed. And afaiui Christoph Hellwig has plans to nuke the DMA_ATTR
> stuff (or at least clean it up) - should we just remove
> DMA_ATTR_NO_WARN and instead only look at __GFP_NOWARN?

No.  __GFP_NOWARN (and gfp_t flags in general) are the wrong interface
for dma allocations and just cause problems.  I actually plan to
get rid of the gfp_t argument in dma_alloc_attrs sooner, and only
allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent.

> Or maybe we should at least enforce that both or none are set, for
> consistency for now?

The interface should be DMA_ATTR_NO_WARN.  __GFP_NOWARN in this
context was never documented, and just slipped in.

Re: [PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Christoph Hellwig]

On Tue, May 01, 2018 at 03:24:11PM +0200, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> The result was printing the warning only when we were explicitly asked
> not to.

Thanks, applied to the dma-mapping tree for 4.17.

Re: [PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Michel Dänzer]

On 2018-05-02 02:41 PM, Christoph Hellwig wrote:
> On Wed, May 02, 2018 at 02:18:56PM +0200, Daniel Vetter wrote:
>> Other dma-api backends like cma just shut up when __GFP_NOWARN is
>> passed. And afaiui Christoph Hellwig has plans to nuke the DMA_ATTR
>> stuff (or at least clean it up) - should we just remove
>> DMA_ATTR_NO_WARN and instead only look at __GFP_NOWARN?
> 
> No.  __GFP_NOWARN (and gfp_t flags in general) are the wrong interface
> for dma allocations and just cause problems.  I actually plan to
> get rid of the gfp_t argument in dma_alloc_attrs sooner, and only
> allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent.

How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically
allocate huge pages (GFP_TRANSHUGE can result in unacceptably long
delays with memory pressure).


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Christoph Hellwig]

On Wed, May 02, 2018 at 04:31:09PM +0200, Michel Dänzer wrote:
> > No.  __GFP_NOWARN (and gfp_t flags in general) are the wrong interface
> > for dma allocations and just cause problems.  I actually plan to
> > get rid of the gfp_t argument in dma_alloc_attrs sooner, and only
> > allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent.
> 
> How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically
> allocate huge pages (GFP_TRANSHUGE can result in unacceptably long
> delays with memory pressure).

Well, that is exactly what I don't want drivers to do - same for
__GFP_COMP in some drm code.  This very much assumes the page allocator
is used to back dma allocations, which very often it actually isn't, and
any use of magic gfp flags creates a tight coupling of consumers with a
specific implementation.

In general I can't think of a good reason not to actually use
GFP_TRANSHUGE_LIGHT by default in the dma allocator unless
DMA_ATTR_ALLOC_SINGLE_PAGES is set.  Can you prepare a patch for that?

Re: [PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Michel Dänzer]

On 2018-05-02 06:21 PM, Christoph Hellwig wrote:
> On Wed, May 02, 2018 at 04:31:09PM +0200, Michel Dänzer wrote:
>>> No.  __GFP_NOWARN (and gfp_t flags in general) are the wrong interface
>>> for dma allocations and just cause problems.  I actually plan to
>>> get rid of the gfp_t argument in dma_alloc_attrs sooner, and only
>>> allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent.
>>
>> How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically
>> allocate huge pages (GFP_TRANSHUGE can result in unacceptably long
>> delays with memory pressure).
> 
> Well, that is exactly what I don't want drivers to do - same for
> __GFP_COMP in some drm code.  This very much assumes the page allocator
> is used to back dma allocations, which very often it actually isn't, and
> any use of magic gfp flags creates a tight coupling of consumers with a
> specific implementation.
> 
> In general I can't think of a good reason not to actually use
> GFP_TRANSHUGE_LIGHT by default in the dma allocator unless
> DMA_ATTR_ALLOC_SINGLE_PAGES is set.  Can you prepare a patch for that?

I'm afraid I'll have to leave that to somebody else.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Christian König]

Am 02.05.2018 um 18:59 schrieb Michel Dänzer:
> On 2018-05-02 06:21 PM, Christoph Hellwig wrote:
>> On Wed, May 02, 2018 at 04:31:09PM +0200, Michel Dänzer wrote:
>>>> No.  __GFP_NOWARN (and gfp_t flags in general) are the wrong interface
>>>> for dma allocations and just cause problems.  I actually plan to
>>>> get rid of the gfp_t argument in dma_alloc_attrs sooner, and only
>>>> allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent.
>>> How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically
>>> allocate huge pages (GFP_TRANSHUGE can result in unacceptably long
>>> delays with memory pressure).
>> Well, that is exactly what I don't want drivers to do - same for
>> __GFP_COMP in some drm code.  This very much assumes the page allocator
>> is used to back dma allocations, which very often it actually isn't, and
>> any use of magic gfp flags creates a tight coupling of consumers with a
>> specific implementation.
>>
>> In general I can't think of a good reason not to actually use
>> GFP_TRANSHUGE_LIGHT by default in the dma allocator unless
>> DMA_ATTR_ALLOC_SINGLE_PAGES is set.  Can you prepare a patch for that?
> I'm afraid I'll have to leave that to somebody else.

Coming back to this topic once more, sorry for the delay but busy as 
usual :)

What exactly do you mean with "dma allocator" here? The TTM allocator 
using the dma_alloc_coherent calls? Or the swiotlb implementation of the 
calls?

Christian.

Re: [PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Christoph Hellwig]

On Tue, May 22, 2018 at 03:13:58PM +0200, Christian König wrote:
> Am 02.05.2018 um 18:59 schrieb Michel Dänzer:
>> On 2018-05-02 06:21 PM, Christoph Hellwig wrote:
>>> On Wed, May 02, 2018 at 04:31:09PM +0200, Michel Dänzer wrote:
>>>>> No.  __GFP_NOWARN (and gfp_t flags in general) are the wrong interface
>>>>> for dma allocations and just cause problems.  I actually plan to
>>>>> get rid of the gfp_t argument in dma_alloc_attrs sooner, and only
>>>>> allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent.
>>>> How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically
>>>> allocate huge pages (GFP_TRANSHUGE can result in unacceptably long
>>>> delays with memory pressure).
>>> Well, that is exactly what I don't want drivers to do - same for
>>> __GFP_COMP in some drm code.  This very much assumes the page allocator
>>> is used to back dma allocations, which very often it actually isn't, and
>>> any use of magic gfp flags creates a tight coupling of consumers with a
>>> specific implementation.
>>>
>>> In general I can't think of a good reason not to actually use
>>> GFP_TRANSHUGE_LIGHT by default in the dma allocator unless
>>> DMA_ATTR_ALLOC_SINGLE_PAGES is set.  Can you prepare a patch for that?
>> I'm afraid I'll have to leave that to somebody else.
>
> Coming back to this topic once more, sorry for the delay but busy as usual 
> :)
>
> What exactly do you mean with "dma allocator" here? The TTM allocator using 
> the dma_alloc_coherent calls? Or the swiotlb implementation of the calls?

dma allocatr in this case: backends for dma_alloc_coherent/
dma_alloc_attrs.  Most importantly dma_direct_alloc.

But while we're at it I can't actually see any GFP_TRANSHUGE_LIGHT
usage in TTM, just plain old GFP_TRANSHUGE.

Re: [PATCH] swiotlb: Fix inversed DMA_ATTR_NO_WARN test

[Michel Dänzer]

On 2018-05-25 10:41 AM, Christoph Hellwig wrote:
> On Tue, May 22, 2018 at 03:13:58PM +0200, Christian König wrote:
>> Am 02.05.2018 um 18:59 schrieb Michel Dänzer:
>>> On 2018-05-02 06:21 PM, Christoph Hellwig wrote:
>>>> On Wed, May 02, 2018 at 04:31:09PM +0200, Michel Dänzer wrote:
>>>>>> No.  __GFP_NOWARN (and gfp_t flags in general) are the wrong interface
>>>>>> for dma allocations and just cause problems.  I actually plan to
>>>>>> get rid of the gfp_t argument in dma_alloc_attrs sooner, and only
>>>>>> allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent.
>>>>> How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically
>>>>> allocate huge pages (GFP_TRANSHUGE can result in unacceptably long
>>>>> delays with memory pressure).
>>>> Well, that is exactly what I don't want drivers to do - same for
>>>> __GFP_COMP in some drm code.  This very much assumes the page allocator
>>>> is used to back dma allocations, which very often it actually isn't, and
>>>> any use of magic gfp flags creates a tight coupling of consumers with a
>>>> specific implementation.
>>>>
>>>> In general I can't think of a good reason not to actually use
>>>> GFP_TRANSHUGE_LIGHT by default in the dma allocator unless
>>>> DMA_ATTR_ALLOC_SINGLE_PAGES is set.  Can you prepare a patch for that?
>>> I'm afraid I'll have to leave that to somebody else.
>>
>> Coming back to this topic once more, sorry for the delay but busy as usual 
>> :)
>>
>> What exactly do you mean with "dma allocator" here? The TTM allocator using 
>> the dma_alloc_coherent calls? Or the swiotlb implementation of the calls?
> 
> dma allocatr in this case: backends for dma_alloc_coherent/
> dma_alloc_attrs.  Most importantly dma_direct_alloc.
> 
> But while we're at it I can't actually see any GFP_TRANSHUGE_LIGHT
> usage in TTM, just plain old GFP_TRANSHUGE.

See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=da291320baec914f0bb4e65a9dccb86bd6c728f2
.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer