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] [email protected]: 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 <[email protected]>
---
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
The code looks fine to me. Description needs a bit polishing :)
On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <[email protected]> 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] [email protected]: 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 <[email protected]>
> ---
> 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
>
On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <[email protected]> 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] [email protected]: 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 <[email protected]>
> ---
> 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
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 <[email protected]> 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!
On Wed, Feb 10, 2021 at 09:32:09AM -0800, John Stultz wrote:
> On Wed, Feb 10, 2021 at 8:26 AM Minchan Kim <[email protected]> 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] [email protected]: 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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
"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/[email protected]
On Wed, Feb 10, 2021 at 9:48 AM Minchan Kim <[email protected]> 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 <[email protected]> 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] [email protected]: 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 <[email protected]>
> > > ---
> > > 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
On Wed, Feb 10, 2021 at 01:40:02PM -0800, John Stultz wrote:
> On Wed, Feb 10, 2021 at 9:48 AM Minchan Kim <[email protected]> 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 <[email protected]> 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] [email protected]: 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 <[email protected]>
> > > > ---
> > > > 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
On Wed, Feb 10, 2021 at 3:17 PM Minchan Kim <[email protected]> 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 <[email protected]> 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 <[email protected]> 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] [email protected]: 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 <[email protected]>
> > > > > ---
> > > > > 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
On Wed, Feb 10, 2021 at 06:14:46PM -0800, John Stultz wrote:
> On Wed, Feb 10, 2021 at 3:17 PM Minchan Kim <[email protected]> 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 <[email protected]> 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 <[email protected]> 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] [email protected]: 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 <[email protected]>
> > > > > > ---
> > > > > > 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.