On Mon, Sep 05, 2022 at 04:37:05PM +0800, Feng Tang wrote:
> On Mon, Sep 05, 2022 at 03:33:14PM +0800, Vlastimil Babka wrote:
> > On 9/5/22 09:06, Feng Tang wrote:
> > > On Mon, Sep 05, 2022 at 02:29:51PM +0800, Vlastimil Babka wrote:
> > >>
> > >> How about get_partial() instantiates an on-stack structure that contains
> > >> gfpflags, ret_slab, orig_size and passes pointer to that to all the nested
> > >> functions.
> > >>
> > >> Would be similar to "struct alloc_context" in page allocation.
> > >> Something like "struct partial_context pc"?
> > >
> > > Yep! This would make the parameters passing much tidier. Will try
> > > this way.
> > >
> > > More aggressively is to also embed the 'kmem_cache' parameter into
> > > it, but this may make the code look ambiguous.
> >
> > That one is used a lot everywhere, so it would be tedious to dereference it
> > from a struct, and also might be a bit better code if it's in a register.
>
> Got it. Following is the incremental patch for 1/4, which uses the
> 'partial_context' to pass parameters. And actually the 4/4 patch will
> benefit more from this refactoring, as the object initialization doesn't
> need to be separated and has race issue.
>
> Thanks,
> Feng
Looks fine to me.
will review when next version arrives :)
> ---
> diff --git a/mm/slub.c b/mm/slub.c
> index 82e7bd3a3966..7497fb6ca8e2 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -194,6 +194,12 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
> #endif
> #endif /* CONFIG_SLUB_DEBUG */
>
> +struct partial_context {
> + struct slab **slab;
> + gfp_t flags;
> + int orig_size;
> +};
> +
> static inline bool kmem_cache_debug(struct kmem_cache *s)
> {
> return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
> @@ -1333,7 +1339,7 @@ static inline int alloc_consistency_checks(struct kmem_cache *s,
> }
>
> static noinline int alloc_debug_processing(struct kmem_cache *s,
> - struct slab *slab, void *object)
> + struct slab *slab, void *object, int orig_size)
> {
> if (s->flags & SLAB_CONSISTENCY_CHECKS) {
> if (!alloc_consistency_checks(s, slab, object))
> @@ -1342,6 +1348,7 @@ static noinline int alloc_debug_processing(struct kmem_cache *s,
>
> /* Success. Perform special debug activities for allocs */
> trace(s, slab, object, 1);
> + set_orig_size(s, object, orig_size);
> init_object(s, object, SLUB_RED_ACTIVE);
> return 1;
>
> @@ -1610,7 +1617,7 @@ static inline
> void setup_slab_debug(struct kmem_cache *s, struct slab *slab, void *addr) {}
>
> static inline int alloc_debug_processing(struct kmem_cache *s,
> - struct slab *slab, void *object) { return 0; }
> + struct slab *slab, void *object, int orig_size) { return 0; }
>
> static inline void set_orig_size(struct kmem_cache *s,
> void *object, unsigned int orig_size) {}
> @@ -2042,7 +2049,7 @@ static inline void remove_partial(struct kmem_cache_node *n,
> * it to full list if it was the last free object.
> */
> static void *alloc_single_from_partial(struct kmem_cache *s,
> - struct kmem_cache_node *n, struct slab *slab)
> + struct kmem_cache_node *n, struct slab *slab, int orig_size)
> {
> void *object;
>
> @@ -2052,7 +2059,7 @@ static void *alloc_single_from_partial(struct kmem_cache *s,
> slab->freelist = get_freepointer(s, object);
> slab->inuse++;
>
> - if (!alloc_debug_processing(s, slab, object)) {
> + if (!alloc_debug_processing(s, slab, object, orig_size)) {
> remove_partial(n, slab);
> return NULL;
> }
> @@ -2071,7 +2078,7 @@ static void *alloc_single_from_partial(struct kmem_cache *s,
> * and put the slab to the partial (or full) list.
> */
> static void *alloc_single_from_new_slab(struct kmem_cache *s,
> - struct slab *slab)
> + struct slab *slab, int orig_size)
> {
> int nid = slab_nid(slab);
> struct kmem_cache_node *n = get_node(s, nid);
> @@ -2083,7 +2090,7 @@ static void *alloc_single_from_new_slab(struct kmem_cache *s,
> slab->freelist = get_freepointer(s, object);
> slab->inuse = 1;
>
> - if (!alloc_debug_processing(s, slab, object))
> + if (!alloc_debug_processing(s, slab, object, orig_size))
> /*
> * It's not really expected that this would fail on a
> * freshly allocated slab, but a concurrent memory
> @@ -2161,7 +2168,7 @@ static inline bool pfmemalloc_match(struct slab *slab, gfp_t gfpflags);
> * Try to allocate a partial slab from a specific node.
> */
> static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
> - struct slab **ret_slab, gfp_t gfpflags)
> + struct partial_context *pc)
> {
> struct slab *slab, *slab2;
> void *object = NULL;
> @@ -2181,11 +2188,11 @@ static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
> list_for_each_entry_safe(slab, slab2, &n->partial, slab_list) {
> void *t;
>
> - if (!pfmemalloc_match(slab, gfpflags))
> + if (!pfmemalloc_match(slab, pc->flags))
> continue;
>
> if (kmem_cache_debug(s)) {
> - object = alloc_single_from_partial(s, n, slab);
> + object = alloc_single_from_partial(s, n, slab, pc->orig_size);
> if (object)
> break;
> continue;
> @@ -2196,7 +2203,7 @@ static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
> break;
>
> if (!object) {
> - *ret_slab = slab;
> + *pc->slab = slab;
> stat(s, ALLOC_FROM_PARTIAL);
> object = t;
> } else {
> @@ -2220,14 +2227,13 @@ static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
> /*
> * Get a slab from somewhere. Search in increasing NUMA distances.
> */
> -static void *get_any_partial(struct kmem_cache *s, gfp_t flags,
> - struct slab **ret_slab)
> +static void *get_any_partial(struct kmem_cache *s, struct partial_context *pc)
> {
> #ifdef CONFIG_NUMA
> struct zonelist *zonelist;
> struct zoneref *z;
> struct zone *zone;
> - enum zone_type highest_zoneidx = gfp_zone(flags);
> + enum zone_type highest_zoneidx = gfp_zone(pc->flags);
> void *object;
> unsigned int cpuset_mems_cookie;
>
> @@ -2255,15 +2261,15 @@ static void *get_any_partial(struct kmem_cache *s, gfp_t flags,
>
> do {
> cpuset_mems_cookie = read_mems_allowed_begin();
> - zonelist = node_zonelist(mempolicy_slab_node(), flags);
> + zonelist = node_zonelist(mempolicy_slab_node(), pc->flags);
> for_each_zone_zonelist(zone, z, zonelist, highest_zoneidx) {
> struct kmem_cache_node *n;
>
> n = get_node(s, zone_to_nid(zone));
>
> - if (n && cpuset_zone_allowed(zone, flags) &&
> + if (n && cpuset_zone_allowed(zone, pc->flags) &&
> n->nr_partial > s->min_partial) {
> - object = get_partial_node(s, n, ret_slab, flags);
> + object = get_partial_node(s, n, pc);
> if (object) {
> /*
> * Don't check read_mems_allowed_retry()
> @@ -2284,8 +2290,7 @@ static void *get_any_partial(struct kmem_cache *s, gfp_t flags,
> /*
> * Get a partial slab, lock it and return it.
> */
> -static void *get_partial(struct kmem_cache *s, gfp_t flags, int node,
> - struct slab **ret_slab)
> +static void *get_partial(struct kmem_cache *s, int node, struct partial_context *pc)
> {
> void *object;
> int searchnode = node;
> @@ -2293,11 +2298,11 @@ static void *get_partial(struct kmem_cache *s, gfp_t flags, int node,
> if (node == NUMA_NO_NODE)
> searchnode = numa_mem_id();
>
> - object = get_partial_node(s, get_node(s, searchnode), ret_slab, flags);
> + object = get_partial_node(s, get_node(s, searchnode), pc);
> if (object || node != NUMA_NO_NODE)
> return object;
>
> - return get_any_partial(s, flags, ret_slab);
> + return get_any_partial(s, pc);
> }
>
> #ifdef CONFIG_PREEMPTION
> @@ -3022,6 +3027,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> void *freelist;
> struct slab *slab;
> unsigned long flags;
> + struct partial_context pc;
>
> stat(s, ALLOC_SLOWPATH);
>
> @@ -3135,7 +3141,10 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>
> new_objects:
>
> - freelist = get_partial(s, gfpflags, node, &slab);
> + pc.flags = gfpflags;
> + pc.slab = &slab;
> + pc.orig_size = orig_size;
> + freelist = get_partial(s, node, &pc);
> if (freelist)
> goto check_new_slab;
>
> @@ -3151,14 +3160,13 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> stat(s, ALLOC_SLAB);
>
> if (kmem_cache_debug(s)) {
> - freelist = alloc_single_from_new_slab(s, slab);
> + freelist = alloc_single_from_new_slab(s, slab, orig_size);
>
> if (unlikely(!freelist))
> goto new_objects;
>
> if (s->flags & SLAB_STORE_USER)
> set_track(s, freelist, TRACK_ALLOC, addr);
> - set_orig_size(s, freelist, orig_size);
>
> return freelist;
> }
> @@ -3184,7 +3192,6 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> */
> if (s->flags & SLAB_STORE_USER)
> set_track(s, freelist, TRACK_ALLOC, addr);
> - set_orig_size(s, freelist, orig_size);
>
> return freelist;
> }
--
Thanks,
Hyeonggon
