From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CBCDBC433E0 for ; Thu, 11 Feb 2021 02:31:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9571464E4A for ; Thu, 11 Feb 2021 02:31:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229789AbhBKCba (ORCPT ); Wed, 10 Feb 2021 21:31:30 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56124 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229752AbhBKCbN (ORCPT ); Wed, 10 Feb 2021 21:31:13 -0500 Received: from mail-pj1-x102d.google.com (mail-pj1-x102d.google.com [IPv6:2607:f8b0:4864:20::102d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6150AC061574 for ; Wed, 10 Feb 2021 18:30:33 -0800 (PST) Received: by mail-pj1-x102d.google.com with SMTP id gb24so2432802pjb.4 for ; Wed, 10 Feb 2021 18:30:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=65eB/aEffg+NC6m9O4O2k2LGrBXfS7B2k/rRczH64E4=; b=DLkCZVlF1J5UOImwOLw0v9TRi6lr0Ke8pCHhUwSC/RQhSV6wLWWtWQL14dIxN24TZq 9cEIw5V9slA0ZvFxuIGRbgjJMV8PjAfjcNjIUuukPI6G9Ecj5oIWFvw9f/sUmofp+acU BWgvwpxvrlUYkNtkrWgWhOGvCW8W50eLzFJNF3IbNHYOv+rapmsZ74Yl0PyVNJ1l9fcE HtCBQa4o1/VXKJaphXSu+Q4d9hUqhoScR2CCHUwSJBaDuXKvNchCEC7do/doI4DUz2F0 4WQ2xk32eiWlkiaRrd5BV+x+bg4I/0lXCEExs0dLCKW8mGME8pP/JU0NQP1ANMwQRtq+ phcw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to; bh=65eB/aEffg+NC6m9O4O2k2LGrBXfS7B2k/rRczH64E4=; b=RmTqvBeKjz6a7t2g3rA0yXMcsQcgsZkwx9x1zHKOmp7zkv5onOsNOv2CTrrtNtAgkt 123pQDJJM/RuHSmtifiwy7gZY4WDkT33heJUDpYJ2aP1Taz6shrEcsldPAaeh9CXhEQN aemcnFzmw/bqY7K9LPIj11jM1ROapzPpKBeXzBWt3LTm/RuYc1edq6j+Nl/R8KQTQg3U 2uNLdBtFuzepkT6+z5UcznE5ZWt+smxfh0+9ILA4s8QAP8xo1t901f2NYbFyN5J/wj/x kz5tDYHPsXMiAzwXIM6Jjhmv/Zn0AGd5Qzli6ChZ2upI3jnfHanOrFQDYMU1NcdVuZC9 2PKw== X-Gm-Message-State: AOAM533AT1LM+TTEzC69ClYb+xxwbCMJ5tvtr4HBbLkEoBCatf9s366x +TcHlkyy9FslGujlU92DQOQvb3jjU+s= X-Google-Smtp-Source: ABdhPJwLnarcijOL4qTg3l5T3HxN4a1FSqfwgxX5H0qoWhVtVOpGrqwBzhm0vT+hBK1IqBaIIyrD0Q== X-Received: by 2002:a17:90a:7404:: with SMTP id a4mr1759231pjg.167.1613010632871; Wed, 10 Feb 2021 18:30:32 -0800 (PST) Received: from google.com ([2620:15c:211:201:e5b0:be96:1dfb:a1f6]) by smtp.gmail.com with ESMTPSA id q2sm3479812pfj.32.2021.02.10.18.30.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 10 Feb 2021 18:30:31 -0800 (PST) Sender: Minchan Kim Date: Wed, 10 Feb 2021 18:30:30 -0800 From: Minchan Kim To: John Stultz Cc: Sumit Semwal , LKML , Suren Baghdasaryan , Hridya Valsaraju , John Dias Subject: Re: [PATCH] dma-buf: system_heap: do not warn for costly allocation Message-ID: References: <20210210162632.3903128-1-minchan@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 10, 2021 at 06:14:46PM -0800, John Stultz wrote: > On Wed, Feb 10, 2021 at 3:17 PM Minchan Kim 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 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 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 > > > > > > --- > > > > > > 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.