Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1365310 > unrolled thread

[PATCH 01/11] mm/slab: hold a slab_mutex when calling __kmem_cache_shrink()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 01/11] mm/slab: hold a slab_mutex when calling __kmem_cache_shrink() js1304@gmail.com - 2016-03-28 07:30 +0200
    Re: [PATCH 01/11] mm/slab: hold a slab_mutex when calling  __kmem_cache_shrink() Christoph Lameter <cl@linux.com> - 2016-03-29 03:00 +0200
      Re: [PATCH 01/11] mm/slab: hold a slab_mutex when calling  __kmem_cache_shrink() Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-03-30 10:20 +0200
    Re: [PATCH 01/11] mm/slab: hold a slab_mutex when calling  __kmem_cache_shrink() Nikolay Borisov <kernel@kyup.com> - 2016-03-31 13:00 +0200
      Re: [PATCH 01/11] mm/slab: hold a slab_mutex when calling  __kmem_cache_shrink() Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-04-01 04:20 +0200

#1365310 — [PATCH 01/11] mm/slab: hold a slab_mutex when calling __kmem_cache_shrink()

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Major kmem_cache metadata in slab subsystem is synchronized with
the slab_mutex. In SLAB, if some of them is changed, node's shared
array cache would be freed and re-populated. If __kmem_cache_shrink()
is called at the same time, it will call drain_array() with n->shared
without holding node lock so problem can happen.

We can fix this small theoretical race condition by holding node lock
in drain_array(), but, holding a slab_mutex in kmem_cache_shrink()
looks more appropriate solution because stable state would make things
less error-prone and this is not performance critical path.

In addtion, annotate on SLAB functions.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 mm/slab.c        | 2 ++
 mm/slab_common.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/mm/slab.c b/mm/slab.c
index a53a0f6..043606a 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2218,6 +2218,7 @@ static void do_drain(void *arg)
 	ac->avail = 0;
 }
 
