linux-kernel - Re: weird allocation pattern in alloc_ila

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170106100433.GH5556@dhcp22.suse.cz>
Date:   Fri, 6 Jan 2017 11:04:33 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Tom Herbert <tom@...bertland.com>
Cc:     linux-mm@...ck.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: weird allocation pattern in alloc_ila_locks

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 <mhocko@...e.com>
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 <mhocko@...e.com>
---
 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