Re: [PATCH] mm: slab: free kmem_cache_node after destroy sysfs file

[Dmitry Safonov]

On 02/07/2016 10:10 PM, Vladimir Davydov wrote:
> On Fri, Feb 05, 2016 at 08:16:52PM +0300, Dmitry Safonov wrote:
> ...
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 6ecc697..41176dd 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -2414,13 +2414,19 @@ int __kmem_cache_shrink(struct kmem_cache *cachep, bool deactivate)
> >   
> >   int __kmem_cache_shutdown(struct kmem_cache *cachep)
> >   {
> > -	int i;
> > -	struct kmem_cache_node *n;
> >   	int rc = __kmem_cache_shrink(cachep, false);
> >   
> >   	if (rc)
> >   		return rc;
> Nit:
>
>   int __kmem_cache_shutdown(struct kmem_cache *cachep)
>   {
> -	int rc = __kmem_cache_shrink(cachep, false);
> -
> -	if (rc)
> -		return rc;
> -
> -	return 0;
> +	return __kmem_cache_shrink(cachep, false);
>   }
Will do
> >   
> > +	return 0;
> > +}
> > +
> > +void __kmem_cache_release(struct kmem_cache *cachep)
> > +{
> > +	int i;
> > +	struct kmem_cache_node *n;
> > +
> >   	free_percpu(cachep->cpu_cache);
> >   
> >   	/* NUMA: free the node structures */
> > @@ -2430,7 +2436,6 @@ int __kmem_cache_shutdown(struct kmem_cache *cachep)
> >   		kfree(n);
> >   		cachep->node[i] = NULL;
> >   	}
> > -	return 0;
> >   }
> >   
> >   /*
> You seem to forget to replace __kmem_cache_shutdown with
> __kmem_cache_release in __kmem_cache_create error path:
>
> @@ -2168,7 +2168,7 @@ done:
>   
>   	err = setup_cpu_cache(cachep, gfp);
>   	if (err) {
> -		__kmem_cache_shutdown(cachep);
> +		__kmem_cache_release(cachep);
>   		return err;
>   	}
>
> ...
Yeah, thanks
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 2e1355a..ce21ce2 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -3173,11 +3173,12 @@ static void early_kmem_cache_node_alloc(int node)
> >   	__add_partial(n, page, DEACTIVATE_TO_HEAD);
> >   }
> >   
> > -static void free_kmem_cache_nodes(struct kmem_cache *s)
> > +void __kmem_cache_release(struct kmem_cache *s)
> >   {
> >   	int node;
> >   	struct kmem_cache_node *n;
> >   
> > +	free_percpu(s->cpu_slab);
> That's rather nit-picking, but this kinda disrupts
> init_kmem_cache_nodes/free_kmem_cache_nodes symmetry.
> I'd leave free_kmem_cache_nodes alone and make
> __kmem_cache_release call it along with free_percpu.
> This would also reduce the patch footprint, because
> the two hunks below wouldn't be needed.
Ok
> >   	for_each_kmem_cache_node(s, node, n) {
> >   		kmem_cache_free(kmem_cache_node, n);
> >   		s->node[node] = NULL;
> > @@ -3199,7 +3200,7 @@ static int init_kmem_cache_nodes(struct kmem_cache *s)
> >   						GFP_KERNEL, node);
> >   
> >   		if (!n) {
> > -			free_kmem_cache_nodes(s);
> > +			__kmem_cache_release(s);
> >   			return 0;
> >   		}
> >   
> > @@ -3405,7 +3406,7 @@ static int kmem_cache_open(struct kmem_cache *s, unsigned long flags)
> >   	if (alloc_kmem_cache_cpus(s))
> >   		return 0;
> >   
> > -	free_kmem_cache_nodes(s);
> > +	__kmem_cache_release(s);
> >   error:
> >   	if (flags & SLAB_PANIC)
> >   		panic("Cannot create slab %s size=%lu realsize=%u "
> > @@ -3443,7 +3444,7 @@ static void list_slab_objects(struct kmem_cache *s, struct page *page,
> >   
> >   /*
> >    * Attempt to free all partial slabs on a node.
> > - * This is called from kmem_cache_close(). We must be the last thread
> > + * This is called from __kmem_cache_shutdown(). We must be the last thread
> >    * using the cache and therefore we do not need to lock anymore.
> Well, that's not true as we've found out - sysfs might still access the
> cache in parallel. And alloc_calls_show -> list_locations does walk over
> the kmem_cache_node->partial list, which we prune on shutdown.
>
> I guess we should reintroduce locking for free_partial() in the scope of
> this patch, partially reverting 69cb8e6b7c298.
I think, we can omit locking for !SLAB_SUPPORTS_SYSFS and reintroduce
for sysfs case. Will do
> >    */
> >   static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
> > @@ -3456,7 +3457,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
> >   			discard_slab(s, page);
> >   		} else {
> >   			list_slab_objects(s, page,
> > -			"Objects remaining in %s on kmem_cache_close()");
> > +			"Objects remaining in %s on __kmem_cache_shutdown()");
> >   		}
> >   	}
> >   }
> > @@ -3464,7 +3465,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
> >   /*
> >    * Release all resources used by a slab cache.
> >    */
> > -static inline int kmem_cache_close(struct kmem_cache *s)
> > +int __kmem_cache_shutdown(struct kmem_cache *s)
> >   {
> >   	int node;
> >   	struct kmem_cache_node *n;
> > @@ -3476,16 +3477,9 @@ static inline int kmem_cache_close(struct kmem_cache *s)
> >   		if (n->nr_partial || slabs_node(s, node))
> >   			return 1;
> >   	}
> > -	free_percpu(s->cpu_slab);
> > -	free_kmem_cache_nodes(s);
> >   	return 0;
> >   }
> >   
> > -int __kmem_cache_shutdown(struct kmem_cache *s)
> > -{
> > -	return kmem_cache_close(s);
> > -}
> > -
> >   /********************************************************************
> >    *		Kmalloc subsystem
> >    *******************************************************************/
> > @@ -3979,8 +3973,10 @@ int __kmem_cache_create(struct kmem_cache *s, unsigned long flags)
> >   
> >   	memcg_propagate_slab_attrs(s);
> >   	err = sysfs_slab_add(s);
> > -	if (err)
> > -		kmem_cache_close(s);
> > +	if (err) {
> > +		__kmem_cache_shutdown(s);
> > +		__kmem_cache_release(s);
> > +	}
> No point calling __kmem_cache_shutdown on __kmem_cache_create error path
> - the cache hasn't been used yet.
Oh, yes. Thanks for review.
> Thanks,
> Vladimir


--
Regards,
Dmitry Safonov