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

[Dmitry Safonov <dsafonov@virtuozzo.com>]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>