Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <7cd33b23-64f5-a736-ea69-b29e40d42e78@suse.cz>
Date:   Mon, 4 Dec 2023 17:58:08 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v5 6/9] slub: Delay freezing of partial slabs
Content-Language: en-US
To:     Chengming Zhou <chengming.zhou@linux.dev>,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc:     cl@linux.com, penberg@kernel.org, rientjes@google.com,
        iamjoonsoo.kim@lge.com, akpm@linux-foundation.org,
        roman.gushchin@linux.dev, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Chengming Zhou <zhouchengming@bytedance.com>
References: <20231102032330.1036151-1-chengming.zhou@linux.dev>
 <20231102032330.1036151-7-chengming.zhou@linux.dev>
 <CAB=+i9TK-_WcmQLVV5BTvngfTfTZra0Xi-XZ2m8JGbamwWDuoA@mail.gmail.com>
 <98763097-d05e-40cd-afe0-4df65083d104@linux.dev>
From:   Vlastimil Babka <vbabka@suse.cz>
In-Reply-To: <98763097-d05e-40cd-afe0-4df65083d104@linux.dev>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 12/3/23 11:15, Chengming Zhou wrote:
> On 2023/12/3 14:53, Hyeonggon Yoo wrote:
>> On Thu, Nov 2, 2023 at 12:25 PM <chengming.zhou@linux.dev> wrote:
>>>
>>> From: Chengming Zhou <zhouchengming@bytedance.com>
>>>
>>> Now we will freeze slabs when moving them out of node partial list to
>>> cpu partial list, this method needs two cmpxchg_double operations:
>>>
>>> 1. freeze slab (acquire_slab()) under the node list_lock
>>> 2. get_freelist() when pick used in ___slab_alloc()
>>>
>>> Actually we don't need to freeze when moving slabs out of node partial
>>> list, we can delay freezing to when use slab freelist in ___slab_alloc(),
>>> so we can save one cmpxchg_double().
>>>
>>> And there are other good points:
>>>  - The moving of slabs between node partial list and cpu partial list
>>>    becomes simpler, since we don't need to freeze or unfreeze at all.
>>>
>>>  - The node list_lock contention would be less, since we don't need to
>>>    freeze any slab under the node list_lock.
>>>
>>> We can achieve this because there is no concurrent path would manipulate
>>> the partial slab list except the __slab_free() path, which is now
>>> serialized by slab_test_node_partial() under the list_lock.
>>>
>>> Since the slab returned by get_partial() interfaces is not frozen anymore
>>> and no freelist is returned in the partial_context, so we need to use the
>>> introduced freeze_slab() to freeze it and get its freelist.
>>>
>>> Similarly, the slabs on the CPU partial list are not frozen anymore,
>>> we need to freeze_slab() on it before use.
>>>
>>> We can now delete acquire_slab() as it became unused.
>>>
>>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>>> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
>>> Tested-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
>>> ---
>>>  mm/slub.c | 113 +++++++++++-------------------------------------------
>>>  1 file changed, 23 insertions(+), 90 deletions(-)
>>>
>>> diff --git a/mm/slub.c b/mm/slub.c
>>> index edf567971679..bcb5b2c4e213 100644
>>> --- a/mm/slub.c
>>> +++ b/mm/slub.c
>>> @@ -2234,51 +2234,6 @@ static void *alloc_single_from_new_slab(struct kmem_cache *s,
>>>         return object;
>>>  }
>>>
>>> -/*
>>> - * Remove slab from the partial list, freeze it and
>>> - * return the pointer to the freelist.
>>> - *
>>> - * Returns a list of objects or NULL if it fails.
>>> - */
>>> -static inline void *acquire_slab(struct kmem_cache *s,
>>> -               struct kmem_cache_node *n, struct slab *slab,
>>> -               int mode)
>> 
>> Nit: alloc_single_from_partial()'s comment still refers to acquire_slab().
>> 
> 
> Ah, right! It should be changed to remove_partial().
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 437485a2408d..623c17a4cdd6 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2463,7 +2463,7 @@ static inline void remove_partial(struct kmem_cache_node *n,
>  }
> 
>  /*
> - * Called only for kmem_cache_debug() caches instead of acquire_slab(), with a
> + * Called only for kmem_cache_debug() caches instead of remove_partial(), with a
>   * slab from the n->partial list. Remove only a single object from the slab, do
>   * the alloc_debug_processing() checks and leave the slab on the list, or move
>   * it to full list if it was the last free object.
> 
> Hi Vlastimil, could you please help to fold it?

Done, thanks.