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
| ||
|
Message-ID: <CAB=+i9TK-_WcmQLVV5BTvngfTfTZra0Xi-XZ2m8JGbamwWDuoA@mail.gmail.com> Date: Sun, 3 Dec 2023 15:53:19 +0900 From: Hyeonggon Yoo <42.hyeyoo@...il.com> To: chengming.zhou@...ux.dev Cc: vbabka@...e.cz, cl@...ux.com, penberg@...nel.org, rientjes@...gle.com, iamjoonsoo.kim@....com, akpm@...ux-foundation.org, roman.gushchin@...ux.dev, linux-mm@...ck.org, linux-kernel@...r.kernel.org, Chengming Zhou <zhouchengming@...edance.com> Subject: Re: [PATCH v5 6/9] slub: Delay freezing of partial slabs On Thu, Nov 2, 2023 at 12:25 PM <chengming.zhou@...ux.dev> wrote: > > From: Chengming Zhou <zhouchengming@...edance.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@...edance.com> > Reviewed-by: Vlastimil Babka <vbabka@...e.cz> > Tested-by: Hyeonggon Yoo <42.hyeyoo@...il.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(). > -{ > - 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@...il.com> Thanks! > 2.20.1 >
Powered by blists - more mailing lists