Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210210162632.3903128-1-minchan@kernel.org> <CALAqxLXzc3tfsr0hA6GS-zHjupWx++Bhcrs2pjbz00LNKeThOQ@mail.gmail.com>
 <YCQcfYRQ3eW+QiMz@google.com> <CALAqxLUaiOOrC6kWYSj1yg6qed32rQhfN4k99HNgn_=0kpFRJw@mail.gmail.com>
 <YCRpclaUOkEWA83o@google.com>
In-Reply-To: <YCRpclaUOkEWA83o@google.com>
From:   John Stultz <john.stultz@linaro.org>
Date:   Wed, 10 Feb 2021 18:14:46 -0800
Message-ID: <CALAqxLU33Mt_mYXPUxcbZB6whynd2PRPuLHNJmX-mx+od6hXVw@mail.gmail.com>
Subject: Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation
To:     Minchan Kim <minchan@kernel.org>
Cc:     Sumit Semwal <sumit.semwal@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Suren Baghdasaryan <surenb@google.com>,
        Hridya Valsaraju <hridya@google.com>,
        John Dias <joaodias@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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