From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qa0-f53.google.com (mail-qa0-f53.google.com [209.85.216.53]) by kanga.kvack.org (Postfix) with ESMTP id 7DB0E6B0036 for ; Thu, 6 Feb 2014 18:02:10 -0500 (EST) Received: by mail-qa0-f53.google.com with SMTP id cm18so3922254qab.26 for ; Thu, 06 Feb 2014 15:02:10 -0800 (PST) Received: from qmta13.emeryville.ca.mail.comcast.net (qmta13.emeryville.ca.mail.comcast.net. [2001:558:fe2d:44:76:96:27:243]) by mx.google.com with ESMTP id p9si611693qcr.35.2014.02.06.11.13.24 for ; Thu, 06 Feb 2014 11:13:54 -0800 (PST) Date: Thu, 6 Feb 2014 13:13:20 -0600 (CST) From: Christoph Lameter Subject: Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove} In-Reply-To: <52F3CF12.70905@parallels.com> Message-ID: References: <1391702294-27289-1-git-send-email-vdavydov@parallels.com> <52F3CF12.70905@parallels.com> Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-linux-mm@kvack.org List-ID: To: Vladimir Davydov Cc: penberg@kernel.org, akpm@linux-foundation.org, rientjes@google.com, mhocko@suse.cz, linux-kernel@vger.kernel.org, linux-mm@kvack.org, devel@openvz.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lb0-f170.google.com (mail-lb0-f170.google.com [209.85.217.170]) by kanga.kvack.org (Postfix) with ESMTP id C453A6B0037 for ; Thu, 6 Feb 2014 18:08:16 -0500 (EST) Received: by mail-lb0-f170.google.com with SMTP id u14so2065299lbd.1 for ; Thu, 06 Feb 2014 15:08:16 -0800 (PST) Received: from relay.parallels.com (relay.parallels.com. [195.214.232.42]) by mx.google.com with ESMTPS id mq2si821881lbb.32.2014.02.06.07.58.17 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 06 Feb 2014 07:58:47 -0800 (PST) From: Vladimir Davydov Subject: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove} Date: Thu, 6 Feb 2014 19:58:13 +0400 Message-ID: <1391702294-27289-1-git-send-email-vdavydov@parallels.com> MIME-Version: 1.0 Content-Type: text/plain Sender: owner-linux-mm@kvack.org List-ID: To: cl@linux.com, penberg@kernel.org Cc: akpm@linux-foundation.org, rientjes@google.com, mhocko@suse.cz, linux-kernel@vger.kernel.org, linux-mm@kvack.org, devel@openvz.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qc0-f175.google.com (mail-qc0-f175.google.com [209.85.216.175]) by kanga.kvack.org (Postfix) with ESMTP id 5FD8F6B0035 for ; Thu, 6 Feb 2014 18:13:49 -0500 (EST) Received: by mail-qc0-f175.google.com with SMTP id x13so4467942qcv.6 for ; Thu, 06 Feb 2014 15:13:49 -0800 (PST) Received: from qmta11.emeryville.ca.mail.comcast.net (qmta11.emeryville.ca.mail.comcast.net. [2001:558:fe2d:44:76:96:27:211]) by mx.google.com with ESMTP id g16si942786qgd.15.2014.02.06.08.23.01 for ; Thu, 06 Feb 2014 08:23:31 -0800 (PST) Date: Thu, 6 Feb 2014 10:22:58 -0600 (CST) From: Christoph Lameter Subject: Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove} In-Reply-To: <1391702294-27289-1-git-send-email-vdavydov@parallels.com> Message-ID: References: <1391702294-27289-1-git-send-email-vdavydov@parallels.com> Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-linux-mm@kvack.org List-ID: To: Vladimir Davydov Cc: penberg@kernel.org, akpm@linux-foundation.org, rientjes@google.com, mhocko@suse.cz, linux-kernel@vger.kernel.org, linux-mm@kvack.org, devel@openvz.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-la0-f48.google.com (mail-la0-f48.google.com [209.85.215.48]) by kanga.kvack.org (Postfix) with ESMTP id 9ED0E6B0036 for ; Thu, 6 Feb 2014 18:24:46 -0500 (EST) Received: by mail-la0-f48.google.com with SMTP id mc6so2019298lab.21 for ; Thu, 06 Feb 2014 15:24:45 -0800 (PST) Received: from relay.parallels.com (relay.parallels.com. [195.214.232.42]) by mx.google.com with ESMTPS id kj3si1006243lbc.99.2014.02.06.10.06.14 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 06 Feb 2014 10:06:44 -0800 (PST) Message-ID: <52F3CF12.70905@parallels.com> Date: Thu, 6 Feb 2014 22:06:10 +0400 From: Vladimir Davydov MIME-Version: 1.0 Subject: Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove} References: <1391702294-27289-1-git-send-email-vdavydov@parallels.com> In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Christoph Lameter Cc: penberg@kernel.org, akpm@linux-foundation.org, rientjes@google.com, mhocko@suse.cz, linux-kernel@vger.kernel.org, linux-mm@kvack.org, devel@openvz.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756372AbaBFP6S (ORCPT ); Thu, 6 Feb 2014 10:58:18 -0500 Received: from relay.parallels.com ([195.214.232.42]:60426 "EHLO relay.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754365AbaBFP6Q (ORCPT ); Thu, 6 Feb 2014 10:58:16 -0500 From: Vladimir Davydov To: , CC: , , , , , Subject: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove} Date: Thu, 6 Feb 2014 19:58:13 +0400 Message-ID: <1391702294-27289-1-git-send-email-vdavydov@parallels.com> X-Mailer: git-send-email 1.7.10.4 MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.30.16.96] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755444AbaBFQXE (ORCPT ); Thu, 6 Feb 2014 11:23:04 -0500 Received: from qmta12.emeryville.ca.mail.comcast.net ([76.96.27.227]:51069 "EHLO qmta12.emeryville.ca.mail.comcast.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751336AbaBFQXC (ORCPT ); Thu, 6 Feb 2014 11:23:02 -0500 Date: Thu, 6 Feb 2014 10:22:58 -0600 (CST) From: Christoph Lameter X-X-Sender: cl@nuc To: Vladimir Davydov cc: penberg@kernel.org, akpm@linux-foundation.org, rientjes@google.com, mhocko@suse.cz, linux-kernel@vger.kernel.org, linux-mm@kvack.org, devel@openvz.org Subject: Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove} In-Reply-To: <1391702294-27289-1-git-send-email-vdavydov@parallels.com> Message-ID: References: <1391702294-27289-1-git-send-email-vdavydov@parallels.com> Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756879AbaBFSGP (ORCPT ); Thu, 6 Feb 2014 13:06:15 -0500 Received: from relay.parallels.com ([195.214.232.42]:51095 "EHLO relay.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751711AbaBFSGN (ORCPT ); Thu, 6 Feb 2014 13:06:13 -0500 Message-ID: <52F3CF12.70905@parallels.com> Date: Thu, 6 Feb 2014 22:06:10 +0400 From: Vladimir Davydov MIME-Version: 1.0 To: Christoph Lameter CC: , , , , , , Subject: Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove} References: <1391702294-27289-1-git-send-email-vdavydov@parallels.com> In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.24.25.245] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756876AbaBFTN1 (ORCPT ); Thu, 6 Feb 2014 14:13:27 -0500 Received: from qmta13.emeryville.ca.mail.comcast.net ([76.96.27.243]:42637 "EHLO qmta13.emeryville.ca.mail.comcast.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756854AbaBFTNY (ORCPT ); Thu, 6 Feb 2014 14:13:24 -0500 Date: Thu, 6 Feb 2014 13:13:20 -0600 (CST) From: Christoph Lameter X-X-Sender: cl@nuc To: Vladimir Davydov cc: penberg@kernel.org, akpm@linux-foundation.org, rientjes@google.com, mhocko@suse.cz, linux-kernel@vger.kernel.org, linux-mm@kvack.org, devel@openvz.org Subject: Re: [PATCH RFC] slub: do not drop slab_mutex for sysfs_slab_{add,remove} In-Reply-To: <52F3CF12.70905@parallels.com> Message-ID: References: <1391702294-27289-1-git-send-email-vdavydov@parallels.com> <52F3CF12.70905@parallels.com> Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.