Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 15 Sep 2019 14:38:37 -0700 (PDT)
From:   David Rientjes <rientjes@google.com>
To:     Pengfei Li <lpf.vector@gmail.com>
cc:     akpm@linux-foundation.org, vbabka@suse.cz, cl@linux.com,
        penberg@kernel.org, iamjoonsoo.kim@lge.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, guro@fb.com
Subject: Re: [RESEND v4 6/7] mm, slab_common: Initialize the same size of
 kmalloc_caches[]
In-Reply-To: <20190915170809.10702-7-lpf.vector@gmail.com>
Message-ID: <alpine.DEB.2.21.1909151434140.211705@chino.kir.corp.google.com>
References: <20190915170809.10702-1-lpf.vector@gmail.com> <20190915170809.10702-7-lpf.vector@gmail.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 16 Sep 2019, Pengfei Li wrote:

> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 2aed30deb071..e7903bd28b1f 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1165,12 +1165,9 @@ void __init setup_kmalloc_cache_index_table(void)
>  		size_index[size_index_elem(i)] = 0;
>  }
>  
> -static void __init
> +static __always_inline void __init
>  new_kmalloc_cache(int idx, enum kmalloc_cache_type type, slab_flags_t flags)
>  {
> -	if (type == KMALLOC_RECLAIM)
> -		flags |= SLAB_RECLAIM_ACCOUNT;
> -
>  	kmalloc_caches[type][idx] = create_kmalloc_cache(
>  					kmalloc_info[idx].name[type],
>  					kmalloc_info[idx].size, flags, 0,
> @@ -1185,30 +1182,22 @@ new_kmalloc_cache(int idx, enum kmalloc_cache_type type, slab_flags_t flags)
>  void __init create_kmalloc_caches(slab_flags_t flags)
>  {
>  	int i;
> -	enum kmalloc_cache_type type;
>  
> -	for (type = KMALLOC_NORMAL; type <= KMALLOC_RECLAIM; type++) {
> -		for (i = 0; i < KMALLOC_CACHE_NUM; i++) {
> -			if (!kmalloc_caches[type][i])
> -				new_kmalloc_cache(i, type, flags);
> -		}
> -	}
> +	for (i = 0; i < KMALLOC_CACHE_NUM; i++) {
> +		if (!kmalloc_caches[KMALLOC_NORMAL][i])
> +			new_kmalloc_cache(i, KMALLOC_NORMAL, flags);
>  
> -	/* Kmalloc array is now usable */
> -	slab_state = UP;
> +		new_kmalloc_cache(i, KMALLOC_RECLAIM,
> +					flags | SLAB_RECLAIM_ACCOUNT);

This seems less robust, no?  Previously we verified that the cache doesn't 
exist before creating a new cache over top of it (for NORMAL and RECLAIM).  
Now we presume that the RECLAIM cache never exists.

Can we just move a check to new_kmalloc_cache() to see if 
kmalloc_caches[type][idx] already exists and, if so, just return?  This 
should be more robust and simplify create_kmalloc_caches() slightly more.