lkml.org 
[lkml]   [2023]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH v5 6/9] slub: Delay freezing of partial slabs
    From
    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?

    Thanks!

    >> -{
    >> - void *freelist;
    >> - unsigned long counters;
    >> - struct slab new;
    >> -
    >> - lockdep_assert_held(&n->list_lock);
    >> -
    >> - /*
    >> - * Zap the freelist and set the frozen bit.
    >> - * The old freelist is the list of objects for the
    >> - * per cpu allocation list.
    >> - */
    >> - freelist = slab->freelist;
    >> - counters = slab->counters;
    >> - new.counters = counters;
    >> - if (mode) {
    >> - new.inuse = slab->objects;
    >> - new.freelist = NULL;
    >> - } else {
    >> - new.freelist = freelist;
    >> - }
    >> -
    >> - VM_BUG_ON(new.frozen);
    >> - new.frozen = 1;
    >> -
    >> - if (!__slab_update_freelist(s, slab,
    >> - freelist, counters,
    >> - new.freelist, new.counters,
    >> - "acquire_slab"))
    >> - return NULL;
    >> -
    >> - remove_partial(n, slab);
    >> - WARN_ON(!freelist);
    >> - return freelist;
    >> -}
    >> -
    >> #ifdef CONFIG_SLUB_CPU_PARTIAL
    >> static void put_cpu_partial(struct kmem_cache *s, struct slab *slab, int drain);
    >> #else
    >> @@ -2295,7 +2250,6 @@ static struct slab *get_partial_node(struct kmem_cache *s,
    >> struct partial_context *pc)
    >> {
    >> struct slab *slab, *slab2, *partial = NULL;
    >> - void *object = NULL;
    >> unsigned long flags;
    >> unsigned int partial_slabs = 0;
    >>
    >> @@ -2314,7 +2268,7 @@ static struct slab *get_partial_node(struct kmem_cache *s,
    >> continue;
    >>
    >> if (IS_ENABLED(CONFIG_SLUB_TINY) || kmem_cache_debug(s)) {
    >> - object = alloc_single_from_partial(s, n, slab,
    >> + void *object = alloc_single_from_partial(s, n, slab,
    >> pc->orig_size);
    >> if (object) {
    >> partial = slab;
    >> @@ -2324,13 +2278,10 @@ static struct slab *get_partial_node(struct kmem_cache *s,
    >> continue;
    >> }
    >>
    >> - object = acquire_slab(s, n, slab, object == NULL);
    >> - if (!object)
    >> - break;
    >> + remove_partial(n, slab);
    >>
    >> if (!partial) {
    >> partial = slab;
    >> - pc->object = object;
    >> stat(s, ALLOC_FROM_PARTIAL);
    >> } else {
    >> put_cpu_partial(s, slab, 0);
    >> @@ -2629,9 +2580,6 @@ static void __unfreeze_partials(struct kmem_cache *s, struct slab *partial_slab)
    >> unsigned long flags = 0;
    >>
    >> while (partial_slab) {
    >> - struct slab new;
    >> - struct slab old;
    >> -
    >> slab = partial_slab;
    >> partial_slab = slab->next;
    >>
    >> @@ -2644,23 +2592,7 @@ static void __unfreeze_partials(struct kmem_cache *s, struct slab *partial_slab)
    >> spin_lock_irqsave(&n->list_lock, flags);
    >> }
    >>
    >> - do {
    >> -
    >> - old.freelist = slab->freelist;
    >> - old.counters = slab->counters;
    >> - VM_BUG_ON(!old.frozen);
    >> -
    >> - new.counters = old.counters;
    >> - new.freelist = old.freelist;
    >> -
    >> - new.frozen = 0;
    >> -
    >> - } while (!__slab_update_freelist(s, slab,
    >> - old.freelist, old.counters,
    >> - new.freelist, new.counters,
    >> - "unfreezing slab"));
    >> -
    >> - if (unlikely(!new.inuse && n->nr_partial >= s->min_partial)) {
    >> + if (unlikely(!slab->inuse && n->nr_partial >= s->min_partial)) {
    >> slab->next = slab_to_discard;
    >> slab_to_discard = slab;
    >> } else {
    >> @@ -3167,7 +3099,6 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
    >> node = NUMA_NO_NODE;
    >> goto new_slab;
    >> }
    >> -redo:
    >>
    >> if (unlikely(!node_match(slab, node))) {
    >> /*
    >> @@ -3243,7 +3174,8 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
    >>
    >> new_slab:
    >>
    >> - if (slub_percpu_partial(c)) {
    >> +#ifdef CONFIG_SLUB_CPU_PARTIAL
    >> + while (slub_percpu_partial(c)) {
    >> local_lock_irqsave(&s->cpu_slab->lock, flags);
    >> if (unlikely(c->slab)) {
    >> local_unlock_irqrestore(&s->cpu_slab->lock, flags);
    >> @@ -3255,12 +3187,22 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
    >> goto new_objects;
    >> }
    >>
    >> - slab = c->slab = slub_percpu_partial(c);
    >> + slab = slub_percpu_partial(c);
    >> slub_set_percpu_partial(c, slab);
    >> local_unlock_irqrestore(&s->cpu_slab->lock, flags);
    >> stat(s, CPU_PARTIAL_ALLOC);
    >> - goto redo;
    >> +
    >> + if (unlikely(!node_match(slab, node) ||
    >> + !pfmemalloc_match(slab, gfpflags))) {
    >> + slab->next = NULL;
    >> + __unfreeze_partials(s, slab);
    >> + continue;
    >> + }
    >> +
    >> + freelist = freeze_slab(s, slab);
    >> + goto retry_load_slab;
    >> }
    >> +#endif
    >>
    >> new_objects:
    >>
    >> @@ -3268,8 +3210,8 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
    >> pc.orig_size = orig_size;
    >> slab = get_partial(s, node, &pc);
    >> if (slab) {
    >> - freelist = pc.object;
    >> if (kmem_cache_debug(s)) {
    >> + freelist = pc.object;
    >> /*
    >> * For debug caches here we had to go through
    >> * alloc_single_from_partial() so just store the
    >> @@ -3281,6 +3223,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
    >> return freelist;
    >> }
    >>
    >> + freelist = freeze_slab(s, slab);
    >> goto retry_load_slab;
    >> }
    >>
    >> @@ -3682,18 +3625,8 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
    >> was_frozen = new.frozen;
    >> new.inuse -= cnt;
    >> if ((!new.inuse || !prior) && !was_frozen) {
    >> -
    >> - if (kmem_cache_has_cpu_partial(s) && !prior) {
    >> -
    >> - /*
    >> - * Slab was on no list before and will be
    >> - * partially empty
    >> - * We can defer the list move and instead
    >> - * freeze it.
    >> - */
    >> - new.frozen = 1;
    >> -
    >> - } else { /* Needs to be taken off a list */
    >> + /* Needs to be taken off a list */
    >> + if (!kmem_cache_has_cpu_partial(s) || prior) {
    >>
    >> n = get_node(s, slab_nid(slab));
    >> /*
    >> @@ -3723,9 +3656,9 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
    >> * activity can be necessary.
    >> */
    >> stat(s, FREE_FROZEN);
    >> - } else if (new.frozen) {
    >> + } else if (kmem_cache_has_cpu_partial(s) && !prior) {
    >> /*
    >> - * If we just froze the slab then put it onto the
    >> + * If we started with a full slab then put it onto the
    >> * per cpu partial list.
    >> */
    >> put_cpu_partial(s, slab, 1);
    >> --
    >
    > Looks good to me,
    > Reviewed-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
    >
    > Thanks!
    >
    >> 2.20.1
    >>

    \
     
     \ /
      Last update: 2023-12-03 11:16    [W:3.330 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site