From: Michel Dänzer <[email protected]>
The result was printing the warning only when we were explicitly asked
not to.
Cc: [email protected]
Fixes: 0176adb004065d6815a8e67946752df4cd947c5b "swiotlb: refactor
coherent buffer allocation"
Signed-off-by: Michel Dänzer <[email protected]>
---
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
Am 01.05.2018 um 15:24 schrieb Michel Dänzer:
> From: Michel Dänzer <[email protected]>
>
> The result was printing the warning only when we were explicitly asked
> not to.
>
> Cc: [email protected]
> Fixes: 0176adb004065d6815a8e67946752df4cd947c5b "swiotlb: refactor
> coherent buffer allocation"
> Signed-off-by: Michel Dänzer <[email protected]>
Good catch, looked at the code multiple times and haven't seen that
myself :)
Reviewed-by: Christian König <[email protected]>.
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);
On Wed, May 2, 2018 at 11:49 AM, Christian König
<[email protected]> wrote:
> Am 01.05.2018 um 15:24 schrieb Michel Dänzer:
>>
>> From: Michel Dänzer <[email protected]>
>>
>> The result was printing the warning only when we were explicitly asked
>> not to.
>>
>> Cc: [email protected]
>> Fixes: 0176adb004065d6815a8e67946752df4cd947c5b "swiotlb: refactor
>> coherent buffer allocation"
>> Signed-off-by: Michel Dänzer <[email protected]>
>
>
> Good catch, looked at the code multiple times and haven't seen that myself
> :)
>
> Reviewed-by: Christian König <[email protected]>.
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 <[email protected]>
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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
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.
On Tue, May 01, 2018 at 03:24:11PM +0200, Michel D?nzer wrote:
> From: Michel D?nzer <[email protected]>
>
> The result was printing the warning only when we were explicitly asked
> not to.
Thanks, applied to the dma-mapping tree for 4.17.
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
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?
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
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.
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.
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