[PATCH 0/3] mm/slub: Fix slabs_node return value

[PATCH 0/3] mm/slub: Fix slabs_node return value

[Muchun Song]

The slabs_node() always return zero when CONFIG_SLUB_DEBUG is disabled.
But some codes determine whether slab is empty by checking the return
value of slabs_node(). As you know, the result is not correct. we move
the nr_slabs of kmem_cache_node out of the CONFIG_SLUB_DEBUG. So we can
get the corrent value returned by the slabs_node().

Muchun Song (3):
  mm/slub: Fix slabs_node return value when CONFIG_SLUB_DEBUG disabled
  mm/slub: Use node_nr_slabs() instead of slabs_node()
  mm/slub: Fix release all resources used by a slab cache

 mm/slab.h |  2 +-
 mm/slub.c | 93 +++++++++++++++++++++++++++++++++------------------------------
 2 files changed, 50 insertions(+), 45 deletions(-)

-- 
2.11.0

[PATCH 1/3] mm/slub: Fix slabs_node return value when CONFIG_SLUB_DEBUG disabled

[Muchun Song]

The slabs_node() always return zero when CONFIG_SLUB_DEBUG is disabled.
But some codes determine whether slab is empty by checking the return
value of slabs_node(). As you know, the result is not correct. This
problem can be reproduce by the follow code(and boot system with the
cmdline of "slub_nomerge"):

  void *objs[32];
  struct kmem_cache *cache = kmem_cache_create("kmem-test", 128, 0,
			0, 0);

  if (cache) {
  	int i;

	/* Make a full slab */
  	for (i = 0; i < ARRAY_SIZE(objs); i++)
		objs[i] = kmem_cache_alloc(cache, GFP_KERNEL_ACCOUNT);

	/*
  	 * This really should fail because the slab cache still has
  	 * objects. But we did destroy the @cache because of zero
  	 * returned by slabs_node().
  	 */
  	kmem_cache_destroy(cache);
  }

To fix it, we can move the nr_slabs of kmem_cache_node out of the
CONFIG_SLUB_DEBUG. So we can get the corrent value returned by the
slabs_node().

With this patch applied, we will get a warning message and stack
trace in the dmesg.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/slab.h |  2 +-
 mm/slub.c | 80 +++++++++++++++++++++++++++++++++------------------------------
 2 files changed, 43 insertions(+), 39 deletions(-)

diff --git a/mm/slab.h b/mm/slab.h
index 0b91f2a7b033..062d4542b7e2 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -619,8 +619,8 @@ struct kmem_cache_node {
 #ifdef CONFIG_SLUB
 	unsigned long nr_partial;
 	struct list_head partial;
-#ifdef CONFIG_SLUB_DEBUG
 	atomic_long_t nr_slabs;
+#ifdef CONFIG_SLUB_DEBUG
 	atomic_long_t total_objects;
 	struct list_head full;
 #endif
diff --git a/mm/slub.c b/mm/slub.c
index 49b5cb7da318..1a3e6a5b7287 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1070,39 +1070,14 @@ static void remove_full(struct kmem_cache *s, struct kmem_cache_node *n, struct
 	list_del(&page->slab_list);
 }
 
-/* Tracking of the number of slabs for debugging purposes */
-static inline unsigned long slabs_node(struct kmem_cache *s, int node)
+/* Tracking of the number of objects for debugging purposes */
+static inline void inc_objects_node(struct kmem_cache_node *n, int objects)
 {
-	struct kmem_cache_node *n = get_node(s, node);
-
-	return atomic_long_read(&n->nr_slabs);
+	atomic_long_add(objects, &n->total_objects);
 }
 
-static inline unsigned long node_nr_slabs(struct kmem_cache_node *n)
+static inline void dec_objects_node(struct kmem_cache_node *n, int objects)
 {
-	return atomic_long_read(&n->nr_slabs);
-}
-
-static inline void inc_slabs_node(struct kmem_cache *s, int node, int objects)
-{
-	struct kmem_cache_node *n = get_node(s, node);
-
-	/*
-	 * May be called early in order to allocate a slab for the
-	 * kmem_cache_node structure. Solve the chicken-egg
-	 * dilemma by deferring the increment of the count during
-	 * bootstrap (see early_kmem_cache_node_alloc).
-	 */
-	if (likely(n)) {
-		atomic_long_inc(&n->nr_slabs);
-		atomic_long_add(objects, &n->total_objects);
-	}
-}
-static inline void dec_slabs_node(struct kmem_cache *s, int node, int objects)
-{
-	struct kmem_cache_node *n = get_node(s, node);
-
-	atomic_long_dec(&n->nr_slabs);
 	atomic_long_sub(objects, &n->total_objects);
 }
 
@@ -1413,15 +1388,8 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
 
 #define disable_higher_order_debug 0
 
-static inline unsigned long slabs_node(struct kmem_cache *s, int node)
-							{ return 0; }
-static inline unsigned long node_nr_slabs(struct kmem_cache_node *n)
-							{ return 0; }
-static inline void inc_slabs_node(struct kmem_cache *s, int node,
-							int objects) {}
-static inline void dec_slabs_node(struct kmem_cache *s, int node,
-							int objects) {}
-
+static inline void inc_objects_node(struct kmem_cache_node *n, int objects) {}
+static inline void dec_objects_node(struct kmem_cache_node *n, int objects) {}
 static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
 			       void *freelist, void *nextfree)
 {
@@ -1429,6 +1397,42 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
 }
 #endif /* CONFIG_SLUB_DEBUG */
 
