| Message ID | 153365626605.19074.16202958374930777592.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce lockless shrink_slab() | expand |
This also requires call_srcu() in memcg_expand_one_shrinker_map() and srcu_dereference() in shrink_slab_memcg(). The updated patch is below: [PATCH] mm: Make shrink_slab() lockless From: Kirill Tkhai <ktkhai@virtuozzo.com> The patch makes shrinker list and shrinker_idr SRCU-safe for readers. This requires synchronize_srcu() on finalize stage unregistering stage, which waits till all parallel shrink_slab() are finished Note, that patch removes rwsem_is_contended() checks from the code, and this does not result in delays during registration, since there is no waiting at all. Unregistration case may be optimized by splitting unregister_shrinker() in tho stages, and this is made in next patches. Also, keep in mind, that in case of SRCU is not allowed to make unconditional (which is done in previous patch), it is possible to use percpu_rw_semaphore instead of it. percpu_down_read() will be used in shrink_slab_memcg() and in shrink_slab(), and consecutive calls percpu_down_write(percpu_rwsem); percpu_up_write(percpu_rwsem); will be used instead of synchronize_srcu(). Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> --- include/linux/shrinker.h | 2 ++ mm/memcontrol.c | 2 +- mm/vmscan.c | 46 +++++++++++++++------------------------------- 3 files changed, 18 insertions(+), 32 deletions(-) diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h index 9443cafd1969..94b44662f430 100644 --- a/include/linux/shrinker.h +++ b/include/linux/shrinker.h @@ -82,6 +82,8 @@ struct shrinker { #define SHRINKER_NUMA_AWARE (1 << 0) #define SHRINKER_MEMCG_AWARE (1 << 1) +extern struct srcu_struct shrinker_srcu; + extern int prealloc_shrinker(struct shrinker *shrinker); extern void register_shrinker_prepared(struct shrinker *shrinker); extern int register_shrinker(struct shrinker *shrinker); diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 4e3c1315b1de..a30bc2cc6380 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -347,7 +347,7 @@ static int memcg_expand_one_shrinker_map(struct mem_cgroup *memcg, memset((void *)new->map + old_size, 0, size - old_size); rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_map, new); - call_rcu(&old->rcu, memcg_free_shrinker_map_rcu); + call_srcu(&shrinker_srcu, &old->rcu, memcg_free_shrinker_map_rcu); } return 0; diff --git a/mm/vmscan.c b/mm/vmscan.c index da135e1acd94..acb087f3ac35 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -168,6 +168,7 @@ unsigned long vm_total_pages; static LIST_HEAD(shrinker_list); static DECLARE_RWSEM(shrinker_rwsem); +DEFINE_SRCU(shrinker_srcu); #ifdef CONFIG_MEMCG_KMEM @@ -192,7 +193,6 @@ static int prealloc_memcg_shrinker(struct shrinker *shrinker) int id, ret = -ENOMEM; down_write(&shrinker_rwsem); - /* This may call shrinker, so it must use down_read_trylock() */ id = idr_alloc(&shrinker_idr, SHRINKER_REGISTERING, 0, 0, GFP_KERNEL); if (id < 0) goto unlock; @@ -406,7 +406,7 @@ void free_prealloced_shrinker(struct shrinker *shrinker) void register_shrinker_prepared(struct shrinker *shrinker) { down_write(&shrinker_rwsem); - list_add_tail(&shrinker->list, &shrinker_list); + list_add_tail_rcu(&shrinker->list, &shrinker_list); idr_replace(&shrinker_idr, shrinker, shrinker->id); up_write(&shrinker_rwsem); } @@ -432,8 +432,11 @@ void unregister_shrinker(struct shrinker *shrinker) if (shrinker->flags & SHRINKER_MEMCG_AWARE) unregister_memcg_shrinker(shrinker); down_write(&shrinker_rwsem); - list_del(&shrinker->list); + list_del_rcu(&shrinker->list); up_write(&shrinker_rwsem); + + synchronize_srcu(&shrinker_srcu); + kfree(shrinker->nr_deferred); shrinker->nr_deferred = NULL; } @@ -567,16 +570,14 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid, { struct memcg_shrinker_map *map; unsigned long freed = 0; - int ret, i; + int ret, i, srcu_id; if (!memcg_kmem_enabled() || !mem_cgroup_online(memcg)) return 0; - if (!down_read_trylock(&shrinker_rwsem)) - return 0; - - map = rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_map, - true); + srcu_id = srcu_read_lock(&shrinker_srcu); + map = srcu_dereference(memcg->nodeinfo[nid]->shrinker_map, + &shrinker_srcu); if (unlikely(!map)) goto unlock; @@ -621,14 +622,9 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid, memcg_set_shrinker_bit(memcg, nid, i); } freed += ret; - - if (rwsem_is_contended(&shrinker_rwsem)) { - freed = freed ? : 1; - break; - } } unlock: - up_read(&shrinker_rwsem); + srcu_read_unlock(&shrinker_srcu, srcu_id); return freed; } #else /* CONFIG_MEMCG_KMEM */ @@ -665,15 +661,13 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid, { struct shrinker *shrinker; unsigned long freed = 0; - int ret; + int srcu_id, ret; if (!mem_cgroup_is_root(memcg)) return shrink_slab_memcg(gfp_mask, nid, memcg, priority); - if (!down_read_trylock(&shrinker_rwsem)) - goto out; - - list_for_each_entry(shrinker, &shrinker_list, list) { + srcu_id = srcu_read_lock(&shrinker_srcu); + list_for_each_entry_rcu(shrinker, &shrinker_list, list) { struct shrink_control sc = { .gfp_mask = gfp_mask, .nid = nid, @@ -684,19 +678,9 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid, if (ret == SHRINK_EMPTY) ret = 0; freed += ret; - /* - * Bail out if someone want to register a new shrinker to - * prevent the regsitration from being stalled for long periods - * by parallel ongoing shrinking. - */ - if (rwsem_is_contended(&shrinker_rwsem)) { - freed = freed ? : 1; - break; - } } + srcu_read_unlock(&shrinker_srcu, srcu_id); - up_read(&shrinker_rwsem); -out: cond_resched(); return freed; }
On 2018/08/08 20:51, Kirill Tkhai wrote: > @@ -192,7 +193,6 @@ static int prealloc_memcg_shrinker(struct shrinker *shrinker) > int id, ret = -ENOMEM; > > down_write(&shrinker_rwsem); > - /* This may call shrinker, so it must use down_read_trylock() */ > id = idr_alloc(&shrinker_idr, SHRINKER_REGISTERING, 0, 0, GFP_KERNEL); > if (id < 0) > goto unlock; I don't know why perf reports down_read_trylock(&shrinker_rwsem). But above code is already bad. GFP_KERNEL allocation involves shrinkers and the OOM killer would be invoked because shrinkers are defunctional due to this down_write(&shrinker_rwsem). Please avoid blocking memory allocation with shrinker_rwsem held.
On 08.08.2018 15:36, Tetsuo Handa wrote: > On 2018/08/08 20:51, Kirill Tkhai wrote: >> @@ -192,7 +193,6 @@ static int prealloc_memcg_shrinker(struct shrinker *shrinker) >> int id, ret = -ENOMEM; >> >> down_write(&shrinker_rwsem); >> - /* This may call shrinker, so it must use down_read_trylock() */ >> id = idr_alloc(&shrinker_idr, SHRINKER_REGISTERING, 0, 0, GFP_KERNEL); >> if (id < 0) >> goto unlock; > > I don't know why perf reports down_read_trylock(&shrinker_rwsem). This happens in the case of many cgroups and mounts on node. This is often happen on the big machines with containers. > But above code is already bad. GFP_KERNEL allocation involves shrinkers and > the OOM killer would be invoked because shrinkers are defunctional due to > this down_write(&shrinker_rwsem). Please avoid blocking memory allocation > with shrinker_rwsem held. There was non-blocking allocation in first versions of the patchset, but it's gone away in the process of the review (CC Vladimir). There are still pages lists shrinkers in case of shrink_slab() is not available, while additional locks makes the code more difficult and not worth this difficulties. Kirill
diff --git a/mm/vmscan.c b/mm/vmscan.c index da135e1acd94..9dda903a1406 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -168,6 +168,7 @@ unsigned long vm_total_pages; static LIST_HEAD(shrinker_list); static DECLARE_RWSEM(shrinker_rwsem); +DEFINE_STATIC_SRCU(srcu); #ifdef CONFIG_MEMCG_KMEM @@ -192,7 +193,6 @@ static int prealloc_memcg_shrinker(struct shrinker *shrinker) int id, ret = -ENOMEM; down_write(&shrinker_rwsem); - /* This may call shrinker, so it must use down_read_trylock() */ id = idr_alloc(&shrinker_idr, SHRINKER_REGISTERING, 0, 0, GFP_KERNEL); if (id < 0) goto unlock; @@ -406,7 +406,7 @@ void free_prealloced_shrinker(struct shrinker *shrinker) void register_shrinker_prepared(struct shrinker *shrinker) { down_write(&shrinker_rwsem); - list_add_tail(&shrinker->list, &shrinker_list); + list_add_tail_rcu(&shrinker->list, &shrinker_list); idr_replace(&shrinker_idr, shrinker, shrinker->id); up_write(&shrinker_rwsem); } @@ -432,8 +432,11 @@ void unregister_shrinker(struct shrinker *shrinker) if (shrinker->flags & SHRINKER_MEMCG_AWARE) unregister_memcg_shrinker(shrinker); down_write(&shrinker_rwsem); - list_del(&shrinker->list); + list_del_rcu(&shrinker->list); up_write(&shrinker_rwsem); + + synchronize_srcu(&srcu); + kfree(shrinker->nr_deferred); shrinker->nr_deferred = NULL; } @@ -567,14 +570,12 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid, { struct memcg_shrinker_map *map; unsigned long freed = 0; - int ret, i; + int ret, i, srcu_id; if (!memcg_kmem_enabled() || !mem_cgroup_online(memcg)) return 0; - if (!down_read_trylock(&shrinker_rwsem)) - return 0; - + srcu_id = srcu_read_lock(&srcu); map = rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_map, true); if (unlikely(!map)) @@ -621,14 +622,9 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid, memcg_set_shrinker_bit(memcg, nid, i); } freed += ret; - - if (rwsem_is_contended(&shrinker_rwsem)) { - freed = freed ? : 1; - break; - } } unlock: - up_read(&shrinker_rwsem); + srcu_read_unlock(&srcu, srcu_id); return freed; } #else /* CONFIG_MEMCG_KMEM */ @@ -665,15 +661,13 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid, { struct shrinker *shrinker; unsigned long freed = 0; - int ret; + int srcu_id, ret; if (!mem_cgroup_is_root(memcg)) return shrink_slab_memcg(gfp_mask, nid, memcg, priority); - if (!down_read_trylock(&shrinker_rwsem)) - goto out; - - list_for_each_entry(shrinker, &shrinker_list, list) { + srcu_id = srcu_read_lock(&srcu); + list_for_each_entry_rcu(shrinker, &shrinker_list, list) { struct shrink_control sc = { .gfp_mask = gfp_mask, .nid = nid, @@ -684,19 +678,9 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid, if (ret == SHRINK_EMPTY) ret = 0; freed += ret; - /* - * Bail out if someone want to register a new shrinker to - * prevent the regsitration from being stalled for long periods - * by parallel ongoing shrinking. - */ - if (rwsem_is_contended(&shrinker_rwsem)) { - freed = freed ? : 1; - break; - } } + srcu_read_unlock(&srcu, srcu_id); - up_read(&shrinker_rwsem); -out: cond_resched(); return freed; }
The patch makes shrinker list and shrinker_idr SRCU-safe for readers. This requires synchronize_srcu() on finalize stage unregistering stage, which waits till all parallel shrink_slab() are finished Note, that patch removes rwsem_is_contended() checks from the code, and this does not result in delays during registration, since there is no waiting at all. Unregistration case may be optimized by splitting unregister_shrinker() in tho stages, and this is made in next patches. Also, keep in mind, that in case of SRCU is not allowed to make unconditional (which is done in previous patch), it is possible to use percpu_rw_semaphore instead of it. percpu_down_read() will be used in shrink_slab_memcg() and in shrink_slab(), and consecutive calls percpu_down_write(percpu_rwsem); percpu_up_write(percpu_rwsem); will be used instead of synchronize_srcu(). Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> --- mm/vmscan.c | 42 +++++++++++++----------------------------- 1 file changed, 13 insertions(+), 29 deletions(-)