ACK/Cmnt: [SRU Focal/Groovy] UBUNTU: SAUCE: Revert "mm: memcg/slab: fix memory leak at non-root kmem_cache destroy"

[Stefan Bader]

On 16.09.20 12:46, Thadeu Lima de Souza Cascardo wrote:
> BugLink: https://bugs.launchpad.net/bugs/1894780
> 
> This reverts commit 79ffe7107b13042c69c4a06394175362121b06b5. This is
> commit d38a2b7a9c939e6d7329ab92b96559ccebf7b135 upstream.
> 
> Said commit causes same-sized kmemcaches to become unmergeable, and when a
> new kmemcache is created, it will fail creating the sysfs entry, making the
> kmemcache creation to fail.
> 
> Considering the original commit fix a leak but causes a different leak and
> failures to create kmemcaches, the revert is preferable until a proper fix
> is developed.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---

I agree with this revert but you have not really answered (at least it was not
obvious to me) what should happen to the revert we already have done in Focal.
Should that be brought back or also reverted in Groovy in addition to this new
revert?

-Stefan

>  mm/slab_common.c | 35 +++++++----------------------------
>  1 file changed, 7 insertions(+), 28 deletions(-)
> 
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index e36dd36c7076..8c1ffbf7de45 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -326,14 +326,6 @@ int slab_unmergeable(struct kmem_cache *s)
>  	if (s->refcount < 0)
>  		return 1;
>  
> -#ifdef CONFIG_MEMCG_KMEM
> -	/*
> -	 * Skip the dying kmem_cache.
> -	 */
> -	if (s->memcg_params.dying)
> -		return 1;
> -#endif
> -
>  	return 0;
>  }
>  
> @@ -894,15 +886,12 @@ static int shutdown_memcg_caches(struct kmem_cache *s)
>  	return 0;
>  }
>  
> -static void memcg_set_kmem_cache_dying(struct kmem_cache *s)
> +static void flush_memcg_workqueue(struct kmem_cache *s)
>  {
>  	spin_lock_irq(&memcg_kmem_wq_lock);
>  	s->memcg_params.dying = true;
>  	spin_unlock_irq(&memcg_kmem_wq_lock);
> -}
>  
> -static void flush_memcg_workqueue(struct kmem_cache *s)
> -{
>  	/*
>  	 * SLAB and SLUB deactivate the kmem_caches through call_rcu. Make
>  	 * sure all registered rcu callbacks have been invoked.
> @@ -934,6 +923,10 @@ static inline int shutdown_memcg_caches(struct kmem_cache *s)
>  {
>  	return 0;
>  }
> +
> +static inline void flush_memcg_workqueue(struct kmem_cache *s)
> +{
> +}
>  #endif /* CONFIG_MEMCG_KMEM */
>  
>  void slab_kmem_cache_release(struct kmem_cache *s)
> @@ -951,6 +944,8 @@ void kmem_cache_destroy(struct kmem_cache *s)
>  	if (unlikely(!s))
>  		return;
>  
> +	flush_memcg_workqueue(s);
> +
>  	get_online_cpus();
>  	get_online_mems();
>  
> @@ -960,22 +955,6 @@ void kmem_cache_destroy(struct kmem_cache *s)
>  	if (s->refcount)
>  		goto out_unlock;
>  
> -#ifdef CONFIG_MEMCG_KMEM
> -	memcg_set_kmem_cache_dying(s);
> -
> -	mutex_unlock(&slab_mutex);
> -
> -	put_online_mems();
> -	put_online_cpus();
> -
> -	flush_memcg_workqueue(s);
> -
> -	get_online_cpus();
> -	get_online_mems();
> -
> -	mutex_lock(&slab_mutex);
> -#endif
> -
>  	err = shutdown_memcg_caches(s);
>  	if (!err)
>  		err = shutdown_cache(s);
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200917/c53de8ff/attachment-0001.sig>