From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755215AbbKGBXa (ORCPT ); Fri, 6 Nov 2015 20:23:30 -0500 Received: from www262.sakura.ne.jp ([202.181.97.72]:36681 "EHLO www262.sakura.ne.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750811AbbKGBX2 (ORCPT ); Fri, 6 Nov 2015 20:23:28 -0500 Subject: Re: [PATCH] jbd2: get rid of superfluous __GFP_REPEAT To: mhocko@kernel.org, linux-mm@kvack.org References: <1446740160-29094-4-git-send-email-mhocko@kernel.org> <1446826623-23959-1-git-send-email-mhocko@kernel.org> Cc: Andrew Morton , "Theodore Ts'o" , LKML , Michal Hocko , john.johansen@canonical.com From: Tetsuo Handa Message-ID: <563D526F.6030504@I-love.SAKURA.ne.jp> Date: Sat, 7 Nov 2015 10:22:55 +0900 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <1446826623-23959-1-git-send-email-mhocko@kernel.org> Content-Type: text/plain; charset=iso-2022-jp Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2015/11/07 1:17, mhocko@kernel.org wrote: > From: Michal Hocko > > jbd2_alloc is explicit about its allocation preferences wrt. the > allocation size. Sub page allocations go to the slab allocator > and larger are using either the page allocator or vmalloc. This > is all good but the logic is unnecessarily complex. Requests larger > than order-3 are doing the vmalloc directly while smaller go to the > page allocator with __GFP_REPEAT. The flag doesn't do anything useful > for those because they are smaller than PAGE_ALLOC_COSTLY_ORDER. > > Let's simplify the code flow and use kmalloc for sub-page requests > and the page allocator for others with fallback to vmalloc if the > allocation fails. > > Cc: "Theodore Ts'o" > Signed-off-by: Michal Hocko > --- > fs/jbd2/journal.c | 35 ++++++++++++----------------------- > 1 file changed, 12 insertions(+), 23 deletions(-) > > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > index 81e622681c82..2945c96f171f 100644 > --- a/fs/jbd2/journal.c > +++ b/fs/jbd2/journal.c > @@ -2299,18 +2299,15 @@ void *jbd2_alloc(size_t size, gfp_t flags) > > BUG_ON(size & (size-1)); /* Must be a power of 2 */ > > - flags |= __GFP_REPEAT; > - if (size == PAGE_SIZE) > - ptr = (void *)__get_free_pages(flags, 0); > - else if (size > PAGE_SIZE) { > + if (size < PAGE_SIZE) > + ptr = kmem_cache_alloc(get_slab(size), flags); > + else { > int order = get_order(size); > > - if (order < 3) > - ptr = (void *)__get_free_pages(flags, order); > - else > + ptr = (void *)__get_free_pages(flags, order); I thought that we can add __GFP_NOWARN for this __get_free_pages() call. But I noticed more important problem. See below. > + if (!ptr) > ptr = vmalloc(size); > - } else > - ptr = kmem_cache_alloc(get_slab(size), flags); > + } > > /* Check alignment; SLUB has gotten this wrong in the past, > * and this can lead to user data corruption! */ > @@ -2321,20 +2318,12 @@ void *jbd2_alloc(size_t size, gfp_t flags) > > void jbd2_free(void *ptr, size_t size) > { > - if (size == PAGE_SIZE) { > - free_pages((unsigned long)ptr, 0); > - return; > - } > - if (size > PAGE_SIZE) { > - int order = get_order(size); > - > - if (order < 3) > - free_pages((unsigned long)ptr, order); > - else > - vfree(ptr); > - return; > - } > - kmem_cache_free(get_slab(size), ptr); > + if (size < PAGE_SIZE) > + kmem_cache_free(get_slab(size), ptr); > + else if (is_vmalloc_addr(ptr)) > + vfree(ptr); > + else > + free_pages((unsigned long)ptr, get_order(size)); > }; > > /* > All jbd2_alloc() callers seem to pass GFP_NOFS. Therefore, use of vmalloc() which implicitly passes GFP_KERNEL | __GFP_HIGHMEM can cause deadlock, can't it? This vmalloc(size) call needs to be replaced with __vmalloc(size, flags). We need to check all vmalloc() callers in case they are calling vmalloc() under GFP_KERNEL-unsafe context. For example, I think that __aa_kvmalloc() needs to use __vmalloc() too.