slab, slub: skip unnecessary kasan_cache_shutdown()
Subsystems (3)
PATCH 1/1 slab, slub: skip unnecessary kasan_cache_shutdown()
The kasan quarantine is designed to delay freeing slab objects to catch use-after-free. The quarantine can be large (several percent of machine memory size). When kmem_caches are deleted related objects are flushed from the quarantine but this requires scanning the entire quarantine which can be very slow. We have seen the kernel busily working on this while holding slab_mutex and badly affecting cache_reaper, slabinfo readers and memcg kmem cache creations.
It can easily reproduced by following script:
yes . | head -1000000 | xargs stat > /dev/null
for i in `seq 1 10`; do
seq 500 | (cd /cg/memory && xargs mkdir)
seq 500 | xargs -I{} sh -c 'echo $BASHPID > \
/cg/memory/{}/tasks && exec stat .' > /dev/null
seq 500 | (cd /cg/memory && xargs rmdir)
doneThe busy stack:
kasan_cache_shutdown
shutdown_cache
memcg_destroy_kmem_caches
mem_cgroup_css_free
css_free_rwork_fn
process_one_work
worker_thread
kthread
ret_from_fork
This patch is based on the observation that if the kmem_cache to be destroyed is empty then there should not be any objects of this cache in the quarantine.
Without the patch the script got stuck for couple of hours. With the patch the script completed within a second.
--- mm/kasan/kasan.c | 3 ++- mm/slab.c | 12 ++++++++++++ mm/slab.h | 1 + mm/slub.c | 11 +++++++++++ 4 files changed, 26 insertions(+), 1 deletion(-)
mm/kasan/kasan.c +2 -1
@@ -382,7 +382,8 @@ void kasan_cache_shrink(struct kmem_cache *cache)void kasan_cache_shutdown (struct kmem_cache *cache) { -quarantine_remove_cache (cache); +if (!__kmem_cache_empty (cache)) +quarantine_remove_cache (cache); }size_t kasan_metadata_size (struct kmem_cache *cache)
mm/slab.c +12 -0
@@ -2291,6 +2291,18 @@ static int drain_freelist(struct kmem_cache *cache,return nr_freed; } +bool __kmem_cache_empty (struct kmem_cache *s) +{ +int node; +struct kmem_cache_node *n; + +for_each_kmem_cache_node (s, node, n) +if (!list_empty (&n->slabs_full) || + !list_empty (&n->slabs_partial)) +return false; +return true; +} +int __kmem_cache_shrink (struct kmem_cache *cachep) {int ret =0 ;
mm/slab.h +1 -0
@@ -166,6 +166,7 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size, SLAB_TEMPORARY | \ SLAB_ACCOUNT) +bool __kmem_cache_empty (struct kmem_cache *);int __kmem_cache_shutdown (struct kmem_cache *);void __kmem_cache_release (struct kmem_cache *);int __kmem_cache_shrink (struct kmem_cache *);
mm/slub.c +11 -0
@@ -3707,6 +3707,17 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)discard_slab (s, page); } +bool __kmem_cache_empty (struct kmem_cache *s) +{ +int node; +struct kmem_cache_node *n; + +for_each_kmem_cache_node (s, node, n) +if (n->nr_partial ||slabs_node (s, node)) +return false; +return true; +} +/* * Release all resources used by a slab cache. */
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
David Rientjes
reviewerQuoted 61 lines · Shakeel Butt
mm/kasan/kasan.c +2
-1@@ -382,7 +382,8 @@ void kasan_cache_shrink(struct kmem_cache *cache)void kasan_cache_shutdown (struct kmem_cache *cache) { -quarantine_remove_cache (cache); +if (!__kmem_cache_empty (cache)) +quarantine_remove_cache (cache); }size_t kasan_metadata_size (struct kmem_cache *cache)mm/slab.c +12
-0@@ -2291,6 +2291,18 @@ static int drain_freelist(struct kmem_cache *cache,return nr_freed; } +bool __kmem_cache_empty (struct kmem_cache *s) +{ +int node; +struct kmem_cache_node *n; + +for_each_kmem_cache_node (s, node, n) +if (!list_empty (&n->slabs_full) || + !list_empty (&n->slabs_partial)) +return false; +return true; +} +int __kmem_cache_shrink (struct kmem_cache *cachep) {int ret =0 ;mm/slab.h +1
-0@@ -166,6 +166,7 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size, SLAB_TEMPORARY | \ SLAB_ACCOUNT) +bool __kmem_cache_empty (struct kmem_cache *);int __kmem_cache_shutdown (struct kmem_cache *);void __kmem_cache_release (struct kmem_cache *);int __kmem_cache_shrink (struct kmem_cache *);mm/slub.c +4
-0@@ -3707,6 +3707,17 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)discard_slab (s, page); } +bool __kmem_cache_empty (struct kmem_cache *s) +{ +int node; +struct kmem_cache_node *n;
mm/slub.c static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n) · line 3723
Quoted 10 lines
mm/slub.c +7
-0+ +for_each_kmem_cache_node (s, node, n) +if (n->nr_partial ||slabs_node (s, node)) +return false; +return true; +} +/* * Release all resources used by a slab cache. */
Any reason not to just make quarantine_remove_cache() part of __kmem_cache_shutdown() instead of duplicating its logic?
Quoted 68 lines · David Rientjes
Quoted 66 lines · Shakeel Butt
mm/kasan/kasan.c +2
-1@@ -382,7 +382,8 @@ void kasan_cache_shrink(struct kmem_cache *cache)void kasan_cache_shutdown (struct kmem_cache *cache) { -quarantine_remove_cache (cache); +if (!__kmem_cache_empty (cache)) +quarantine_remove_cache (cache); }size_t kasan_metadata_size (struct kmem_cache *cache)mm/slab.c +12
-0@@ -2291,6 +2291,18 @@ static int drain_freelist(struct kmem_cache *cache,return nr_freed; } +bool __kmem_cache_empty (struct kmem_cache *s) +{ +int node; +struct kmem_cache_node *n; + +for_each_kmem_cache_node (s, node, n) +if (!list_empty (&n->slabs_full) || + !list_empty (&n->slabs_partial)) +return false; +return true; +} +int __kmem_cache_shrink (struct kmem_cache *cachep) {int ret =0 ;mm/slab.h +1
-0@@ -166,6 +166,7 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size, SLAB_TEMPORARY | \ SLAB_ACCOUNT) +bool __kmem_cache_empty (struct kmem_cache *);int __kmem_cache_shutdown (struct kmem_cache *);void __kmem_cache_release (struct kmem_cache *);int __kmem_cache_shrink (struct kmem_cache *);mm/slub.c +9
-0@@ -3707,6 +3707,17 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)discard_slab (s, page); } +bool __kmem_cache_empty (struct kmem_cache *s) +{ +int node; +struct kmem_cache_node *n; + +for_each_kmem_cache_node (s, node, n) +if (n->nr_partial ||slabs_node (s, node)) +return false; +return true;
Quoted 8 lines
mm/slub.c static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n) · line 3723
Quoted 5 lines
mm/slub.c +2
-0+} +/* * Release all resources used by a slab cache. */ Any reason not to just make
quarantine_remove_cache()part of__kmem_cache_shutdown()instead of duplicating its logic?
Can you please explain what you mean by making quarantine_remove_cache() part of __kmem_cache_shutdown()? Do you mean calling quarantine_remove_cache() inside __kmem_cache_shutdown()? The __kmem_cache_shutdown() of both SLAB & SLUB does per-cpu draining/flushing and we want the free the quarantined objects before that. So, I am not sure how to incorporate quarantine_remove_cache() into __kmem_cache_shutdown() without duplicating the for-loop & if-condition.
David Rientjes
reviewerQuoted 78 lines · Shakeel Butt
Quoted 68 lines · David Rientjes
Quoted 66 lines · Shakeel Butt
mm/kasan/kasan.c +2
-1@@ -382,7 +382,8 @@ void kasan_cache_shrink(struct kmem_cache *cache)void kasan_cache_shutdown (struct kmem_cache *cache) { -quarantine_remove_cache (cache); +if (!__kmem_cache_empty (cache)) +quarantine_remove_cache (cache); }size_t kasan_metadata_size (struct kmem_cache *cache)mm/slab.c +12
-0@@ -2291,6 +2291,18 @@ static int drain_freelist(struct kmem_cache *cache,return nr_freed; } +bool __kmem_cache_empty (struct kmem_cache *s) +{ +int node; +struct kmem_cache_node *n; + +for_each_kmem_cache_node (s, node, n) +if (!list_empty (&n->slabs_full) || + !list_empty (&n->slabs_partial)) +return false; +return true; +} +int __kmem_cache_shrink (struct kmem_cache *cachep) {int ret =0 ;mm/slab.h +1
-0@@ -166,6 +166,7 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size, SLAB_TEMPORARY | \ SLAB_ACCOUNT) +bool __kmem_cache_empty (struct kmem_cache *);int __kmem_cache_shutdown (struct kmem_cache *);void __kmem_cache_release (struct kmem_cache *);int __kmem_cache_shrink (struct kmem_cache *);mm/slub.c +9
-0@@ -3707,6 +3707,17 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)discard_slab (s, page); } +bool __kmem_cache_empty (struct kmem_cache *s) +{ +int node; +struct kmem_cache_node *n; + +for_each_kmem_cache_node (s, node, n) +if (n->nr_partial ||slabs_node (s, node)) +return false; +return true;Quoted 8 lines
mm/slub.c static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n) · line 3723
Quoted 5 lines
mm/slub.c +2
-0+} +/* * Release all resources used by a slab cache. */ Any reason not to just make
quarantine_remove_cache()part of__kmem_cache_shutdown()instead of duplicating its logic?
Quoted 9 lines
Can you please explain what you mean by making
quarantine_remove_cache()part of__kmem_cache_shutdown()? Do you mean callingquarantine_remove_cache()inside__kmem_cache_shutdown()? The__kmem_cache_shutdown()of both SLAB & SLUB does per-cpu draining/flushing and we want the free the quarantined objects before that. So, I am not sure how to incorporatequarantine_remove_cache()into__kmem_cache_shutdown()without duplicating the for-loop & if-condition.
__kmem_cache_empty() is largely a copy and paste of __kmem_cache_shutdown() logic, is there no way to simplify this? I was thinking of generalizing the iteration (for_each_kmem_cach_node_nonempty?) that eliminates the need for __kmem_cache_empty().
kasan_cache_shutdown() would do
for_each_kmem_cache_node_nonempty(cache, node, n) {
quarantine_remove_cache(cache);
break;
}and __kmem_cache_shutdown() would use it for the iteration over kmem_cache_node's to drain.
Quoted 29 lines · Shakeel Butt
The kasan quarantine is designed to delay freeing slab objects to catch use-after-free. The quarantine can be large (several percent of machine memory size). When
kmem_cachesare deleted related objects are flushed from the quarantine but this requires scanning the entire quarantine which can be very slow. We have seen the kernel busily working on this while holdingslab_mutexand badly affectingcache_reaper, slabinfo readers and memcg kmem cache creations.It can easily reproduced by following script:
yes . | head -1000000 | xargs stat > /dev/null for i in `seq 1 10`; do seq 500 | (cd /cg/memory && xargs mkdir) seq 500 | xargs -I{} sh -c 'echo $BASHPID > \ /cg/memory/{}/tasks && exec stat .' > /dev/null seq 500 | (cd /cg/memory && xargs rmdir) doneThe busy stack:
kasan_cache_shutdown
shutdown_cache
memcg_destroy_kmem_caches
mem_cgroup_css_free
css_free_rwork_fn
process_one_work
worker_thread
kthread
ret_from_fork
Quoted 9 lines
This patch is based on the observation that if the
kmem_cacheto be destroyed is empty then there should not be any objects of this cache in the quarantine.Without the patch the script got stuck for couple of hours. With the patch the script completed within a second.
Signed-off-by Shakeel Butt <[email protected]>
Quoted 7 lines · Shakeel Butt
The kasan quarantine is designed to delay freeing slab objects to catch use-after-free. The quarantine can be large (several percent of machine memory size). When
kmem_cachesare deleted related objects are flushed from the quarantine but this requires scanning the entire quarantine which can be very slow. We have seen the kernel busily working on this while holdingslab_mutexand badly affectingcache_reaper, slabinfo readers and memcg kmem cache creations.