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;
MIME-Version: 1.0
References: <20190915170809.10702-1-lpf.vector@gmail.com> <20190915170809.10702-7-lpf.vector@gmail.com>
 <alpine.DEB.2.21.1909151434140.211705@chino.kir.corp.google.com>
In-Reply-To: <alpine.DEB.2.21.1909151434140.211705@chino.kir.corp.google.com>
From:   Pengfei Li <lpf.vector@gmail.com>
Date:   Mon, 16 Sep 2019 23:04:29 +0800
Message-ID: <CAD7_sbEKRc0ipTh2SU93C9wJ8hOdqpwFFhacFkWg0Yn71ZYQfA@mail.gmail.com>
Subject: Re: [RESEND v4 6/7] mm, slab_common: Initialize the same size of kmalloc_caches[]
To:     David Rientjes <rientjes@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Christopher Lameter <cl@linux.com>, penberg@kernel.org,
        iamjoonsoo.kim@lge.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Roman Gushchin <guro@fb.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Sep 16, 2019 at 5:38 AM David Rientjes <rientjes@google.com> wrote:

Thanks for your review comments!

>
> 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.
>

Agree, this is really less robust.

I have checked the code and found that there is no place to initialize
kmalloc-rcl-xxx before create_kmalloc_caches(). So I assume that
kmalloc-rcl-xxx is NULL.

> 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.

For better robustness, I will do it as you suggested in v5.