+static inline unsigned long slabs_node(struct kmem_cache *s, int node)
+{
+	struct kmem_cache_node *n = get_node(s, node);
+
+	return atomic_long_read(&n->nr_slabs);
+}
+
+static inline unsigned long node_nr_slabs(struct kmem_cache_node *n)
+{
+	return atomic_long_read(&n->nr_slabs);
+}
+
+static inline void inc_slabs_node(struct kmem_cache *s, int node, int objects)
+{
+	struct kmem_cache_node *n = get_node(s, node);
+
+	/*
+	 * May be called early in order to allocate a slab for the
+	 * kmem_cache_node structure. Solve the chicken-egg
+	 * dilemma by deferring the increment of the count during
+	 * bootstrap (see early_kmem_cache_node_alloc).
+	 */
+	if (likely(n)) {
+		atomic_long_inc(&n->nr_slabs);
+		inc_objects_node(n, objects);
+	}
+}
+
+static inline void dec_slabs_node(struct kmem_cache *s, int node, int objects)
+{
+	struct kmem_cache_node *n = get_node(s, node);
+
+	atomic_long_dec(&n->nr_slabs);
+	dec_objects_node(n, objects);
+}
+
 /*
  * Hooks for other subsystems that check memory allocations. In a typical
  * production configuration these hooks all should produce no code at all.
-- 
2.11.0

[PATCH 2/3] mm/slub: Use node_nr_slabs() instead of slabs_node()

[Muchun Song]

In the some code, we already get the kmem_cache_node, so we can
use node_nr_slabs() directly instead of slabs_node(). Check the
condition of n->nr_partial can also be removed because we can get
the correct result via node_nr_slabs().

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/slub.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 1a3e6a5b7287..b73505df3de2 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3829,7 +3829,7 @@ bool __kmem_cache_empty(struct kmem_cache *s)
 	struct kmem_cache_node *n;
 
 	for_each_kmem_cache_node(s, node, n)
-		if (n->nr_partial || slabs_node(s, node))
+		if (node_nr_slabs(n))
 			return false;
 	return true;
 }
@@ -3846,7 +3846,7 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
 	/* Attempt to free all objects */
 	for_each_kmem_cache_node(s, node, n) {
 		free_partial(s, n);
-		if (n->nr_partial || slabs_node(s, node))
+		if (node_nr_slabs(n))
 			return 1;
 	}
 	sysfs_slab_remove(s);
