diff mbox series

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

Message ID 20200916104621.376598-1-cascardo@canonical.com
State New
Headers show
Series [SRU,Focal/Groovy] UBUNTU: SAUCE: Revert "mm: memcg/slab: fix memory leak at non-root kmem_cache destroy" | expand

Commit Message

Thadeu Lima de Souza Cascardo Sept. 16, 2020, 10:46 a.m. UTC
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@canonical.com>
---
 mm/slab_common.c | 35 +++++++----------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

Comments

Kleber Souza Sept. 16, 2020, 12:51 p.m. UTC | #1
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@canonical.com>

Hi Thadeu,

How does this interact with the other commit we reverted on Focal
(dde3c6b72a16 mm/slub: fix a memory leak in sysfs_slab_add())?
Should we keep that revert in Focal and also revert it in Groovy
or add this commit back to Focal?


Kleber

> ---
>  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);
>
Thadeu Lima de Souza Cascardo Sept. 16, 2020, 1:16 p.m. UTC | #2
On Wed, Sep 16, 2020 at 02:51:17PM +0200, Kleber Souza wrote:
> 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@canonical.com>
> 
> Hi Thadeu,
> 
> How does this interact with the other commit we reverted on Focal
> (dde3c6b72a16 mm/slub: fix a memory leak in sysfs_slab_add())?
> Should we keep that revert in Focal and also revert it in Groovy
> or add this commit back to Focal?
> 

We should keep the revert and fix it some other way upstream. But I'd rather
have a leak than the crash caused by the double free.

Cascardo.

> 
> Kleber
> 
> > ---
> >  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);
> > 
>
Stefan Bader Sept. 17, 2020, 7:34 a.m. UTC | #3
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@canonical.com>
Acked-by: Stefan Bader <stefan.bader@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);
>
Thadeu Lima de Souza Cascardo Sept. 17, 2020, 11 a.m. UTC | #4
On Thu, Sep 17, 2020 at 09:34:20AM +0200, Stefan Bader wrote:
> 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@canonical.com>
> Acked-by: Stefan Bader <stefan.bader@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
> 

I wrote:

> We should keep the revert and fix it some other way upstream. But I'd rather
> have a leak than the crash caused by the double free.

To be clear, we should also revert that on Groovy, until we have a better fix.
In case kobject_init_and_add fails, the reverted commit would cause a
double-free, leading to the crash that was reported on LP: #1894780. This
second commit we are reverting was causing kobject_init_and_add to return
EEXIST, but there are other reasons kboject_init_and_add would fail. And I
would rather have the memleak that the first commit claims to fix than the
double free it causes.

I will prepare a fix for Groovy (or just point Seth to the one that was already
sent), and work on an alternative fix.

Thanks.
Cascardo.


> >  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);
> > 
> 
>
Ian May Sept. 17, 2020, 5:40 p.m. UTC | #5
Acked-by: Ian May <ian.may@canonical.com>

On 2020-09-16 07:46:21 , 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@canonical.com>
> ---
>  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);
> -- 
> 2.25.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Ian May Sept. 17, 2020, 11:19 p.m. UTC | #6
Applied to Focal/master-next. Thanks!

Ian

On 2020-09-16 07:46:21 , 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@canonical.com>
> ---
>  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);
> -- 
> 2.25.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Paolo Pisati Sept. 29, 2020, 3:06 p.m. UTC | #7
On Thu, Sep 17, 2020 at 08:00:09AM -0300, Thadeu Lima de Souza Cascardo wrote:
> 
> I will prepare a fix for Groovy (or just point Seth to the one that was already
> sent), and work on an alternative fix.

We miss this fix in Groovy: is this patch sufficient? From the conversation, it
appears some other patch was necessary.
Thadeu Lima de Souza Cascardo Sept. 29, 2020, 3:45 p.m. UTC | #8
On Tue, Sep 29, 2020 at 05:06:29PM +0200, Paolo Pisati wrote:
> On Thu, Sep 17, 2020 at 08:00:09AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > 
> > I will prepare a fix for Groovy (or just point Seth to the one that was already
> > sent), and work on an alternative fix.
> 
> We miss this fix in Groovy: is this patch sufficient? From the conversation, it
> appears some other patch was necessary.
> -- 
> bye,
> p.

You should apply this one and this other one:
https://lists.ubuntu.com/archives/kernel-team/2020-September/113343.html
[PATCH 1/1] UBUNTU: SAUCE: Revert "mm/slub: fix a memory leak in sysfs_slab_add()"

Cascardo.
Paolo Pisati Sept. 29, 2020, 4 p.m. UTC | #9
On Tue, Sep 29, 2020 at 12:45:39PM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Sep 29, 2020 at 05:06:29PM +0200, Paolo Pisati wrote:
> > On Thu, Sep 17, 2020 at 08:00:09AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > > 
> > > I will prepare a fix for Groovy (or just point Seth to the one that was already
> > > sent), and work on an alternative fix.
> > 
> > We miss this fix in Groovy: is this patch sufficient? From the conversation, it
> > appears some other patch was necessary.
> > -- 
> > bye,
> > p.
> 
> You should apply this one and this other one:
> https://lists.ubuntu.com/archives/kernel-team/2020-September/113343.html
> [PATCH 1/1] UBUNTU: SAUCE: Revert "mm/slub: fix a memory leak in sysfs_slab_add()"
> 
> Cascardo.
diff mbox series

Patch

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);