+/* Should be called with slab_mutex to prevent from freeing shared array */
 static void drain_cpu_caches(struct kmem_cache *cachep)
 {
 	struct kmem_cache_node *n;
@@ -3871,6 +3872,7 @@ skip_setup:
  * Drain an array if it contains any elements taking the node lock only if
  * necessary. Note that the node listlock also protects the array_cache
  * if drain_array() is used on the shared array.
+ * Should be called with slab_mutex to prevent from freeing shared array.
  */
 static void drain_array(struct kmem_cache *cachep, struct kmem_cache_node *n,
 			 struct array_cache *ac, int force, int node)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index a65dad7..5bed565 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -755,7 +755,11 @@ int kmem_cache_shrink(struct kmem_cache *cachep)
 	get_online_cpus();
 	get_online_mems();
 	kasan_cache_shrink(cachep);
+
+	mutex_lock(&slab_mutex);
 	ret = __kmem_cache_shrink(cachep, false);
+	mutex_unlock(&slab_mutex);
+
 	put_online_mems();
 	put_online_cpus();
 	return ret;
-- 
1.9.1

[toc] | [next] | [standalone]

#1365744 — Re: [PATCH 01/11] mm/slab: hold a slab_mutex when calling __kmem_cache_shrink()

On Mon, 28 Mar 2016, js1304@gmail.com wrote:

> Major kmem_cache metadata in slab subsystem is synchronized with
> the slab_mutex. In SLAB, if some of them is changed, node's shared
> array cache would be freed and re-populated. If __kmem_cache_shrink()
> is called at the same time, it will call drain_array() with n->shared
> without holding node lock so problem can happen.
>
> We can fix this small theoretical race condition by holding node lock
> in drain_array(), but, holding a slab_mutex in kmem_cache_shrink()
> looks more appropriate solution because stable state would make things
> less error-prone and this is not performance critical path.

Ummm.. The mutex taking is added to common code. So this will also affect
SLUB.  The patch needs to consider this. Do we want to force all
allocators to run shrinking only when holding the lock? SLUB does not
need to hold the mutex. And frankly the mutex is for reconfiguration of
metadata which is *not* occurring here. A shrink operation does not do
that. Can we figure out a slab specific way of handling synchronization
in the strange free/realloc cycle?

It seems that taking the node lock is the appropriate level of
synchrnonization since the concern is with the contents of a shared cache
at that level. There is no change of metadata which would require the
mutex.

[toc] | [prev] | [next] | [standalone]

#1366985 — Re: [PATCH 01/11] mm/slab: hold a slab_mutex when calling __kmem_cache_shrink()

On Mon, Mar 28, 2016 at 07:50:36PM -0500, Christoph Lameter wrote:
> On Mon, 28 Mar 2016, js1304@gmail.com wrote:
> 
> > Major kmem_cache metadata in slab subsystem is synchronized with
> > the slab_mutex. In SLAB, if some of them is changed, node's shared
> > array cache would be freed and re-populated. If __kmem_cache_shrink()
> > is called at the same time, it will call drain_array() with n->shared
> > without holding node lock so problem can happen.
> >
> > We can fix this small theoretical race condition by holding node lock
> > in drain_array(), but, holding a slab_mutex in kmem_cache_shrink()
> > looks more appropriate solution because stable state would make things
> > less error-prone and this is not performance critical path.
> 
> Ummm.. The mutex taking is added to common code. So this will also affect
> SLUB.  The patch needs to consider this. Do we want to force all
> allocators to run shrinking only when holding the lock? SLUB does not
> need to hold the mutex. And frankly the mutex is for reconfiguration of
> metadata which is *not* occurring here. A shrink operation does not do
> that. Can we figure out a slab specific way of handling synchronization
> in the strange free/realloc cycle?
> 
> It seems that taking the node lock is the appropriate level of
> synchrnonization since the concern is with the contents of a shared cache
> at that level. There is no change of metadata which would require the
> mutex.

Okay. I will fix it.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1368170 — Re: [PATCH 01/11] mm/slab: hold a slab_mutex when calling __kmem_cache_shrink()

On 03/28/2016 08:26 AM, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> Major kmem_cache metadata in slab subsystem is synchronized with
> the slab_mutex. In SLAB, if some of them is changed, node's shared
> array cache would be freed and re-populated. If __kmem_cache_shrink()
> is called at the same time, it will call drain_array() with n->shared
> without holding node lock so problem can happen.
> 
> We can fix this small theoretical race condition by holding node lock
> in drain_array(), but, holding a slab_mutex in kmem_cache_shrink()
> looks more appropriate solution because stable state would make things
> less error-prone and this is not performance critical path.
> 
> In addtion, annotate on SLAB functions.

Just a nit but would it not be better instead of doing comment-style
annotation to use lockdep_assert_held/_once. In both cases for someone
to understand what locks have to be held will go and read the source. In
my mind it's easier to miss a comment line, rather than the
lockdep_assert. Furthermore in case lockdep is enabled a locking
violation would spew useful info to dmesg.

[toc] | [prev] | [next] | [standalone]

#1368957 — Re: [PATCH 01/11] mm/slab: hold a slab_mutex when calling __kmem_cache_shrink()

On Thu, Mar 31, 2016 at 01:53:14PM +0300, Nikolay Borisov wrote:
> 
> 
> On 03/28/2016 08:26 AM, js1304@gmail.com wrote:
> > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > 
> > Major kmem_cache metadata in slab subsystem is synchronized with
> > the slab_mutex. In SLAB, if some of them is changed, node's shared
> > array cache would be freed and re-populated. If __kmem_cache_shrink()
> > is called at the same time, it will call drain_array() with n->shared
> > without holding node lock so problem can happen.
> > 
> > We can fix this small theoretical race condition by holding node lock
> > in drain_array(), but, holding a slab_mutex in kmem_cache_shrink()
> > looks more appropriate solution because stable state would make things
> > less error-prone and this is not performance critical path.
> > 
> > In addtion, annotate on SLAB functions.
> 
> Just a nit but would it not be better instead of doing comment-style
> annotation to use lockdep_assert_held/_once. In both cases for someone
> to understand what locks have to be held will go and read the source. In
> my mind it's easier to miss a comment line, rather than the
> lockdep_assert. Furthermore in case lockdep is enabled a locking
> violation would spew useful info to dmesg.

Good idea. I'm not sure if lockdep_assert is best fit but I will add
something to check it rather than just adding the comment.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web