On Mon, Jan 11, 2021 at 9:34 AM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote:
>
> On 11.01.2021 20:08, Yang Shi wrote:
> > On Wed, Jan 6, 2021 at 1:55 AM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote:
> >>
> >> On 06.01.2021 01:58, Yang Shi wrote:
> >>> Since memcg_shrinker_map_size just can be changd under holding shrinker_rwsem
> >>> exclusively, the read side can be protected by holding read lock, so it sounds
> >>> superfluous to have a dedicated mutex. This should not exacerbate the contention
> >>> to shrinker_rwsem since just one read side critical section is added.
> >>>
> >>> Signed-off-by: Yang Shi <shy828301@xxxxxxxxx>
> >>> ---
> >>> mm/vmscan.c | 16 ++++++----------
> >>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/mm/vmscan.c b/mm/vmscan.c
> >>> index 9db7b4d6d0ae..ddb9f972f856 100644
> >>> --- a/mm/vmscan.c
> >>> +++ b/mm/vmscan.c
> >>> @@ -187,7 +187,6 @@ static DECLARE_RWSEM(shrinker_rwsem);
> >>> #ifdef CONFIG_MEMCG
> >>>
> >>> static int memcg_shrinker_map_size;
> >>> -static DEFINE_MUTEX(memcg_shrinker_map_mutex);
> >>>
> >>> static void memcg_free_shrinker_map_rcu(struct rcu_head *head)
> >>> {
> >>> @@ -200,8 +199,6 @@ static int memcg_expand_one_shrinker_map(struct mem_cgroup *memcg,
> >>> struct memcg_shrinker_map *new, *old;
> >>> int nid;
> >>>
> >>> - lockdep_assert_held(&memcg_shrinker_map_mutex);
> >>> -
> >>> for_each_node(nid) {
> >>> old = rcu_dereference_protected(
> >>> mem_cgroup_nodeinfo(memcg, nid)->shrinker_map, true);
> >>> @@ -250,7 +247,7 @@ int memcg_alloc_shrinker_maps(struct mem_cgroup *memcg)
> >>> if (mem_cgroup_is_root(memcg))
> >>> return 0;
> >>>
> >>> - mutex_lock(&memcg_shrinker_map_mutex);
> >>> + down_read(&shrinker_rwsem);
> >>> size = memcg_shrinker_map_size;
> >>> for_each_node(nid) {
> >>> map = kvzalloc(sizeof(*map) + size, GFP_KERNEL);
> >>> @@ -261,7 +258,7 @@ int memcg_alloc_shrinker_maps(struct mem_cgroup *memcg)
> >>> }
> >>> rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_map, map);
> >>
> >> Here we do STORE operation, and since we want the assignment is visible
> >> for shrink_slab_memcg() under down_read(), we have to use down_write()
> >> in memcg_alloc_shrinker_maps().
> >
> > I apologize for the late reply, these emails went to my SPAM again.
>
> This is the second time the problem appeared. Just add my email address to allow list,
> and there won't be this problem again.
Yes, I thought clicking "not spam" would add your email address to the
allow list automatically. But it turns out not true.
>
> > Before this patch it was not serialized by any lock either, right? Do
> > we have to serialize it? As Johannes mentioned if shrinker_maps has
> > not been initialized yet, it means the memcg is a newborn, there
> > should not be significant amount of reclaimable slab caches, so it is
> > fine to skip it. The point makes some sense to me.
> >
> > So, the read lock seems good enough.
>
> No, this is not so.
>
> Patch "[v3 PATCH 07/11] mm: vmscan: add per memcg shrinker nr_deferred" adds
> new assignments:
>
> + info->map = (unsigned long *)((unsigned long)info + sizeof(*info));
> + info->nr_deferred = (atomic_long_t *)((unsigned long)info +
> + sizeof(*info) + m_size);
>
> info->map and info->nr_deferred are not visible under READ lock in shrink_slab_memcg(),
> unless you use WRITE lock in memcg_alloc_shrinker_maps().
However map and nr_deferred are assigned before
rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, new). The
shrink_slab_memcg() checks shrinker_info pointer.
But that order might be not guaranteed, so it seems a memory barrier
before rcu_assign_pointer should be good enough, right?
>
> Nowhere in your patchset you convert READ lock to WRITE lock in memcg_alloc_shrinker_maps().
>
> So, just use the true lock in this patch from the first time.
>
> >>
> >>> }
> >>> - mutex_unlock(&memcg_shrinker_map_mutex);
> >>> + up_read(&shrinker_rwsem);
> >>>
> >>> return ret;
> >>> }
> >>> @@ -276,9 +273,8 @@ static int memcg_expand_shrinker_maps(int new_id)
> >>> if (size <= old_size)
> >>> return 0;
> >>>
> >>> - mutex_lock(&memcg_shrinker_map_mutex);
> >>> if (!root_mem_cgroup)
> >>> - goto unlock;
> >>> + goto out;
> >>>
> >>> memcg = mem_cgroup_iter(NULL, NULL, NULL);
> >>> do {
> >>> @@ -287,13 +283,13 @@ static int memcg_expand_shrinker_maps(int new_id)
> >>> ret = memcg_expand_one_shrinker_map(memcg, size, old_size);
> >>> if (ret) {
> >>> mem_cgroup_iter_break(NULL, memcg);
> >>> - goto unlock;
> >>> + goto out;
> >>> }
> >>> } while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)) != NULL);
> >>> -unlock:
> >>> +out:
> >>> if (!ret)
> >>> memcg_shrinker_map_size = size;
> >>> - mutex_unlock(&memcg_shrinker_map_mutex);
> >>> +
> >>> return ret;
> >>> }
> >>>
> >>>
> >>
> >>
>
>
