For !CONFIG_SLUB_DEBUG, SLUB does not maintain the number of slabs allocated per node for a kmem_cache. Thus, slabs_node() in __kmem_cache_empty() will always return 0. So, in such situation, it is required to check per-cpu slabs to make sure if a kmem_cache is empty or not.
Please note that __kmem_cache_shutdown() and __kmem_cache_shrink() are not affected by !CONFIG_SLUB_DEBUG as they call flush_all() to clear per-cpu slabs.
Fixes: f9e13c0a5a33 ("slab, slub: skip unnecessary kasan_cache_shutdown()") Signed-off-by: Shakeel Butt shakeelb@google.com Reported-by: Jason A . Donenfeld Jason@zx2c4.com Cc: Christoph Lameter cl@linux.com Cc: Pekka Enberg penberg@kernel.org Cc: David Rientjes rientjes@google.com Cc: Joonsoo Kim iamjoonsoo.kim@lge.com Cc: Andrew Morton akpm@linux-foundation.org Cc: stable@vger.kernel.org --- mm/slub.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c index a3b8467c14af..731c02b371ae 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3673,9 +3673,23 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
bool __kmem_cache_empty(struct kmem_cache *s) { - int node; + int cpu, node; struct kmem_cache_node *n;
+ /* + * slabs_node will always be 0 for !CONFIG_SLUB_DEBUG. So, manually + * check slabs for all cpus. + */ + if (!IS_ENABLED(CONFIG_SLUB_DEBUG)) { + for_each_online_cpu(cpu) { + struct kmem_cache_cpu *c; + + c = per_cpu_ptr(s->cpu_slab, cpu); + if (c->page || slub_percpu_partial(c)) + return false; + } + } + for_each_kmem_cache_node(s, node, n) if (n->nr_partial || slabs_node(s, node)) return false;
On Tue, Jun 19, 2018 at 2:33 PM Shakeel Butt shakeelb@google.com wrote:
For !CONFIG_SLUB_DEBUG, SLUB does not maintain the number of slabs allocated per node for a kmem_cache. Thus, slabs_node() in __kmem_cache_empty() will always return 0. So, in such situation, it is required to check per-cpu slabs to make sure if a kmem_cache is empty or not.
Please note that __kmem_cache_shutdown() and __kmem_cache_shrink() are not affected by !CONFIG_SLUB_DEBUG as they call flush_all() to clear per-cpu slabs.
Fixes: f9e13c0a5a33 ("slab, slub: skip unnecessary kasan_cache_shutdown()") Signed-off-by: Shakeel Butt shakeelb@google.com Reported-by: Jason A . Donenfeld Jason@zx2c4.com Cc: Christoph Lameter cl@linux.com Cc: Pekka Enberg penberg@kernel.org Cc: David Rientjes rientjes@google.com Cc: Joonsoo Kim iamjoonsoo.kim@lge.com Cc: Andrew Morton akpm@linux-foundation.org Cc: stable@vger.kernel.org
Forgot to Cc: Andrey Ryabinin aryabinin@virtuozzo.com
mm/slub.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c index a3b8467c14af..731c02b371ae 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3673,9 +3673,23 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
bool __kmem_cache_empty(struct kmem_cache *s) {
int node;
int cpu, node; struct kmem_cache_node *n;/** slabs_node will always be 0 for !CONFIG_SLUB_DEBUG. So, manually* check slabs for all cpus.*/if (!IS_ENABLED(CONFIG_SLUB_DEBUG)) {for_each_online_cpu(cpu) {struct kmem_cache_cpu *c;c = per_cpu_ptr(s->cpu_slab, cpu);if (c->page || slub_percpu_partial(c))return false;}}for_each_kmem_cache_node(s, node, n) if (n->nr_partial || slabs_node(s, node)) return false;-- 2.18.0.rc1.244.gcf134e6275-goog
On Tue, Jun 19, 2018 at 11:34 PM Shakeel Butt shakeelb@google.com wrote:
For !CONFIG_SLUB_DEBUG, SLUB does not maintain the number of slabs allocated per node for a kmem_cache. Thus, slabs_node() in __kmem_cache_empty() will always return 0. So, in such situation, it is required to check per-cpu slabs to make sure if a kmem_cache is empty or not.
Please note that __kmem_cache_shutdown() and __kmem_cache_shrink() are not affected by !CONFIG_SLUB_DEBUG as they call flush_all() to clear per-cpu slabs.
Fixes: f9e13c0a5a33 ("slab, slub: skip unnecessary kasan_cache_shutdown()") Signed-off-by: Shakeel Butt shakeelb@google.com Reported-by: Jason A . Donenfeld Jason@zx2c4.com Cc: Christoph Lameter cl@linux.com Cc: Pekka Enberg penberg@kernel.org Cc: David Rientjes rientjes@google.com Cc: Joonsoo Kim iamjoonsoo.kim@lge.com Cc: Andrew Morton akpm@linux-foundation.org Cc: stable@vger.kernel.org
mm/slub.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c index a3b8467c14af..731c02b371ae 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3673,9 +3673,23 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
bool __kmem_cache_empty(struct kmem_cache *s) {
int node;
int cpu, node; struct kmem_cache_node *n;/** slabs_node will always be 0 for !CONFIG_SLUB_DEBUG. So, manually* check slabs for all cpus.*/if (!IS_ENABLED(CONFIG_SLUB_DEBUG)) {for_each_online_cpu(cpu) {struct kmem_cache_cpu *c;c = per_cpu_ptr(s->cpu_slab, cpu);if (c->page || slub_percpu_partial(c))return false;}}for_each_kmem_cache_node(s, node, n) if (n->nr_partial || slabs_node(s, node)) return false;-- 2.18.0.rc1.244.gcf134e6275-goog
I can confirm that this fixes the test case on build.wireguard.com.
On Tue, 19 Jun 2018 14:33:52 -0700 Shakeel Butt shakeelb@google.com wrote:
For !CONFIG_SLUB_DEBUG, SLUB does not maintain the number of slabs allocated per node for a kmem_cache. Thus, slabs_node() in __kmem_cache_empty() will always return 0. So, in such situation, it is required to check per-cpu slabs to make sure if a kmem_cache is empty or not.
Please note that __kmem_cache_shutdown() and __kmem_cache_shrink() are not affected by !CONFIG_SLUB_DEBUG as they call flush_all() to clear per-cpu slabs.
Thanks guys. I'll beef up this changelog a bit by adding
f9e13c0a5a33 ("slab, slub: skip unnecessary kasan_cache_shutdown()") causes crashes when using slub, as described at http://lkml.kernel.org/r/CAHmME9rtoPwxUSnktxzKso14iuVCWT7BE_-_8PAC=pGw1iJnQg...
So that a) Greg knows why we're sending it at him and b) other people who are seeing the same sorts of crashes in their various kernels will know that this patch will probably fix them.
On Tue, 19 Jun 2018, Shakeel Butt wrote:
diff --git a/mm/slub.c b/mm/slub.c index a3b8467c14af..731c02b371ae 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3673,9 +3673,23 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n) bool __kmem_cache_empty(struct kmem_cache *s) {
- int node;
- int cpu, node;
Nit: wouldn't cpu be unused if CONFIG_SLUB_DEBUG is disabled?
struct kmem_cache_node *n;
- /*
* slabs_node will always be 0 for !CONFIG_SLUB_DEBUG. So, manually* check slabs for all cpus.*/- if (!IS_ENABLED(CONFIG_SLUB_DEBUG)) {
for_each_online_cpu(cpu) {struct kmem_cache_cpu *c;c = per_cpu_ptr(s->cpu_slab, cpu);if (c->page || slub_percpu_partial(c))return false;}- }
- for_each_kmem_cache_node(s, node, n) if (n->nr_partial || slabs_node(s, node)) return false;
Wouldn't it just be better to allow {inc,dec}_slabs_node() to adjust the nr_slabs counter instead of doing the per-cpu iteration on every shutdown?
On Tue, Jun 19, 2018 at 5:49 PM David Rientjes rientjes@google.com wrote:
On Tue, 19 Jun 2018, Shakeel Butt wrote:
diff --git a/mm/slub.c b/mm/slub.c index a3b8467c14af..731c02b371ae 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3673,9 +3673,23 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
bool __kmem_cache_empty(struct kmem_cache *s) {
int node;
int cpu, node;Nit: wouldn't cpu be unused if CONFIG_SLUB_DEBUG is disabled?
I think I didn't get the warning as I didn't use #ifdef.
struct kmem_cache_node *n;
/** slabs_node will always be 0 for !CONFIG_SLUB_DEBUG. So, manually* check slabs for all cpus.*/if (!IS_ENABLED(CONFIG_SLUB_DEBUG)) {for_each_online_cpu(cpu) {struct kmem_cache_cpu *c;c = per_cpu_ptr(s->cpu_slab, cpu);if (c->page || slub_percpu_partial(c))return false;}}for_each_kmem_cache_node(s, node, n) if (n->nr_partial || slabs_node(s, node)) return false;Wouldn't it just be better to allow {inc,dec}_slabs_node() to adjust the nr_slabs counter instead of doing the per-cpu iteration on every shutdown?
Yes that is doable as the functions {inc,dec}_slabs_node() are called in slow path. I can move them out of CONFIG_SLUB_DEBUG. I think the reason 0f389ec63077 ("slub: No need for per node slab counters if !SLUB_DEBUG") put them under CONFIG_SLUB_DEBUG is because these counters were only read through sysfs API which were disabled on !CONFIG_SLUB_DEBUG. However we have a usecase other than sysfs API.
Please let me know if there is any objection to this conversion. For large machines I think this is preferable approach.
thanks, Shakeel
On 06/20/2018 12:33 AM, Shakeel Butt wrote:
For !CONFIG_SLUB_DEBUG, SLUB does not maintain the number of slabs allocated per node for a kmem_cache. Thus, slabs_node() in __kmem_cache_empty() will always return 0. So, in such situation, it is required to check per-cpu slabs to make sure if a kmem_cache is empty or not.
Please note that __kmem_cache_shutdown() and __kmem_cache_shrink() are not affected by !CONFIG_SLUB_DEBUG as they call flush_all() to clear per-cpu slabs.
So what? Yes, they call flush_all() and then check if there are non-empty slabs left. And that check doesn't work in case of disabled CONFIG_SLUB_DEBUG. How is flush_all() or per-cpu slabs even relevant here?
On Wed, Jun 20, 2018 at 5:08 AM Andrey Ryabinin aryabinin@virtuozzo.com wrote:
On 06/20/2018 12:33 AM, Shakeel Butt wrote:
For !CONFIG_SLUB_DEBUG, SLUB does not maintain the number of slabs allocated per node for a kmem_cache. Thus, slabs_node() in __kmem_cache_empty() will always return 0. So, in such situation, it is required to check per-cpu slabs to make sure if a kmem_cache is empty or not.
Please note that __kmem_cache_shutdown() and __kmem_cache_shrink() are not affected by !CONFIG_SLUB_DEBUG as they call flush_all() to clear per-cpu slabs.
So what? Yes, they call flush_all() and then check if there are non-empty slabs left. And that check doesn't work in case of disabled CONFIG_SLUB_DEBUG. How is flush_all() or per-cpu slabs even relevant here?
The flush_all() will move all cpu slabs and partials to node's partial list and thus later check of node's partial list will handle non-empty slabs situation. However what I missed is the 'full slabs' which are not on any list for !CONFIG_SLUB_DEBUG. So, this patch is not the complete solution. I think David's suggestion is the complete solution. I will post a patch based on David's suggestion.
linux-stable-mirror@lists.linaro.org
-
Andrew Morton -
Andrey Ryabinin -
David Rientjes -
Jason A. Donenfeld -
Shakeel Butt