[PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove}

[PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove}

[Vladimir Davydov]

When creating/destroying a kmem cache, we do a lot of work holding the
slab_mutex, but we drop it for sysfs_slab_{add,remove} for some reason.
Since __kmem_cache_create and __kmem_cache_shutdown are extremely rare,
I propose to simplify locking by calling sysfs_slab_{add,remove} w/o
dropping the slab_mutex.

I'm interested in this, because when creating a memcg cache I need the
slab_mutex locked until the cache is fully initialized and registered to
the memcg subsys (memcg_cache_register() is called). If this is not
true, I get races when several threads try to create a cache for the
same memcg.  An alternative fix for my problem would be moving
sysfs_slab_{add,remove} after the slab_mutex is dropped, but I'd like to
try the shortest path first.

Any objections to this?

Thanks.
---
 mm/slub.c |   15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 3d3a8a7a0f8c..6f4393892d2d 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3229,19 +3229,8 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
 {
 	int rc = kmem_cache_close(s);
 
-	if (!rc) {
-		/*
-		 * We do the same lock strategy around sysfs_slab_add, see
-		 * __kmem_cache_create. Because this is pretty much the last
-		 * operation we do and the lock will be released shortly after
-		 * that in slab_common.c, we could just move sysfs_slab_remove
-		 * to a later point in common code. We should do that when we
-		 * have a common sysfs framework for all allocators.
-		 */
-		mutex_unlock(&slab_mutex);
+	if (!rc)
 		sysfs_slab_remove(s);
-		mutex_lock(&slab_mutex);
-	}
 
 	return rc;
 }
@@ -3772,9 +3761,7 @@ int __kmem_cache_create(struct kmem_cache *s, unsigned long flags)
 		return 0;
 
 	memcg_propagate_slab_attrs(s);
-	mutex_unlock(&slab_mutex);
 	err = sysfs_slab_add(s);
-	mutex_lock(&slab_mutex);
 
 	if (err)
 		kmem_cache_close(s);
-- 
1.7.10.4

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

[PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove}

[Vladimir Davydov]

When creating/destroying a kmem cache, we do a lot of work holding the
slab_mutex, but we drop it for sysfs_slab_{add,remove} for some reason.
Since __kmem_cache_create and __kmem_cache_shutdown are extremely rare,
I propose to simplify locking by calling sysfs_slab_{add,remove} w/o
dropping the slab_mutex.

I'm interested in this, because when creating a memcg cache I need the
slab_mutex locked until the cache is fully initialized and registered to
the memcg subsys (memcg_cache_register() is called). If this is not
true, I get races when several threads try to create a cache for the
same memcg.  An alternative fix for my problem would be moving
sysfs_slab_{add,remove} after the slab_mutex is dropped, but I'd like to
try the shortest path first.

Any objections to this?

Thanks.
---
 mm/slub.c |   15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 3d3a8a7a0f8c..6f4393892d2d 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3229,19 +3229,8 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
 {
 	int rc = kmem_cache_close(s);
 
-	if (!rc) {
-		/*
-		 * We do the same lock strategy around sysfs_slab_add, see
-		 * __kmem_cache_create. Because this is pretty much the last
-		 * operation we do and the lock will be released shortly after
-		 * that in slab_common.c, we could just move sysfs_slab_remove
-		 * to a later point in common code. We should do that when we
-		 * have a common sysfs framework for all allocators.
-		 */
-		mutex_unlock(&slab_mutex);
+	if (!rc)
 		sysfs_slab_remove(s);
-		mutex_lock(&slab_mutex);
-	}
 
 	return rc;
 }
@@ -3772,9 +3761,7 @@ int __kmem_cache_create(struct kmem_cache *s, unsigned long flags)
 		return 0;
 
 	memcg_propagate_slab_attrs(s);
-	mutex_unlock(&slab_mutex);
 	err = sysfs_slab_add(s);
-	mutex_lock(&slab_mutex);
 
 	if (err)
 		kmem_cache_close(s);
-- 
1.7.10.4

Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove}

[Christoph Lameter]

On Thu, 6 Feb 2014, Vladimir Davydov wrote:

> When creating/destroying a kmem cache, we do a lot of work holding the
> slab_mutex, but we drop it for sysfs_slab_{add,remove} for some reason.
> Since __kmem_cache_create and __kmem_cache_shutdown are extremely rare,
> I propose to simplify locking by calling sysfs_slab_{add,remove} w/o
> dropping the slab_mutex.

The problem is that sysfs does nasty things like spawning a process in
user space that may lead to something wanting to create slabs too. The
module may then hang waiting on the lock ...

I would be very thankful, if you can get that actually working reliably
without deadlock issues.

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

Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove}

[Christoph Lameter]

On Thu, 6 Feb 2014, Vladimir Davydov wrote:

> When creating/destroying a kmem cache, we do a lot of work holding the
> slab_mutex, but we drop it for sysfs_slab_{add,remove} for some reason.
> Since __kmem_cache_create and __kmem_cache_shutdown are extremely rare,
> I propose to simplify locking by calling sysfs_slab_{add,remove} w/o
> dropping the slab_mutex.

The problem is that sysfs does nasty things like spawning a process in
user space that may lead to something wanting to create slabs too. The
module may then hang waiting on the lock ...

I would be very thankful, if you can get that actually working reliably
without deadlock issues.

Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove}

[Vladimir Davydov]