@@ -4126,7 +4126,7 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 		list_for_each_entry_safe(page, t, &discard, slab_list)
 			discard_slab(s, page);
 
-		if (slabs_node(s, node))
+		if (node_nr_slabs(n))
 			ret = 1;
 	}
 
@@ -4201,7 +4201,7 @@ static void slab_mem_offline_callback(void *arg)
 			 * and offline_pages() function shouldn't call this
 			 * callback. So, we must fail.
 			 */
-			BUG_ON(slabs_node(s, offline_node));
+			BUG_ON(node_nr_slabs(n));
 
 			s->node[offline_node] = NULL;
 			kmem_cache_free(kmem_cache_node, n);
-- 
2.11.0

[PATCH 3/3] mm/slub: Fix release all resources used by a slab cache

[Muchun Song]

The function of __kmem_cache_shutdown() is that release all resources
used by the slab cache, while currently it stop release resources when
the preceding node is not empty.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/slub.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index b73505df3de2..4e477ef0f2b9 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s)
  */
 int __kmem_cache_shutdown(struct kmem_cache *s)
 {
+	int ret = 0;
 	int node;
 	struct kmem_cache_node *n;
 
@@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
 	/* Attempt to free all objects */
 	for_each_kmem_cache_node(s, node, n) {
 		free_partial(s, n);
-		if (node_nr_slabs(n))
-			return 1;
+		if (!ret && node_nr_slabs(n))
+			ret = 1;
 	}
 	sysfs_slab_remove(s);
-	return 0;
+	return ret;
 }
 
 /********************************************************************
-- 
2.11.0

Re: [PATCH 1/3] mm/slub: Fix slabs_node return value when CONFIG_SLUB_DEBUG disabled

[Joonsoo Kim]

2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성:
>
> The slabs_node() always return zero when CONFIG_SLUB_DEBUG is disabled.
> But some codes determine whether slab is empty by checking the return
> value of slabs_node(). As you know, the result is not correct. This
> problem can be reproduce by the follow code(and boot system with the
> cmdline of "slub_nomerge"):
>
>   void *objs[32];
>   struct kmem_cache *cache = kmem_cache_create("kmem-test", 128, 0,
>                         0, 0);
>
>   if (cache) {
>         int i;
>
>         /* Make a full slab */
>         for (i = 0; i < ARRAY_SIZE(objs); i++)
>                 objs[i] = kmem_cache_alloc(cache, GFP_KERNEL_ACCOUNT);
>
>         /*
>          * This really should fail because the slab cache still has
>          * objects. But we did destroy the @cache because of zero
>          * returned by slabs_node().
>          */
>         kmem_cache_destroy(cache);
>   }
>
> To fix it, we can move the nr_slabs of kmem_cache_node out of the
> CONFIG_SLUB_DEBUG. So we can get the corrent value returned by the
> slabs_node().
>
> With this patch applied, we will get a warning message and stack
> trace in the dmesg.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/slab.h |  2 +-
>  mm/slub.c | 80 +++++++++++++++++++++++++++++++++------------------------------
>  2 files changed, 43 insertions(+), 39 deletions(-)
>
> diff --git a/mm/slab.h b/mm/slab.h
> index 0b91f2a7b033..062d4542b7e2 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -619,8 +619,8 @@ struct kmem_cache_node {
>  #ifdef CONFIG_SLUB
>         unsigned long nr_partial;
>         struct list_head partial;
> -#ifdef CONFIG_SLUB_DEBUG
>         atomic_long_t nr_slabs;
> +#ifdef CONFIG_SLUB_DEBUG
>         atomic_long_t total_objects;
>         struct list_head full;
>  #endif
> diff --git a/mm/slub.c b/mm/slub.c
> index 49b5cb7da318..1a3e6a5b7287 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c

Hello,

You also need to initialize nr_slabs in init_kmem_cache_node()
on !CONFIG_SLUB_DEBUG.

Otherwise, looks good to me.

Thanks.

Re: [PATCH 2/3] mm/slub: Use node_nr_slabs() instead of slabs_node()

[Joonsoo Kim]

2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성:
>
> In the some code, we already get the kmem_cache_node, so we can
> use node_nr_slabs() directly instead of slabs_node(). Check the
> condition of n->nr_partial can also be removed because we can get
> the correct result via node_nr_slabs().
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>

If appying this patch, there is no reference to slabs_node(). Please remove it.

Thanks.

Re: [External] Re: [PATCH 2/3] mm/slub: Use node_nr_slabs() instead of slabs_node()

[Muchun Song]

On Mon, Jun 15, 2020 at 2:15 PM Joonsoo Kim <js1304@gmail.com> wrote:
>
> 2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성:
> >
> > In the some code, we already get the kmem_cache_node, so we can
> > use node_nr_slabs() directly instead of slabs_node(). Check the
> > condition of n->nr_partial can also be removed because we can get
> > the correct result via node_nr_slabs().
> >
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>
> If appying this patch, there is no reference to slabs_node(). Please remove it.
>
> Thanks.

OK, Thanks. I will remove it in the next version.

-- 
Yours,
Muchun

Re: [PATCH 3/3] mm/slub: Fix release all resources used by a slab cache

[Joonsoo Kim]

2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성:
>
> The function of __kmem_cache_shutdown() is that release all resources
> used by the slab cache, while currently it stop release resources when
> the preceding node is not empty.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/slub.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index b73505df3de2..4e477ef0f2b9 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s)
>   */
>  int __kmem_cache_shutdown(struct kmem_cache *s)
>  {
> +       int ret = 0;
>         int node;
>         struct kmem_cache_node *n;
>
> @@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
>         /* Attempt to free all objects */
>         for_each_kmem_cache_node(s, node, n) {
>                 free_partial(s, n);
> -               if (node_nr_slabs(n))
> -                       return 1;
> +               if (!ret && node_nr_slabs(n))
> +                       ret = 1;
>         }

I don't think that this is an improvement.

If the shutdown condition isn't met, we don't need to process further.
Just 'return 1' looks okay to me.

And, with this change, sysfs_slab_remove() is called even if the
shutdown is failed.
It's better not to have side effects when failing.

Thanks.

Re: [External] Re: [PATCH 1/3] mm/slub: Fix slabs_node return value when CONFIG_SLUB_DEBUG disabled

[Muchun Song]

On Mon, Jun 15, 2020 at 2:11 PM Joonsoo Kim <js1304@gmail.com> wrote:
>
> 2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성:
> >
> > The slabs_node() always return zero when CONFIG_SLUB_DEBUG is disabled.
> > But some codes determine whether slab is empty by checking the return
> > value of slabs_node(). As you know, the result is not correct. This
> > problem can be reproduce by the follow code(and boot system with the
> > cmdline of "slub_nomerge"):
> >
> >   void *objs[32];
> >   struct kmem_cache *cache = kmem_cache_create("kmem-test", 128, 0,
> >                         0, 0);
> >
> >   if (cache) {
> >         int i;
> >
> >         /* Make a full slab */
> >         for (i = 0; i < ARRAY_SIZE(objs); i++)
> >                 objs[i] = kmem_cache_alloc(cache, GFP_KERNEL_ACCOUNT);
> >
> >         /*
> >          * This really should fail because the slab cache still has
> >          * objects. But we did destroy the @cache because of zero
> >          * returned by slabs_node().
> >          */
> >         kmem_cache_destroy(cache);
> >   }
> >
> > To fix it, we can move the nr_slabs of kmem_cache_node out of the
> > CONFIG_SLUB_DEBUG. So we can get the corrent value returned by the
> > slabs_node().
> >
> > With this patch applied, we will get a warning message and stack
> > trace in the dmesg.
> >
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  mm/slab.h |  2 +-
> >  mm/slub.c | 80 +++++++++++++++++++++++++++++++++------------------------------
> >  2 files changed, 43 insertions(+), 39 deletions(-)
> >
> > diff --git a/mm/slab.h b/mm/slab.h
> > index 0b91f2a7b033..062d4542b7e2 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -619,8 +619,8 @@ struct kmem_cache_node {
> >  #ifdef CONFIG_SLUB
> >         unsigned long nr_partial;
> >         struct list_head partial;
> > -#ifdef CONFIG_SLUB_DEBUG
> >         atomic_long_t nr_slabs;
> > +#ifdef CONFIG_SLUB_DEBUG
> >         atomic_long_t total_objects;
> >         struct list_head full;
> >  #endif
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 49b5cb7da318..1a3e6a5b7287 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
>
> Hello,
>
> You also need to initialize nr_slabs in init_kmem_cache_node()
> on !CONFIG_SLUB_DEBUG.

Good catch, thanks! I will fix it in the next version.

>
> Otherwise, looks good to me.
>
> Thanks.



-- 
Yours,
Muchun

Re: [External] Re: [PATCH 3/3] mm/slub: Fix release all resources used by a slab cache

[Muchun Song]

On Mon, Jun 15, 2020 at 2:23 PM Joonsoo Kim <js1304@gmail.com> wrote:
>
> 2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성:
> >
> > The function of __kmem_cache_shutdown() is that release all resources
> > used by the slab cache, while currently it stop release resources when
> > the preceding node is not empty.
> >
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  mm/slub.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index b73505df3de2..4e477ef0f2b9 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s)
> >   */
> >  int __kmem_cache_shutdown(struct kmem_cache *s)
> >  {
> > +       int ret = 0;
> >         int node;
> >         struct kmem_cache_node *n;
> >
> > @@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
> >         /* Attempt to free all objects */
> >         for_each_kmem_cache_node(s, node, n) {
> >                 free_partial(s, n);
> > -               if (node_nr_slabs(n))
> > -                       return 1;
> > +               if (!ret && node_nr_slabs(n))
> > +                       ret = 1;
> >         }
>
> I don't think that this is an improvement.
>
> If the shutdown condition isn't met, we don't need to process further.
> Just 'return 1' looks okay to me.
>
> And, with this change, sysfs_slab_remove() is called even if the
> shutdown is failed.
> It's better not to have side effects when failing.

If someone calls __kmem_cache_shutdown, he may want to release
resources used by the slab cache as much as possible. If we continue,
we may release more pages. From this point, is it an improvement?

-- 
Yours,
Muchun

Re: [External] Re: [PATCH 3/3] mm/slub: Fix release all resources used by a slab cache

[Joonsoo Kim]

2020년 6월 15일 (월) 오후 3:41, Muchun Song <songmuchun@bytedance.com>님이 작성:
>
> On Mon, Jun 15, 2020 at 2:23 PM Joonsoo Kim <js1304@gmail.com> wrote:
> >
> > 2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성:
> > >
> > > The function of __kmem_cache_shutdown() is that release all resources
> > > used by the slab cache, while currently it stop release resources when
> > > the preceding node is not empty.
> > >
> > > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > > ---
> > >  mm/slub.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/mm/slub.c b/mm/slub.c
> > > index b73505df3de2..4e477ef0f2b9 100644
> > > --- a/mm/slub.c
> > > +++ b/mm/slub.c
> > > @@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s)
> > >   */
> > >  int __kmem_cache_shutdown(struct kmem_cache *s)
> > >  {
> > > +       int ret = 0;
> > >         int node;
> > >         struct kmem_cache_node *n;
> > >
> > > @@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
> > >         /* Attempt to free all objects */
> > >         for_each_kmem_cache_node(s, node, n) {
> > >                 free_partial(s, n);
> > > -               if (node_nr_slabs(n))
> > > -                       return 1;
> > > +               if (!ret && node_nr_slabs(n))
> > > +                       ret = 1;
> > >         }
> >
> > I don't think that this is an improvement.
> >
> > If the shutdown condition isn't met, we don't need to process further.
> > Just 'return 1' looks okay to me.
> >
> > And, with this change, sysfs_slab_remove() is called even if the
> > shutdown is failed.
> > It's better not to have side effects when failing.
>
> If someone calls __kmem_cache_shutdown, he may want to release
> resources used by the slab cache as much as possible. If we continue,
> we may release more pages. From this point, is it an improvement?

My opinion is not strong but I still think that it's not useful enough
to complicate
the code.

If shutdown is failed, it implies there are some bugs and someone should fix it.
Releasing more resources would mitigate the resource problem but doesn't
change the situation that someone should fix it soon.

Anyway, I don't object more if you don't agree with my opinion. In that case,
please fix not to call sysfs_slab_remove() when ret is 1.

Thanks.

Re: [External] Re: [PATCH 3/3] mm/slub: Fix release all resources used by a slab cache

[Muchun Song]

On Mon, Jun 15, 2020 at 3:25 PM Joonsoo Kim <js1304@gmail.com> wrote:
>
> 2020년 6월 15일 (월) 오후 3:41, Muchun Song <songmuchun@bytedance.com>님이 작성:
> >
> > On Mon, Jun 15, 2020 at 2:23 PM Joonsoo Kim <js1304@gmail.com> wrote:
> > >
> > > 2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성:
> > > >
> > > > The function of __kmem_cache_shutdown() is that release all resources
> > > > used by the slab cache, while currently it stop release resources when
> > > > the preceding node is not empty.
> > > >
> > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > > > ---
> > > >  mm/slub.c | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/mm/slub.c b/mm/slub.c
> > > > index b73505df3de2..4e477ef0f2b9 100644
> > > > --- a/mm/slub.c
> > > > +++ b/mm/slub.c
> > > > @@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s)
> > > >   */
> > > >  int __kmem_cache_shutdown(struct kmem_cache *s)
> > > >  {
> > > > +       int ret = 0;
> > > >         int node;
> > > >         struct kmem_cache_node *n;
> > > >
> > > > @@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
> > > >         /* Attempt to free all objects */
> > > >         for_each_kmem_cache_node(s, node, n) {
> > > >                 free_partial(s, n);
> > > > -               if (node_nr_slabs(n))
> > > > -                       return 1;
> > > > +               if (!ret && node_nr_slabs(n))
> > > > +                       ret = 1;
> > > >         }
> > >
> > > I don't think that this is an improvement.
> > >
> > > If the shutdown condition isn't met, we don't need to process further.
> > > Just 'return 1' looks okay to me.
> > >
> > > And, with this change, sysfs_slab_remove() is called even if the
> > > shutdown is failed.
> > > It's better not to have side effects when failing.
> >
> > If someone calls __kmem_cache_shutdown, he may want to release
> > resources used by the slab cache as much as possible. If we continue,
> > we may release more pages. From this point, is it an improvement?
>
> My opinion is not strong but I still think that it's not useful enough
> to complicate
> the code.
>
> If shutdown is failed, it implies there are some bugs and someone should fix it.

Yeah, I agree with you.

> Releasing more resources would mitigate the resource problem but doesn't
> change the situation that someone should fix it soon.
>
> Anyway, I don't object more if you don't agree with my opinion. In that case,
> please fix not to call sysfs_slab_remove() when ret is 1.
>

Yeah, we should call sysfs_slab_remove only when ret is zero. Thanks very
much.

-- 
Yours,
Muchun

Re: [PATCH 1/3] mm/slub: Fix slabs_node return value when CONFIG_SLUB_DEBUG disabled

[Vlastimil Babka]

On 6/14/20 2:39 PM, Muchun Song wrote:
> The slabs_node() always return zero when CONFIG_SLUB_DEBUG is disabled.
> But some codes determine whether slab is empty by checking the return
> value of slabs_node(). As you know, the result is not correct. This
> problem can be reproduce by the follow code(and boot system with the
> cmdline of "slub_nomerge"):
> 
>   void *objs[32];
>   struct kmem_cache *cache = kmem_cache_create("kmem-test", 128, 0,
> 			0, 0);
> 
>   if (cache) {
>   	int i;
> 
> 	/* Make a full slab */
>   	for (i = 0; i < ARRAY_SIZE(objs); i++)
> 		objs[i] = kmem_cache_alloc(cache, GFP_KERNEL_ACCOUNT);
> 
> 	/*
>   	 * This really should fail because the slab cache still has
>   	 * objects. But we did destroy the @cache because of zero
>   	 * returned by slabs_node().
>   	 */
>   	kmem_cache_destroy(cache);
>   }

Hmm, the whole SLUB subsystem (or even kernel) has some trust in the other
individual subsystems doing the right thing. I.e. if allocate an object once and
free it twice, bad things will happen. If you are debugging such a problem, you
enable SLUB_DEBUG (and the boot parameter).

This particular situation is the same - SLUB trusts its users to free their
objects before calling kmem_cache_destroy(). If they don't and just leak them,
then they stay leaked and nothing worse will happen (I'd expect). If they
destroy the cache and later try to e.g. free the objects, then there will most
likely be a crash.

And yeah, if you are debugging such thing, you will compile with SLUB_DEBUG and
make sure there's slub_nomerge (or you won't catch it anyway).

Hmm, but there's one exception in __kmem_cache_shrink() where the caller wants
to know if all slabs were discarded. kmem_cache_shrink() passes that to its
callers, but seems nobody cares about the return value.

The only caller that seems to care is __kmemcg_cache_deactivate_after_rcu():

        if (!__kmem_cache_shrink(s))
                sysfs_slab_remove(s);

Perhaps that will go away with memcg rework? (CC Roman). If yes, we also have
the option to stop returning the value in kmem_cache_shrink() (as nobody seems
to care), and then slabs_node() remains a debug only thing.

> To fix it, we can move the nr_slabs of kmem_cache_node out of the
> CONFIG_SLUB_DEBUG. So we can get the corrent value returned by the
> slabs_node().
> 
> With this patch applied, we will get a warning message and stack
> trace in the dmesg.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/slab.h |  2 +-
>  mm/slub.c | 80 +++++++++++++++++++++++++++++++++------------------------------
>  2 files changed, 43 insertions(+), 39 deletions(-)
> 
> diff --git a/mm/slab.h b/mm/slab.h
> index 0b91f2a7b033..062d4542b7e2 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -619,8 +619,8 @@ struct kmem_cache_node {
>  #ifdef CONFIG_SLUB
>  	unsigned long nr_partial;
>  	struct list_head partial;
> -#ifdef CONFIG_SLUB_DEBUG
>  	atomic_long_t nr_slabs;
> +#ifdef CONFIG_SLUB_DEBUG
>  	atomic_long_t total_objects;
>  	struct list_head full;
>  #endif
> diff --git a/mm/slub.c b/mm/slub.c
> index 49b5cb7da318..1a3e6a5b7287 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1070,39 +1070,14 @@ static void remove_full(struct kmem_cache *s, struct kmem_cache_node *n, struct
>  	list_del(&page->slab_list);
>  }
>  
> -/* Tracking of the number of slabs for debugging purposes */
> -static inline unsigned long slabs_node(struct kmem_cache *s, int node)
> +/* Tracking of the number of objects for debugging purposes */
> +static inline void inc_objects_node(struct kmem_cache_node *n, int objects)
>  {
> -	struct kmem_cache_node *n = get_node(s, node);
> -
> -	return atomic_long_read(&n->nr_slabs);
> +	atomic_long_add(objects, &n->total_objects);
>  }
>  
> -static inline unsigned long node_nr_slabs(struct kmem_cache_node *n)
> +static inline void dec_objects_node(struct kmem_cache_node *n, int objects)
>  {
> -	return atomic_long_read(&n->nr_slabs);
> -}
> -
> -static inline void inc_slabs_node(struct kmem_cache *s, int node, int objects)
> -{
> -	struct kmem_cache_node *n = get_node(s, node);
> -
> -	/*
> -	 * May be called early in order to allocate a slab for the
> -	 * kmem_cache_node structure. Solve the chicken-egg
> -	 * dilemma by deferring the increment of the count during
> -	 * bootstrap (see early_kmem_cache_node_alloc).
> -	 */
> -	if (likely(n)) {
> -		atomic_long_inc(&n->nr_slabs);
> -		atomic_long_add(objects, &n->total_objects);
> -	}
> -}
> -static inline void dec_slabs_node(struct kmem_cache *s, int node, int objects)
> -{
> -	struct kmem_cache_node *n = get_node(s, node);
> -
> -	atomic_long_dec(&n->nr_slabs);
>  	atomic_long_sub(objects, &n->total_objects);
>  }
>  
> @@ -1413,15 +1388,8 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
>  
>  #define disable_higher_order_debug 0
>  
> -static inline unsigned long slabs_node(struct kmem_cache *s, int node)
> -							{ return 0; }
> -static inline unsigned long node_nr_slabs(struct kmem_cache_node *n)
> -							{ return 0; }
> -static inline void inc_slabs_node(struct kmem_cache *s, int node,
> -							int objects) {}
> -static inline void dec_slabs_node(struct kmem_cache *s, int node,
> -							int objects) {}
> -
> +static inline void inc_objects_node(struct kmem_cache_node *n, int objects) {}
> +static inline void dec_objects_node(struct kmem_cache_node *n, int objects) {}
>  static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
>  			       void *freelist, void *nextfree)
>  {
> @@ -1429,6 +1397,42 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
>  }
>  #endif /* CONFIG_SLUB_DEBUG */
>  
> +static inline unsigned long slabs_node(struct kmem_cache *s, int node)
> +{
> +	struct kmem_cache_node *n = get_node(s, node);
> +
> +	return atomic_long_read(&n->nr_slabs);
> +}
> +
> +static inline unsigned long node_nr_slabs(struct kmem_cache_node *n)
> +{
> +	return atomic_long_read(&n->nr_slabs);
> +}
> +
> +static inline void inc_slabs_node(struct kmem_cache *s, int node, int objects)
> +{
> +	struct kmem_cache_node *n = get_node(s, node);
> +
> +	/*
> +	 * May be called early in order to allocate a slab for the
> +	 * kmem_cache_node structure. Solve the chicken-egg
> +	 * dilemma by deferring the increment of the count during
> +	 * bootstrap (see early_kmem_cache_node_alloc).
> +	 */
> +	if (likely(n)) {
> +		atomic_long_inc(&n->nr_slabs);
> +		inc_objects_node(n, objects);
> +	}
> +}
> +
> +static inline void dec_slabs_node(struct kmem_cache *s, int node, int objects)
> +{
> +	struct kmem_cache_node *n = get_node(s, node);
> +
> +	atomic_long_dec(&n->nr_slabs);
> +	dec_objects_node(n, objects);
> +}
> +
>  /*
>   * Hooks for other subsystems that check memory allocations. In a typical
>   * production configuration these hooks all should produce no code at all.
> 

Re: [PATCH 0/3] mm/slub: Fix slabs_node return value

[Christopher Lameter]

On Sun, 14 Jun 2020, Muchun Song wrote:

> The slabs_node() always return zero when CONFIG_SLUB_DEBUG is disabled.
> But some codes determine whether slab is empty by checking the return
> value of slabs_node(). As you know, the result is not correct. we move
> the nr_slabs of kmem_cache_node out of the CONFIG_SLUB_DEBUG. So we can
> get the corrent value returned by the slabs_node().

Not that all distribution kernels have CONFIG_SLUB_DEBUG enabled. This
does not enable runtime debugging but only compiles the debug code in that
can then be enabled at runtime.

Users of !CONFIG_SLUB_DEBUG do not want to have the debug code included
because they have extreme requirements on memory use.

This patch increases use of memory by enabling fields that were excluded
under !CONFIG_SLUB_DEBUG before!

There is nothing wrong with slab_node's return value if one wants to
sacrifice debugging and consistency checks for a small code build.