Messages in this thread | | | Date | Wed, 22 Feb 2023 15:32:00 +0800 | Subject | Re: [PATCH 2/5] mm: vmscan: make memcg slab shrink lockless | From | Qi Zheng <> |
| |
On 2023/2/22 05:28, Kirill Tkhai wrote: > On 20.02.2023 12:16, Qi Zheng wrote: <...> >> >> void reparent_shrinker_deferred(struct mem_cgroup *memcg) >> { >> - int i, nid; >> + int i, nid, srcu_idx; >> long nr; >> struct mem_cgroup *parent; >> struct shrinker_info *child_info, *parent_info; >> @@ -429,16 +443,16 @@ void reparent_shrinker_deferred(struct mem_cgroup *memcg) >> parent = root_mem_cgroup; >> >> /* Prevent from concurrent shrinker_info expand */ >> - down_read(&shrinker_rwsem); >> + srcu_idx = srcu_read_lock(&shrinker_srcu); > > Don't we still have to be protected against parallel expand_one_shrinker_info()? > > It looks like parent->nodeinfo[nid]->shrinker_info pointer may be changed in expand* > right after we've dereferenced it here.
Hi Kirill,
Oh, indeed. We may wrongly reparent the child's nr_deferred to the old parent's nr_deferred (it is about to be freed by call_srcu).
The reparent_shrinker_deferred() will only be called on the offline path (not a hotspot path), so we may be able to use shrinker_mutex introduced later for protection. What do you think?
Thanks, Qi
> >> for_each_node(nid) { >> - child_info = shrinker_info_protected(memcg, nid); >> - parent_info = shrinker_info_protected(parent, nid); >> + child_info = shrinker_info_srcu(memcg, nid); >> + parent_info = shrinker_info_srcu(parent, nid); >> for (i = 0; i < shrinker_nr_max; i++) { >> nr = atomic_long_read(&child_info->nr_deferred[i]); >> atomic_long_add(nr, &parent_info->nr_deferred[i]); >> } >> } >> - up_read(&shrinker_rwsem); >> + srcu_read_unlock(&shrinker_srcu, srcu_idx); >> } >> >> static bool cgroup_reclaim(struct scan_control *sc) >> @@ -891,15 +905,14 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid, >> { >> struct shrinker_info *info; >> unsigned long ret, freed = 0; >> + int srcu_idx; >> int i; >> >> if (!mem_cgroup_online(memcg)) >> return 0; >> >> - if (!down_read_trylock(&shrinker_rwsem)) >> - return 0; >> - >> - info = shrinker_info_protected(memcg, nid); >> + srcu_idx = srcu_read_lock(&shrinker_srcu); >> + info = shrinker_info_srcu(memcg, nid); >> if (unlikely(!info)) >> goto unlock; >> >> @@ -949,14 +962,9 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid, >> 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_idx); >> return freed; >> } >> #else /* CONFIG_MEMCG */ >
-- Thanks, Qi
| |