* [PATCH] dma-buf: system_heap: do not warn for costly allocation
@ 2021-02-10 16:26 Minchan Kim
2021-02-10 17:24 ` Suren Baghdasaryan
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Minchan Kim @ 2021-02-10 16:26 UTC (permalink / raw)
To: sumit.semwal, john.stultz; +Cc: LKML, surenb, hridya, joaodias, Minchan Kim
Linux VM is not hard to support PAGE_ALLOC_COSTLY_ODER allocation
so normally expects driver passes __GFP_NOWARN in that case
if they has fallback options.
system_heap in dmabuf is the case so do not flood into demsg
with the warning for recording more precious information logs.
(below is ION warning example I got but dmabuf system heap is
nothing different).
[ 1233.911533][ T460] warn_alloc: 11 callbacks suppressed
[ 1233.911539][ T460] allocator@2.0-s: page allocation failure: order:4, mode:0x140dc2(GFP_HIGHUSER|__GFP_COMP|__GFP_ZERO), nodemask=(null),cpuset=/,mems_allowed=0
[ 1233.926235][ T460] Call trace:
[ 1233.929370][ T460] dump_backtrace+0x0/0x1d8
[ 1233.933704][ T460] show_stack+0x18/0x24
[ 1233.937701][ T460] dump_stack+0xc0/0x140
[ 1233.941783][ T460] warn_alloc+0xf4/0x148
[ 1233.945862][ T460] __alloc_pages_slowpath+0x9fc/0xa10
[ 1233.951101][ T460] __alloc_pages_nodemask+0x278/0x2c0
[ 1233.956285][ T460] ion_page_pool_alloc+0xd8/0x100
[ 1233.961144][ T460] ion_system_heap_allocate+0xbc/0x2f0
[ 1233.966440][ T460] ion_buffer_create+0x68/0x274
[ 1233.971130][ T460] ion_buffer_alloc+0x8c/0x110
[ 1233.975733][ T460] ion_dmabuf_alloc+0x44/0xe8
[ 1233.980248][ T460] ion_ioctl+0x100/0x320
[ 1233.984332][ T460] __arm64_sys_ioctl+0x90/0xc8
[ 1233.988934][ T460] el0_svc_common+0x9c/0x168
[ 1233.993360][ T460] do_el0_svc+0x1c/0x28
[ 1233.997358][ T460] el0_sync_handler+0xd8/0x250
[ 1234.001989][ T460] el0_sync+0x148/0x180
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
drivers/dma-buf/heaps/system_heap.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 29e49ac17251..33c25a5e06f9 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -40,7 +40,7 @@ struct dma_heap_attachment {
bool mapped;
};
-#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \
+#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO \
| __GFP_NORETRY) & ~__GFP_RECLAIM) \
| __GFP_COMP)
#define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP)
@@ -315,6 +315,7 @@ static struct page *alloc_largest_available(unsigned long size,
unsigned int max_order)
{
struct page *page;
+ unsigned long gfp_flags;
int i;
for (i = 0; i < NUM_ORDERS; i++) {
@@ -323,7 +324,11 @@ static struct page *alloc_largest_available(unsigned long size,
if (max_order < orders[i])
continue;
- page = alloc_pages(order_flags[i], orders[i]);
+ gfp_flags = order_flags[i];
+ if (orders[i] > PAGE_ALLOC_COSTLY_ORDER)
+ gfp_flags |= __GFP_NOWARN;
+
+ page = alloc_pages(gfp_flags, orders[i]);
if (!page)
continue;
return page;
--
2.30.0.478.g8a0d178c01-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation 2021-02-10 16:26 [PATCH] dma-buf: system_heap: do not warn for costly allocation Minchan Kim @ 2021-02-10 17:24 ` Suren Baghdasaryan 2021-02-10 17:41 ` Minchan Kim 2021-02-10 17:32 ` John Stultz 2021-02-10 21:12 ` kernel test robot 2 siblings, 1 reply; 10+ messages in thread From: Suren Baghdasaryan @ 2021-02-10 17:24 UTC (permalink / raw) To: Minchan Kim; +Cc: Sumit Semwal, John Stultz, LKML, Hridya Valsaraju, John Dias The code looks fine to me. Description needs a bit polishing :) On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <minchan@kernel.org> wrote: > > Linux VM is not hard to support PAGE_ALLOC_COSTLY_ODER allocation > so normally expects driver passes __GFP_NOWARN in that case > if they has fallback options. > > system_heap in dmabuf is the case so do not flood into demsg > with the warning for recording more precious information logs. > (below is ION warning example I got but dmabuf system heap is > nothing different). Suggestion: Dmabuf system_heap allocation logic starts with the highest necessary allocation order before falling back to lower orders. The requested order can be higher than PAGE_ALLOC_COSTLY_ODER and failures to allocate will flood dmesg with warnings. Such high-order allocations are not unexpected and are handled by the system_heap's allocation fallback mechanism. Prevent these warnings when allocating higher than PAGE_ALLOC_COSTLY_ODER pages using __GFP_NOWARN flag. Below is ION warning example I got but dmabuf system heap is nothing different: > > [ 1233.911533][ T460] warn_alloc: 11 callbacks suppressed > [ 1233.911539][ T460] allocator@2.0-s: page allocation failure: order:4, mode:0x140dc2(GFP_HIGHUSER|__GFP_COMP|__GFP_ZERO), nodemask=(null),cpuset=/,mems_allowed=0 > [ 1233.926235][ T460] Call trace: > [ 1233.929370][ T460] dump_backtrace+0x0/0x1d8 > [ 1233.933704][ T460] show_stack+0x18/0x24 > [ 1233.937701][ T460] dump_stack+0xc0/0x140 > [ 1233.941783][ T460] warn_alloc+0xf4/0x148 > [ 1233.945862][ T460] __alloc_pages_slowpath+0x9fc/0xa10 > [ 1233.951101][ T460] __alloc_pages_nodemask+0x278/0x2c0 > [ 1233.956285][ T460] ion_page_pool_alloc+0xd8/0x100 > [ 1233.961144][ T460] ion_system_heap_allocate+0xbc/0x2f0 > [ 1233.966440][ T460] ion_buffer_create+0x68/0x274 > [ 1233.971130][ T460] ion_buffer_alloc+0x8c/0x110 > [ 1233.975733][ T460] ion_dmabuf_alloc+0x44/0xe8 > [ 1233.980248][ T460] ion_ioctl+0x100/0x320 > [ 1233.984332][ T460] __arm64_sys_ioctl+0x90/0xc8 > [ 1233.988934][ T460] el0_svc_common+0x9c/0x168 > [ 1233.993360][ T460] do_el0_svc+0x1c/0x28 > [ 1233.997358][ T460] el0_sync_handler+0xd8/0x250 > [ 1234.001989][ T460] el0_sync+0x148/0x180 > > Signed-off-by: Minchan Kim <minchan@kernel.org> > --- > drivers/dma-buf/heaps/system_heap.c | 9 +++++++-- > 1 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 29e49ac17251..33c25a5e06f9 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -40,7 +40,7 @@ struct dma_heap_attachment { > bool mapped; > }; > > -#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \ > +#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO \ > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > | __GFP_COMP) > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > @@ -315,6 +315,7 @@ static struct page *alloc_largest_available(unsigned long size, > unsigned int max_order) > { > struct page *page; > + unsigned long gfp_flags; > int i; > > for (i = 0; i < NUM_ORDERS; i++) { > @@ -323,7 +324,11 @@ static struct page *alloc_largest_available(unsigned long size, > if (max_order < orders[i]) > continue; > > - page = alloc_pages(order_flags[i], orders[i]); > + gfp_flags = order_flags[i]; > + if (orders[i] > PAGE_ALLOC_COSTLY_ORDER) > + gfp_flags |= __GFP_NOWARN; > + > + page = alloc_pages(gfp_flags, orders[i]); > if (!page) > continue; > return page; > -- > 2.30.0.478.g8a0d178c01-goog > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation 2021-02-10 17:24 ` Suren Baghdasaryan @ 2021-02-10 17:41 ` Minchan Kim 0 siblings, 0 replies; 10+ messages in thread From: Minchan Kim @ 2021-02-10 17:41 UTC (permalink / raw) To: Suren Baghdasaryan Cc: Sumit Semwal, John Stultz, LKML, Hridya Valsaraju, John Dias On Wed, Feb 10, 2021 at 09:24:52AM -0800, Suren Baghdasaryan wrote: > The code looks fine to me. Description needs a bit polishing :) > > On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <minchan@kernel.org> wrote: > > > > Linux VM is not hard to support PAGE_ALLOC_COSTLY_ODER allocation > > so normally expects driver passes __GFP_NOWARN in that case > > if they has fallback options. > > > > system_heap in dmabuf is the case so do not flood into demsg > > with the warning for recording more precious information logs. > > (below is ION warning example I got but dmabuf system heap is > > nothing different). > > Suggestion: > Dmabuf system_heap allocation logic starts with the highest necessary > allocation order before falling back to lower orders. The requested > order can be higher than PAGE_ALLOC_COSTLY_ODER and failures to > allocate will flood dmesg with warnings. Such high-order allocations > are not unexpected and are handled by the system_heap's allocation > fallback mechanism. > Prevent these warnings when allocating higher than > PAGE_ALLOC_COSTLY_ODER pages using __GFP_NOWARN flag. > > Below is ION warning example I got but dmabuf system heap is nothing different: I will take it. Thanks, Suren! ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation 2021-02-10 16:26 [PATCH] dma-buf: system_heap: do not warn for costly allocation Minchan Kim 2021-02-10 17:24 ` Suren Baghdasaryan @ 2021-02-10 17:32 ` John Stultz 2021-02-10 17:48 ` Minchan Kim 2021-02-10 21:12 ` kernel test robot 2 siblings, 1 reply; 10+ messages in thread From: John Stultz @ 2021-02-10 17:32 UTC (permalink / raw) To: Minchan Kim Cc: Sumit Semwal, LKML, Suren Baghdasaryan, Hridya Valsaraju, John Dias On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <minchan@kernel.org> wrote: > > Linux VM is not hard to support PAGE_ALLOC_COSTLY_ODER allocation > so normally expects driver passes __GFP_NOWARN in that case > if they has fallback options. > > system_heap in dmabuf is the case so do not flood into demsg > with the warning for recording more precious information logs. > (below is ION warning example I got but dmabuf system heap is > nothing different). > > [ 1233.911533][ T460] warn_alloc: 11 callbacks suppressed > [ 1233.911539][ T460] allocator@2.0-s: page allocation failure: order:4, mode:0x140dc2(GFP_HIGHUSER|__GFP_COMP|__GFP_ZERO), nodemask=(null),cpuset=/,mems_allowed=0 > [ 1233.926235][ T460] Call trace: > [ 1233.929370][ T460] dump_backtrace+0x0/0x1d8 > [ 1233.933704][ T460] show_stack+0x18/0x24 > [ 1233.937701][ T460] dump_stack+0xc0/0x140 > [ 1233.941783][ T460] warn_alloc+0xf4/0x148 > [ 1233.945862][ T460] __alloc_pages_slowpath+0x9fc/0xa10 > [ 1233.951101][ T460] __alloc_pages_nodemask+0x278/0x2c0 > [ 1233.956285][ T460] ion_page_pool_alloc+0xd8/0x100 > [ 1233.961144][ T460] ion_system_heap_allocate+0xbc/0x2f0 > [ 1233.966440][ T460] ion_buffer_create+0x68/0x274 > [ 1233.971130][ T460] ion_buffer_alloc+0x8c/0x110 > [ 1233.975733][ T460] ion_dmabuf_alloc+0x44/0xe8 > [ 1233.980248][ T460] ion_ioctl+0x100/0x320 > [ 1233.984332][ T460] __arm64_sys_ioctl+0x90/0xc8 > [ 1233.988934][ T460] el0_svc_common+0x9c/0x168 > [ 1233.993360][ T460] do_el0_svc+0x1c/0x28 > [ 1233.997358][ T460] el0_sync_handler+0xd8/0x250 > [ 1234.001989][ T460] el0_sync+0x148/0x180 > > Signed-off-by: Minchan Kim <minchan@kernel.org> > --- > drivers/dma-buf/heaps/system_heap.c | 9 +++++++-- > 1 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 29e49ac17251..33c25a5e06f9 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -40,7 +40,7 @@ struct dma_heap_attachment { > bool mapped; > }; > > -#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \ > +#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO \ > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > | __GFP_COMP) > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > @@ -315,6 +315,7 @@ static struct page *alloc_largest_available(unsigned long size, > unsigned int max_order) > { > struct page *page; > + unsigned long gfp_flags; > int i; > > for (i = 0; i < NUM_ORDERS; i++) { > @@ -323,7 +324,11 @@ static struct page *alloc_largest_available(unsigned long size, > if (max_order < orders[i]) > continue; > > - page = alloc_pages(order_flags[i], orders[i]); > + gfp_flags = order_flags[i]; > + if (orders[i] > PAGE_ALLOC_COSTLY_ORDER) > + gfp_flags |= __GFP_NOWARN; > + > + page = alloc_pages(gfp_flags, orders[i]); Would it be cleaner to just set up the flags properly in the order_flags array? I'm not sure I understand why your patch does it dynamically? thanks -john ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation 2021-02-10 17:32 ` John Stultz @ 2021-02-10 17:48 ` Minchan Kim 2021-02-10 21:40 ` John Stultz 0 siblings, 1 reply; 10+ messages in thread From: Minchan Kim @ 2021-02-10 17:48 UTC (permalink / raw) To: John Stultz Cc: Sumit Semwal, LKML, Suren Baghdasaryan, Hridya Valsaraju, John Dias On Wed, Feb 10, 2021 at 09:32:09AM -0800, John Stultz wrote: > On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <minchan@kernel.org> wrote: > > > > Linux VM is not hard to support PAGE_ALLOC_COSTLY_ODER allocation > > so normally expects driver passes __GFP_NOWARN in that case > > if they has fallback options. > > > > system_heap in dmabuf is the case so do not flood into demsg > > with the warning for recording more precious information logs. > > (below is ION warning example I got but dmabuf system heap is > > nothing different). > > > > [ 1233.911533][ T460] warn_alloc: 11 callbacks suppressed > > [ 1233.911539][ T460] allocator@2.0-s: page allocation failure: order:4, mode:0x140dc2(GFP_HIGHUSER|__GFP_COMP|__GFP_ZERO), nodemask=(null),cpuset=/,mems_allowed=0 > > [ 1233.926235][ T460] Call trace: > > [ 1233.929370][ T460] dump_backtrace+0x0/0x1d8 > > [ 1233.933704][ T460] show_stack+0x18/0x24 > > [ 1233.937701][ T460] dump_stack+0xc0/0x140 > > [ 1233.941783][ T460] warn_alloc+0xf4/0x148 > > [ 1233.945862][ T460] __alloc_pages_slowpath+0x9fc/0xa10 > > [ 1233.951101][ T460] __alloc_pages_nodemask+0x278/0x2c0 > > [ 1233.956285][ T460] ion_page_pool_alloc+0xd8/0x100 > > [ 1233.961144][ T460] ion_system_heap_allocate+0xbc/0x2f0 > > [ 1233.966440][ T460] ion_buffer_create+0x68/0x274 > > [ 1233.971130][ T460] ion_buffer_alloc+0x8c/0x110 > > [ 1233.975733][ T460] ion_dmabuf_alloc+0x44/0xe8 > > [ 1233.980248][ T460] ion_ioctl+0x100/0x320 > > [ 1233.984332][ T460] __arm64_sys_ioctl+0x90/0xc8 > > [ 1233.988934][ T460] el0_svc_common+0x9c/0x168 > > [ 1233.993360][ T460] do_el0_svc+0x1c/0x28 > > [ 1233.997358][ T460] el0_sync_handler+0xd8/0x250 > > [ 1234.001989][ T460] el0_sync+0x148/0x180 > > > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > --- > > drivers/dma-buf/heaps/system_heap.c | 9 +++++++-- > > 1 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > > index 29e49ac17251..33c25a5e06f9 100644 > > --- a/drivers/dma-buf/heaps/system_heap.c > > +++ b/drivers/dma-buf/heaps/system_heap.c > > @@ -40,7 +40,7 @@ struct dma_heap_attachment { > > bool mapped; > > }; > > > > -#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \ > > +#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO \ > > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > > | __GFP_COMP) > > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > > @@ -315,6 +315,7 @@ static struct page *alloc_largest_available(unsigned long size, > > unsigned int max_order) > > { > > struct page *page; > > + unsigned long gfp_flags; > > int i; > > > > for (i = 0; i < NUM_ORDERS; i++) { > > @@ -323,7 +324,11 @@ static struct page *alloc_largest_available(unsigned long size, > > if (max_order < orders[i]) > > continue; > > > > - page = alloc_pages(order_flags[i], orders[i]); > > + gfp_flags = order_flags[i]; > > + if (orders[i] > PAGE_ALLOC_COSTLY_ORDER) > > + gfp_flags |= __GFP_NOWARN; > > + > > + page = alloc_pages(gfp_flags, orders[i]); > > Would it be cleaner to just set up the flags properly in the > order_flags array? I'm not sure I understand why your patch does it > dynamically? That's exactly I had in my branch for aosp fix but I wanted to hear it explicitly from dmabuf maintainer since I was worried chaninging order-4 allocation behavior, especially, __GFP_NORETRY and &~__GFP_RECLAIM. (It will make allocation failure easier than old and that's not thing my patch is addressing). If you want this, I am happy to change it. Shall I? diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 29e49ac17251..865ec847013d 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -44,7 +44,7 @@ struct dma_heap_attachment { | __GFP_NORETRY) & ~__GFP_RECLAIM) \ | __GFP_COMP) #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) -static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP}; +static gfp_t order_flags[] = {HIGH_ORDER_GFP, HIGH_ORDER_GFP, LOW_ORDER_GFP}; /* * The selection of the orders used for allocation (1MB, 64K, 4K) is designed * to match with the sizes often found in IOMMUs. Using order 4 pages instead ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation 2021-02-10 17:48 ` Minchan Kim @ 2021-02-10 21:40 ` John Stultz 2021-02-10 23:17 ` Minchan Kim 0 siblings, 1 reply; 10+ messages in thread From: John Stultz @ 2021-02-10 21:40 UTC (permalink / raw) To: Minchan Kim Cc: Sumit Semwal, LKML, Suren Baghdasaryan, Hridya Valsaraju, John Dias On Wed, Feb 10, 2021 at 9:48 AM Minchan Kim <minchan@kernel.org> wrote: > > On Wed, Feb 10, 2021 at 09:32:09AM -0800, John Stultz wrote: > > On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <minchan@kernel.org> wrote: > > > > > > Linux VM is not hard to support PAGE_ALLOC_COSTLY_ODER allocation > > > so normally expects driver passes __GFP_NOWARN in that case > > > if they has fallback options. > > > > > > system_heap in dmabuf is the case so do not flood into demsg > > > with the warning for recording more precious information logs. > > > (below is ION warning example I got but dmabuf system heap is > > > nothing different). > > > > > > [ 1233.911533][ T460] warn_alloc: 11 callbacks suppressed > > > [ 1233.911539][ T460] allocator@2.0-s: page allocation failure: order:4, mode:0x140dc2(GFP_HIGHUSER|__GFP_COMP|__GFP_ZERO), nodemask=(null),cpuset=/,mems_allowed=0 > > > [ 1233.926235][ T460] Call trace: > > > [ 1233.929370][ T460] dump_backtrace+0x0/0x1d8 > > > [ 1233.933704][ T460] show_stack+0x18/0x24 > > > [ 1233.937701][ T460] dump_stack+0xc0/0x140 > > > [ 1233.941783][ T460] warn_alloc+0xf4/0x148 > > > [ 1233.945862][ T460] __alloc_pages_slowpath+0x9fc/0xa10 > > > [ 1233.951101][ T460] __alloc_pages_nodemask+0x278/0x2c0 > > > [ 1233.956285][ T460] ion_page_pool_alloc+0xd8/0x100 > > > [ 1233.961144][ T460] ion_system_heap_allocate+0xbc/0x2f0 > > > [ 1233.966440][ T460] ion_buffer_create+0x68/0x274 > > > [ 1233.971130][ T460] ion_buffer_alloc+0x8c/0x110 > > > [ 1233.975733][ T460] ion_dmabuf_alloc+0x44/0xe8 > > > [ 1233.980248][ T460] ion_ioctl+0x100/0x320 > > > [ 1233.984332][ T460] __arm64_sys_ioctl+0x90/0xc8 > > > [ 1233.988934][ T460] el0_svc_common+0x9c/0x168 > > > [ 1233.993360][ T460] do_el0_svc+0x1c/0x28 > > > [ 1233.997358][ T460] el0_sync_handler+0xd8/0x250 > > > [ 1234.001989][ T460] el0_sync+0x148/0x180 > > > > > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > > --- > > > drivers/dma-buf/heaps/system_heap.c | 9 +++++++-- > > > 1 files changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > > > index 29e49ac17251..33c25a5e06f9 100644 > > > --- a/drivers/dma-buf/heaps/system_heap.c > > > +++ b/drivers/dma-buf/heaps/system_heap.c > > > @@ -40,7 +40,7 @@ struct dma_heap_attachment { > > > bool mapped; > > > }; > > > > > > -#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \ > > > +#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO \ > > > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > > > | __GFP_COMP) > > > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > > > @@ -315,6 +315,7 @@ static struct page *alloc_largest_available(unsigned long size, > > > unsigned int max_order) > > > { > > > struct page *page; > > > + unsigned long gfp_flags; > > > int i; > > > > > > for (i = 0; i < NUM_ORDERS; i++) { > > > @@ -323,7 +324,11 @@ static struct page *alloc_largest_available(unsigned long size, > > > if (max_order < orders[i]) > > > continue; > > > > > > - page = alloc_pages(order_flags[i], orders[i]); > > > + gfp_flags = order_flags[i]; > > > + if (orders[i] > PAGE_ALLOC_COSTLY_ORDER) > > > + gfp_flags |= __GFP_NOWARN; > > > + > > > + page = alloc_pages(gfp_flags, orders[i]); > > > > Would it be cleaner to just set up the flags properly in the > > order_flags array? I'm not sure I understand why your patch does it > > dynamically? > > That's exactly I had in my branch for aosp fix but I wanted to > hear it explicitly from dmabuf maintainer since I was worried > chaninging order-4 allocation behavior, especially, > __GFP_NORETRY and &~__GFP_RECLAIM. > (It will make allocation failure easier than old and that's not > thing my patch is addressing). Yea. I might stick to changing just the __GFP_NOWARN. > If you want this, I am happy to change it. Shall I? > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 29e49ac17251..865ec847013d 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -44,7 +44,7 @@ struct dma_heap_attachment { > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > | __GFP_COMP) > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > -static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP}; > +static gfp_t order_flags[] = {HIGH_ORDER_GFP, HIGH_ORDER_GFP, LOW_ORDER_GFP}; Maybe can you define a MID_ORDER_GFP as LOW_ORDER | __GFP_NOWARN (along with a comment in the code as to why) instead ? That avoids introducing any subtle behavioral change unintentionally. thanks -john ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation 2021-02-10 21:40 ` John Stultz @ 2021-02-10 23:17 ` Minchan Kim 2021-02-11 2:14 ` John Stultz 0 siblings, 1 reply; 10+ messages in thread From: Minchan Kim @ 2021-02-10 23:17 UTC (permalink / raw) To: John Stultz Cc: Sumit Semwal, LKML, Suren Baghdasaryan, Hridya Valsaraju, John Dias On Wed, Feb 10, 2021 at 01:40:02PM -0800, John Stultz wrote: > On Wed, Feb 10, 2021 at 9:48 AM Minchan Kim <minchan@kernel.org> wrote: > > > > On Wed, Feb 10, 2021 at 09:32:09AM -0800, John Stultz wrote: > > > On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <minchan@kernel.org> wrote: > > > > > > > > Linux VM is not hard to support PAGE_ALLOC_COSTLY_ODER allocation > > > > so normally expects driver passes __GFP_NOWARN in that case > > > > if they has fallback options. > > > > > > > > system_heap in dmabuf is the case so do not flood into demsg > > > > with the warning for recording more precious information logs. > > > > (below is ION warning example I got but dmabuf system heap is > > > > nothing different). > > > > > > > > [ 1233.911533][ T460] warn_alloc: 11 callbacks suppressed > > > > [ 1233.911539][ T460] allocator@2.0-s: page allocation failure: order:4, mode:0x140dc2(GFP_HIGHUSER|__GFP_COMP|__GFP_ZERO), nodemask=(null),cpuset=/,mems_allowed=0 > > > > [ 1233.926235][ T460] Call trace: > > > > [ 1233.929370][ T460] dump_backtrace+0x0/0x1d8 > > > > [ 1233.933704][ T460] show_stack+0x18/0x24 > > > > [ 1233.937701][ T460] dump_stack+0xc0/0x140 > > > > [ 1233.941783][ T460] warn_alloc+0xf4/0x148 > > > > [ 1233.945862][ T460] __alloc_pages_slowpath+0x9fc/0xa10 > > > > [ 1233.951101][ T460] __alloc_pages_nodemask+0x278/0x2c0 > > > > [ 1233.956285][ T460] ion_page_pool_alloc+0xd8/0x100 > > > > [ 1233.961144][ T460] ion_system_heap_allocate+0xbc/0x2f0 > > > > [ 1233.966440][ T460] ion_buffer_create+0x68/0x274 > > > > [ 1233.971130][ T460] ion_buffer_alloc+0x8c/0x110 > > > > [ 1233.975733][ T460] ion_dmabuf_alloc+0x44/0xe8 > > > > [ 1233.980248][ T460] ion_ioctl+0x100/0x320 > > > > [ 1233.984332][ T460] __arm64_sys_ioctl+0x90/0xc8 > > > > [ 1233.988934][ T460] el0_svc_common+0x9c/0x168 > > > > [ 1233.993360][ T460] do_el0_svc+0x1c/0x28 > > > > [ 1233.997358][ T460] el0_sync_handler+0xd8/0x250 > > > > [ 1234.001989][ T460] el0_sync+0x148/0x180 > > > > > > > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > > > --- > > > > drivers/dma-buf/heaps/system_heap.c | 9 +++++++-- > > > > 1 files changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > > > > index 29e49ac17251..33c25a5e06f9 100644 > > > > --- a/drivers/dma-buf/heaps/system_heap.c > > > > +++ b/drivers/dma-buf/heaps/system_heap.c > > > > @@ -40,7 +40,7 @@ struct dma_heap_attachment { > > > > bool mapped; > > > > }; > > > > > > > > -#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \ > > > > +#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO \ > > > > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > > > > | __GFP_COMP) > > > > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > > > > @@ -315,6 +315,7 @@ static struct page *alloc_largest_available(unsigned long size, > > > > unsigned int max_order) > > > > { > > > > struct page *page; > > > > + unsigned long gfp_flags; > > > > int i; > > > > > > > > for (i = 0; i < NUM_ORDERS; i++) { > > > > @@ -323,7 +324,11 @@ static struct page *alloc_largest_available(unsigned long size, > > > > if (max_order < orders[i]) > > > > continue; > > > > > > > > - page = alloc_pages(order_flags[i], orders[i]); > > > > + gfp_flags = order_flags[i]; > > > > + if (orders[i] > PAGE_ALLOC_COSTLY_ORDER) > > > > + gfp_flags |= __GFP_NOWARN; > > > > + > > > > + page = alloc_pages(gfp_flags, orders[i]); > > > > > > Would it be cleaner to just set up the flags properly in the > > > order_flags array? I'm not sure I understand why your patch does it > > > dynamically? > > > > That's exactly I had in my branch for aosp fix but I wanted to > > hear it explicitly from dmabuf maintainer since I was worried > > chaninging order-4 allocation behavior, especially, > > __GFP_NORETRY and &~__GFP_RECLAIM. > > (It will make allocation failure easier than old and that's not > > thing my patch is addressing). > > Yea. I might stick to changing just the __GFP_NOWARN. > > > If you want this, I am happy to change it. Shall I? > > > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > > index 29e49ac17251..865ec847013d 100644 > > --- a/drivers/dma-buf/heaps/system_heap.c > > +++ b/drivers/dma-buf/heaps/system_heap.c > > @@ -44,7 +44,7 @@ struct dma_heap_attachment { > > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > > | __GFP_COMP) > > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > > -static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP}; > > +static gfp_t order_flags[] = {HIGH_ORDER_GFP, HIGH_ORDER_GFP, LOW_ORDER_GFP}; > > Maybe can you define a MID_ORDER_GFP as LOW_ORDER | __GFP_NOWARN > (along with a comment in the code as to why) instead ? > > That avoids introducing any subtle behavioral change unintentionally. How about this one? Feel free to suggest better wording. diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 29e49ac17251..6e17ff06331e 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -44,7 +44,13 @@ struct dma_heap_attachment { | __GFP_NORETRY) & ~__GFP_RECLAIM) \ | __GFP_COMP) #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) -static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP}; +/* + * order-4 is PAGE_ALLOC_COSTLY_ORDER which is order allocator could fail + * easier than lower orders. Since we have fallback order-0 allocation, + * do not add warn. + */ +#define MID_ORDER_GFP (LOW_ORDER_GFP | __GFP_NOWARN) +static gfp_t order_flags[] = {HIGH_ORDER_GFP, MID_ORDER_GFP, LOW_ORDER_GFP}; /* * The selection of the orders used for allocation (1MB, 64K, 4K) is designed * to match with the sizes often found in IOMMUs. Using order 4 pages instead ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation 2021-02-10 23:17 ` Minchan Kim @ 2021-02-11 2:14 ` John Stultz 2021-02-11 2:30 ` Minchan Kim 0 siblings, 1 reply; 10+ messages in thread From: John Stultz @ 2021-02-11 2:14 UTC (permalink / raw) To: Minchan Kim Cc: Sumit Semwal, LKML, Suren Baghdasaryan, Hridya Valsaraju, John Dias On Wed, Feb 10, 2021 at 3:17 PM Minchan Kim <minchan@kernel.org> wrote: > > On Wed, Feb 10, 2021 at 01:40:02PM -0800, John Stultz wrote: > > On Wed, Feb 10, 2021 at 9:48 AM Minchan Kim <minchan@kernel.org> wrote: > > > > > > On Wed, Feb 10, 2021 at 09:32:09AM -0800, John Stultz wrote: > > > > On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <minchan@kernel.org> wrote: > > > > > > > > > > Linux VM is not hard to support PAGE_ALLOC_COSTLY_ODER allocation > > > > > so normally expects driver passes __GFP_NOWARN in that case > > > > > if they has fallback options. > > > > > > > > > > system_heap in dmabuf is the case so do not flood into demsg > > > > > with the warning for recording more precious information logs. > > > > > (below is ION warning example I got but dmabuf system heap is > > > > > nothing different). > > > > > > > > > > [ 1233.911533][ T460] warn_alloc: 11 callbacks suppressed > > > > > [ 1233.911539][ T460] allocator@2.0-s: page allocation failure: order:4, mode:0x140dc2(GFP_HIGHUSER|__GFP_COMP|__GFP_ZERO), nodemask=(null),cpuset=/,mems_allowed=0 > > > > > [ 1233.926235][ T460] Call trace: > > > > > [ 1233.929370][ T460] dump_backtrace+0x0/0x1d8 > > > > > [ 1233.933704][ T460] show_stack+0x18/0x24 > > > > > [ 1233.937701][ T460] dump_stack+0xc0/0x140 > > > > > [ 1233.941783][ T460] warn_alloc+0xf4/0x148 > > > > > [ 1233.945862][ T460] __alloc_pages_slowpath+0x9fc/0xa10 > > > > > [ 1233.951101][ T460] __alloc_pages_nodemask+0x278/0x2c0 > > > > > [ 1233.956285][ T460] ion_page_pool_alloc+0xd8/0x100 > > > > > [ 1233.961144][ T460] ion_system_heap_allocate+0xbc/0x2f0 > > > > > [ 1233.966440][ T460] ion_buffer_create+0x68/0x274 > > > > > [ 1233.971130][ T460] ion_buffer_alloc+0x8c/0x110 > > > > > [ 1233.975733][ T460] ion_dmabuf_alloc+0x44/0xe8 > > > > > [ 1233.980248][ T460] ion_ioctl+0x100/0x320 > > > > > [ 1233.984332][ T460] __arm64_sys_ioctl+0x90/0xc8 > > > > > [ 1233.988934][ T460] el0_svc_common+0x9c/0x168 > > > > > [ 1233.993360][ T460] do_el0_svc+0x1c/0x28 > > > > > [ 1233.997358][ T460] el0_sync_handler+0xd8/0x250 > > > > > [ 1234.001989][ T460] el0_sync+0x148/0x180 > > > > > > > > > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > > > > --- > > > > > drivers/dma-buf/heaps/system_heap.c | 9 +++++++-- > > > > > 1 files changed, 7 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > > > > > index 29e49ac17251..33c25a5e06f9 100644 > > > > > --- a/drivers/dma-buf/heaps/system_heap.c > > > > > +++ b/drivers/dma-buf/heaps/system_heap.c > > > > > @@ -40,7 +40,7 @@ struct dma_heap_attachment { > > > > > bool mapped; > > > > > }; > > > > > > > > > > -#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \ > > > > > +#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO \ > > > > > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > > > > > | __GFP_COMP) > > > > > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > > > > > @@ -315,6 +315,7 @@ static struct page *alloc_largest_available(unsigned long size, > > > > > unsigned int max_order) > > > > > { > > > > > struct page *page; > > > > > + unsigned long gfp_flags; > > > > > int i; > > > > > > > > > > for (i = 0; i < NUM_ORDERS; i++) { > > > > > @@ -323,7 +324,11 @@ static struct page *alloc_largest_available(unsigned long size, > > > > > if (max_order < orders[i]) > > > > > continue; > > > > > > > > > > - page = alloc_pages(order_flags[i], orders[i]); > > > > > + gfp_flags = order_flags[i]; > > > > > + if (orders[i] > PAGE_ALLOC_COSTLY_ORDER) > > > > > + gfp_flags |= __GFP_NOWARN; > > > > > + > > > > > + page = alloc_pages(gfp_flags, orders[i]); > > > > > > > > Would it be cleaner to just set up the flags properly in the > > > > order_flags array? I'm not sure I understand why your patch does it > > > > dynamically? > > > > > > That's exactly I had in my branch for aosp fix but I wanted to > > > hear it explicitly from dmabuf maintainer since I was worried > > > chaninging order-4 allocation behavior, especially, > > > __GFP_NORETRY and &~__GFP_RECLAIM. > > > (It will make allocation failure easier than old and that's not > > > thing my patch is addressing). > > > > Yea. I might stick to changing just the __GFP_NOWARN. > > > > > If you want this, I am happy to change it. Shall I? > > > > > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > > > index 29e49ac17251..865ec847013d 100644 > > > --- a/drivers/dma-buf/heaps/system_heap.c > > > +++ b/drivers/dma-buf/heaps/system_heap.c > > > @@ -44,7 +44,7 @@ struct dma_heap_attachment { > > > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > > > | __GFP_COMP) > > > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > > > -static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP}; > > > +static gfp_t order_flags[] = {HIGH_ORDER_GFP, HIGH_ORDER_GFP, LOW_ORDER_GFP}; > > > > Maybe can you define a MID_ORDER_GFP as LOW_ORDER | __GFP_NOWARN > > (along with a comment in the code as to why) instead ? > > > > That avoids introducing any subtle behavioral change unintentionally. > > How about this one? Feel free to suggest better wording. > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 29e49ac17251..6e17ff06331e 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -44,7 +44,13 @@ struct dma_heap_attachment { > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > | __GFP_COMP) > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > -static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP}; > +/* > + * order-4 is PAGE_ALLOC_COSTLY_ORDER which is order allocator could fail > + * easier than lower orders. Since we have fallback order-0 allocation, > + * do not add warn. > + */ Maybe: "Avoid warning on order-4 allocation failures as we'll fall back to order-0 in that case." > +#define MID_ORDER_GFP (LOW_ORDER_GFP | __GFP_NOWARN) My only other nit is to suggest sorting the LOW/MID/HIGH defines. thanks -john ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation 2021-02-11 2:14 ` John Stultz @ 2021-02-11 2:30 ` Minchan Kim 0 siblings, 0 replies; 10+ messages in thread From: Minchan Kim @ 2021-02-11 2:30 UTC (permalink / raw) To: John Stultz Cc: Sumit Semwal, LKML, Suren Baghdasaryan, Hridya Valsaraju, John Dias On Wed, Feb 10, 2021 at 06:14:46PM -0800, John Stultz wrote: > On Wed, Feb 10, 2021 at 3:17 PM Minchan Kim <minchan@kernel.org> wrote: > > > > On Wed, Feb 10, 2021 at 01:40:02PM -0800, John Stultz wrote: > > > On Wed, Feb 10, 2021 at 9:48 AM Minchan Kim <minchan@kernel.org> wrote: > > > > > > > > On Wed, Feb 10, 2021 at 09:32:09AM -0800, John Stultz wrote: > > > > > On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <minchan@kernel.org> wrote: > > > > > > > > > > > > Linux VM is not hard to support PAGE_ALLOC_COSTLY_ODER allocation > > > > > > so normally expects driver passes __GFP_NOWARN in that case > > > > > > if they has fallback options. > > > > > > > > > > > > system_heap in dmabuf is the case so do not flood into demsg > > > > > > with the warning for recording more precious information logs. > > > > > > (below is ION warning example I got but dmabuf system heap is > > > > > > nothing different). > > > > > > > > > > > > [ 1233.911533][ T460] warn_alloc: 11 callbacks suppressed > > > > > > [ 1233.911539][ T460] allocator@2.0-s: page allocation failure: order:4, mode:0x140dc2(GFP_HIGHUSER|__GFP_COMP|__GFP_ZERO), nodemask=(null),cpuset=/,mems_allowed=0 > > > > > > [ 1233.926235][ T460] Call trace: > > > > > > [ 1233.929370][ T460] dump_backtrace+0x0/0x1d8 > > > > > > [ 1233.933704][ T460] show_stack+0x18/0x24 > > > > > > [ 1233.937701][ T460] dump_stack+0xc0/0x140 > > > > > > [ 1233.941783][ T460] warn_alloc+0xf4/0x148 > > > > > > [ 1233.945862][ T460] __alloc_pages_slowpath+0x9fc/0xa10 > > > > > > [ 1233.951101][ T460] __alloc_pages_nodemask+0x278/0x2c0 > > > > > > [ 1233.956285][ T460] ion_page_pool_alloc+0xd8/0x100 > > > > > > [ 1233.961144][ T460] ion_system_heap_allocate+0xbc/0x2f0 > > > > > > [ 1233.966440][ T460] ion_buffer_create+0x68/0x274 > > > > > > [ 1233.971130][ T460] ion_buffer_alloc+0x8c/0x110 > > > > > > [ 1233.975733][ T460] ion_dmabuf_alloc+0x44/0xe8 > > > > > > [ 1233.980248][ T460] ion_ioctl+0x100/0x320 > > > > > > [ 1233.984332][ T460] __arm64_sys_ioctl+0x90/0xc8 > > > > > > [ 1233.988934][ T460] el0_svc_common+0x9c/0x168 > > > > > > [ 1233.993360][ T460] do_el0_svc+0x1c/0x28 > > > > > > [ 1233.997358][ T460] el0_sync_handler+0xd8/0x250 > > > > > > [ 1234.001989][ T460] el0_sync+0x148/0x180 > > > > > > > > > > > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > > > > > --- > > > > > > drivers/dma-buf/heaps/system_heap.c | 9 +++++++-- > > > > > > 1 files changed, 7 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > > > > > > index 29e49ac17251..33c25a5e06f9 100644 > > > > > > --- a/drivers/dma-buf/heaps/system_heap.c > > > > > > +++ b/drivers/dma-buf/heaps/system_heap.c > > > > > > @@ -40,7 +40,7 @@ struct dma_heap_attachment { > > > > > > bool mapped; > > > > > > }; > > > > > > > > > > > > -#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \ > > > > > > +#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO \ > > > > > > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > > > > > > | __GFP_COMP) > > > > > > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > > > > > > @@ -315,6 +315,7 @@ static struct page *alloc_largest_available(unsigned long size, > > > > > > unsigned int max_order) > > > > > > { > > > > > > struct page *page; > > > > > > + unsigned long gfp_flags; > > > > > > int i; > > > > > > > > > > > > for (i = 0; i < NUM_ORDERS; i++) { > > > > > > @@ -323,7 +324,11 @@ static struct page *alloc_largest_available(unsigned long size, > > > > > > if (max_order < orders[i]) > > > > > > continue; > > > > > > > > > > > > - page = alloc_pages(order_flags[i], orders[i]); > > > > > > + gfp_flags = order_flags[i]; > > > > > > + if (orders[i] > PAGE_ALLOC_COSTLY_ORDER) > > > > > > + gfp_flags |= __GFP_NOWARN; > > > > > > + > > > > > > + page = alloc_pages(gfp_flags, orders[i]); > > > > > > > > > > Would it be cleaner to just set up the flags properly in the > > > > > order_flags array? I'm not sure I understand why your patch does it > > > > > dynamically? > > > > > > > > That's exactly I had in my branch for aosp fix but I wanted to > > > > hear it explicitly from dmabuf maintainer since I was worried > > > > chaninging order-4 allocation behavior, especially, > > > > __GFP_NORETRY and &~__GFP_RECLAIM. > > > > (It will make allocation failure easier than old and that's not > > > > thing my patch is addressing). > > > > > > Yea. I might stick to changing just the __GFP_NOWARN. > > > > > > > If you want this, I am happy to change it. Shall I? > > > > > > > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > > > > index 29e49ac17251..865ec847013d 100644 > > > > --- a/drivers/dma-buf/heaps/system_heap.c > > > > +++ b/drivers/dma-buf/heaps/system_heap.c > > > > @@ -44,7 +44,7 @@ struct dma_heap_attachment { > > > > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > > > > | __GFP_COMP) > > > > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > > > > -static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP}; > > > > +static gfp_t order_flags[] = {HIGH_ORDER_GFP, HIGH_ORDER_GFP, LOW_ORDER_GFP}; > > > > > > Maybe can you define a MID_ORDER_GFP as LOW_ORDER | __GFP_NOWARN > > > (along with a comment in the code as to why) instead ? > > > > > > That avoids introducing any subtle behavioral change unintentionally. > > > > How about this one? Feel free to suggest better wording. > > > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > > index 29e49ac17251..6e17ff06331e 100644 > > --- a/drivers/dma-buf/heaps/system_heap.c > > +++ b/drivers/dma-buf/heaps/system_heap.c > > @@ -44,7 +44,13 @@ struct dma_heap_attachment { > > | __GFP_NORETRY) & ~__GFP_RECLAIM) \ > > | __GFP_COMP) > > #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP) > > -static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP}; > > +/* > > + * order-4 is PAGE_ALLOC_COSTLY_ORDER which is order allocator could fail > > + * easier than lower orders. Since we have fallback order-0 allocation, > > + * do not add warn. > > + */ > > Maybe: "Avoid warning on order-4 allocation failures as we'll fall > back to order-0 in that case." > > > +#define MID_ORDER_GFP (LOW_ORDER_GFP | __GFP_NOWARN) > > My only other nit is to suggest sorting the LOW/MID/HIGH defines. Yub, let me cook it Thanks for the review, John. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation 2021-02-10 16:26 [PATCH] dma-buf: system_heap: do not warn for costly allocation Minchan Kim 2021-02-10 17:24 ` Suren Baghdasaryan 2021-02-10 17:32 ` John Stultz @ 2021-02-10 21:12 ` kernel test robot 2 siblings, 0 replies; 10+ messages in thread From: kernel test robot @ 2021-02-10 21:12 UTC (permalink / raw) To: Minchan Kim, sumit.semwal, john.stultz Cc: kbuild-all, LKML, surenb, hridya, joaodias, Minchan Kim [-- Attachment #1: Type: text/plain, Size: 3290 bytes --] Hi Minchan, I love your patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v5.11-rc7 next-20210125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Minchan-Kim/dma-buf-system_heap-do-not-warn-for-costly-allocation/20210211-003048 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e0756cfc7d7cd08c98a53b6009c091a3f6a50be6 config: i386-randconfig-s002-20210209 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-215-g0fb77bb6-dirty # https://github.com/0day-ci/linux/commit/28176611ddc50d5d84aa71679f32b0b22dbf4b1c git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Minchan-Kim/dma-buf-system_heap-do-not-warn-for-costly-allocation/20210211-003048 git checkout 28176611ddc50d5d84aa71679f32b0b22dbf4b1c # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> "sparse warnings: (new ones prefixed by >>)" >> drivers/dma-buf/heaps/system_heap.c:327:27: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned long gfp_flags @@ got restricted gfp_t @@ drivers/dma-buf/heaps/system_heap.c:327:27: sparse: expected unsigned long gfp_flags drivers/dma-buf/heaps/system_heap.c:327:27: sparse: got restricted gfp_t >> drivers/dma-buf/heaps/system_heap.c:329:35: sparse: sparse: invalid assignment: |= >> drivers/dma-buf/heaps/system_heap.c:329:35: sparse: left side has type unsigned long >> drivers/dma-buf/heaps/system_heap.c:329:35: sparse: right side has type restricted gfp_t >> drivers/dma-buf/heaps/system_heap.c:331:36: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected restricted gfp_t [usertype] gfp_mask @@ got unsigned long gfp_flags @@ drivers/dma-buf/heaps/system_heap.c:331:36: sparse: expected restricted gfp_t [usertype] gfp_mask drivers/dma-buf/heaps/system_heap.c:331:36: sparse: got unsigned long gfp_flags vim +327 drivers/dma-buf/heaps/system_heap.c 313 314 static struct page *alloc_largest_available(unsigned long size, 315 unsigned int max_order) 316 { 317 struct page *page; 318 unsigned long gfp_flags; 319 int i; 320 321 for (i = 0; i < NUM_ORDERS; i++) { 322 if (size < (PAGE_SIZE << orders[i])) 323 continue; 324 if (max_order < orders[i]) 325 continue; 326 > 327 gfp_flags = order_flags[i]; 328 if (orders[i] > PAGE_ALLOC_COSTLY_ORDER) > 329 gfp_flags |= __GFP_NOWARN; 330 > 331 page = alloc_pages(gfp_flags, orders[i]); 332 if (!page) 333 continue; 334 return page; 335 } 336 return NULL; 337 } 338 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 34719 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-11 2:31 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-10 16:26 [PATCH] dma-buf: system_heap: do not warn for costly allocation Minchan Kim 2021-02-10 17:24 ` Suren Baghdasaryan 2021-02-10 17:41 ` Minchan Kim 2021-02-10 17:32 ` John Stultz 2021-02-10 17:48 ` Minchan Kim 2021-02-10 21:40 ` John Stultz 2021-02-10 23:17 ` Minchan Kim 2021-02-11 2:14 ` John Stultz 2021-02-11 2:30 ` Minchan Kim 2021-02-10 21:12 ` kernel test robot
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).