On 02/06/2014 08:22 PM, Christoph Lameter wrote:
> On Thu, 6 Feb 2014, Vladimir Davydov wrote:
>
>> When creating/destroying a kmem cache, we do a lot of work holding the
>> slab_mutex, but we drop it for sysfs_slab_{add,remove} for some reason.
>> Since __kmem_cache_create and __kmem_cache_shutdown are extremely rare,
>> I propose to simplify locking by calling sysfs_slab_{add,remove} w/o
>> dropping the slab_mutex.
> The problem is that sysfs does nasty things like spawning a process in
> user space that may lead to something wanting to create slabs too. The
> module may then hang waiting on the lock ...

Hmm... IIUC the only function of concern is kobject_uevent() -
everything else called from sysfs_slab_{add,remove} is a mix of kmalloc,
kfree, mutex_lock/unlock - in short, nothing dangerous. There we do
call_usermodehelper(), but we do it with UMH_WAIT_EXEC, which means
"wait for exec only, but not for the process to complete". An exec
shouldn't issue any slab-related stuff AFAIU. At least, I tried to run
the patched kernel with lockdep enabled and got no warnings at all when
getting uevents about adding/removing caches. That's why I started to
doubt whether we really need this lock...

Please correct me if I'm wrong.

> I would be very thankful, if you can get that actually working reliably
> without deadlock issues.

If there is no choice rather than moving sysfs_slab_{add,remove} out of
the slab_mutex critical section, I'll have to do it that way. But first
I'd like to make sure it cannot be done with less footprint.

Thanks.

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

Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove}

[Vladimir Davydov]

On 02/06/2014 08:22 PM, Christoph Lameter wrote:
> On Thu, 6 Feb 2014, Vladimir Davydov wrote:
>
>> When creating/destroying a kmem cache, we do a lot of work holding the
>> slab_mutex, but we drop it for sysfs_slab_{add,remove} for some reason.
>> Since __kmem_cache_create and __kmem_cache_shutdown are extremely rare,
>> I propose to simplify locking by calling sysfs_slab_{add,remove} w/o
>> dropping the slab_mutex.
> The problem is that sysfs does nasty things like spawning a process in
> user space that may lead to something wanting to create slabs too. The
> module may then hang waiting on the lock ...

Hmm... IIUC the only function of concern is kobject_uevent() -
everything else called from sysfs_slab_{add,remove} is a mix of kmalloc,
kfree, mutex_lock/unlock - in short, nothing dangerous. There we do
call_usermodehelper(), but we do it with UMH_WAIT_EXEC, which means
"wait for exec only, but not for the process to complete". An exec
shouldn't issue any slab-related stuff AFAIU. At least, I tried to run
the patched kernel with lockdep enabled and got no warnings at all when
getting uevents about adding/removing caches. That's why I started to
doubt whether we really need this lock...

Please correct me if I'm wrong.

> I would be very thankful, if you can get that actually working reliably
> without deadlock issues.

If there is no choice rather than moving sysfs_slab_{add,remove} out of
the slab_mutex critical section, I'll have to do it that way. But first
I'd like to make sure it cannot be done with less footprint.

Thanks.

Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove}

[Christoph Lameter]

On Thu, 6 Feb 2014, Vladimir Davydov wrote:

> Hmm... IIUC the only function of concern is kobject_uevent() -
> everything else called from sysfs_slab_{add,remove} is a mix of kmalloc,
> kfree, mutex_lock/unlock - in short, nothing dangerous. There we do
> call_usermodehelper(), but we do it with UMH_WAIT_EXEC, which means
> "wait for exec only, but not for the process to complete". An exec
> shouldn't issue any slab-related stuff AFAIU. At least, I tried to run
> the patched kernel with lockdep enabled and got no warnings at all when
> getting uevents about adding/removing caches. That's why I started to
> doubt whether we really need this lock...
>
> Please correct me if I'm wrong.

I have had this deadlock a couple of years ago. Sysfs seems to change over
time. Not sure if that is still the case.

> > I would be very thankful, if you can get that actually working reliably
> > without deadlock issues.
>
> If there is no choice rather than moving sysfs_slab_{add,remove} out of
> the slab_mutex critical section, I'll have to do it that way. But first
> I'd like to make sure it cannot be done with less footprint.

I am all for holding the lock as long as possible.

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

Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove}

[Christoph Lameter]

On Thu, 6 Feb 2014, Vladimir Davydov wrote:

> Hmm... IIUC the only function of concern is kobject_uevent() -
> everything else called from sysfs_slab_{add,remove} is a mix of kmalloc,
> kfree, mutex_lock/unlock - in short, nothing dangerous. There we do
> call_usermodehelper(), but we do it with UMH_WAIT_EXEC, which means
> "wait for exec only, but not for the process to complete". An exec
> shouldn't issue any slab-related stuff AFAIU. At least, I tried to run
> the patched kernel with lockdep enabled and got no warnings at all when
> getting uevents about adding/removing caches. That's why I started to
> doubt whether we really need this lock...
>
> Please correct me if I'm wrong.

I have had this deadlock a couple of years ago. Sysfs seems to change over
time. Not sure if that is still the case.

> > I would be very thankful, if you can get that actually working reliably
> > without deadlock issues.
>
> If there is no choice rather than moving sysfs_slab_{add,remove} out of
> the slab_mutex critical section, I'll have to do it that way. But first
> I'd like to make sure it cannot be done with less footprint.

I am all for holding the lock as long as possible.