From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1764007AbdAFKZL (ORCPT ); Fri, 6 Jan 2017 05:25:11 -0500 Received: from mx2.suse.de ([195.135.220.15]:38773 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S970076AbdAFKY3 (ORCPT ); Fri, 6 Jan 2017 05:24:29 -0500 Date: Fri, 6 Jan 2017 11:04:33 +0100 From: Michal Hocko To: Tom Herbert Cc: linux-mm@kvack.org, LKML Subject: Re: weird allocation pattern in alloc_ila_locks Message-ID: <20170106100433.GH5556@dhcp22.suse.cz> References: <20170106095115.GG5556@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170106095115.GG5556@dhcp22.suse.cz> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri 06-01-17 10:51:15, Michal Hocko wrote: > Hi Tom, > I am currently looking at kmalloc with vmalloc fallback users [1] > and came across alloc_ila_locks which is using a pretty unusual > allocation pattern - it seems to be a c&p alloc_bucket_locks which > is doing a similar thing - except it has to support GFP_ATOMIC. > > I am really wondering what is the point of > #ifdef CONFIG_NUMA > if (size * sizeof(spinlock_t) > PAGE_SIZE) > ilan->locks = vmalloc(size * sizeof(spinlock_t)); > else > #endif > > there doesn't seem to be any NUMA awareness in the ifdef code so I can > only assume that the intention is to reflect that NUMA machines tend to > have more CPUs. On the other hand nr_pcpus is limited to 32 so this > doesn't seem to be the case here... > Can we just get rid of this ugly and confusing code and do something as > simple as > diff --git a/net/ipv6/ila/ila_xlat.c b/net/ipv6/ila/ila_xlat.c > index af8f52ee7180..1d86ceae61b3 100644 > --- a/net/ipv6/ila/ila_xlat.c > +++ b/net/ipv6/ila/ila_xlat.c > @@ -41,13 +41,11 @@ static int alloc_ila_locks(struct ila_net *ilan) > size = roundup_pow_of_two(nr_pcpus * LOCKS_PER_CPU); > > if (sizeof(spinlock_t) != 0) { > -#ifdef CONFIG_NUMA > - if (size * sizeof(spinlock_t) > PAGE_SIZE) > - ilan->locks = vmalloc(size * sizeof(spinlock_t)); > - else > -#endif > ilan->locks = kmalloc_array(size, sizeof(spinlock_t), > - GFP_KERNEL); > + GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN); > + if (!ilan->locks) > + ilan->locks = vmalloc(size * sizeof(spinlock_t)); > + > if (!ilan->locks) > return -ENOMEM; > for (i = 0; i < size; i++) > > which I would then simply turn into kvmalloc()? The patch would look as follows: --- >>From 37f4478c6d3540664741c5172b29a5a5f6ee3a14 Mon Sep 17 00:00:00 2001 From: Michal Hocko Date: Fri, 6 Jan 2017 10:52:20 +0100 Subject: [PATCH] ila: simplify a strange allocation pattern alloc_ila_locks seemed to c&p from alloc_bucket_locks allocation pattern which is quite unusual. We are preferring vmalloc when CONFIG_NUMA is enabled which doesn't make much sense because there is no special NUMA locality handled in that code path. Let's just simplify the code and use kvmalloc helper which is a transparent way to use kmalloc with vmalloc fallback. Signed-off-by: Michal Hocko --- net/ipv6/ila/ila_xlat.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/net/ipv6/ila/ila_xlat.c b/net/ipv6/ila/ila_xlat.c index af8f52ee7180..2fd5ca151dcf 100644 --- a/net/ipv6/ila/ila_xlat.c +++ b/net/ipv6/ila/ila_xlat.c @@ -41,13 +41,7 @@ static int alloc_ila_locks(struct ila_net *ilan) size = roundup_pow_of_two(nr_pcpus * LOCKS_PER_CPU); if (sizeof(spinlock_t) != 0) { -#ifdef CONFIG_NUMA - if (size * sizeof(spinlock_t) > PAGE_SIZE) - ilan->locks = vmalloc(size * sizeof(spinlock_t)); - else -#endif - ilan->locks = kmalloc_array(size, sizeof(spinlock_t), - GFP_KERNEL); + ilan->locks = kvmalloc(size * sizeof(spinlock_t), GFP_KERNEL); if (!ilan->locks) return -ENOMEM; for (i = 0; i < size; i++) -- 2.11.0 -- Michal Hocko SUSE Labs