Re: [v8 PATCH 03/13] mm: vmscan: use shrinker_rwsem to protect shrinker_maps allocation

[Shakeel Butt]

On Tue, Feb 16, 2021 at 4:13 PM Yang Shi <shy828301@xxxxxxxxx> wrote:
>
> Since memcg_shrinker_map_size just can be changed under holding shrinker_rwsem
> exclusively, the read side can be protected by holding read lock, so it sounds
> superfluous to have a dedicated mutex.
>
> Kirill Tkhai suggested use write lock since:
>
>   * We want the assignment to shrinker_maps is visible for shrink_slab_memcg().
>   * The rcu_dereference_protected() dereferrencing in shrink_slab_memcg(), but
>     in case of we use READ lock in alloc_shrinker_maps(), the dereferrencing
>     is not actually protected.
>   * READ lock makes alloc_shrinker_info() racy against memory allocation fail.
>     alloc_shrinker_info()->free_shrinker_info() may free memory right after
>     shrink_slab_memcg() dereferenced it. You may say
>     shrink_slab_memcg()->mem_cgroup_online() protects us from it? Yes, sure,
>     but this is not the thing we want to remember in the future, since this
>     spreads modularity.
>
> And a test with heavy paging workload didn't show write lock makes things worse.
>
> Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> Acked-by: Kirill Tkhai <ktkhai@xxxxxxxxxxxxx>
> Acked-by: Roman Gushchin <guro@xxxxxx>
> Signed-off-by: Yang Shi <shy828301@xxxxxxxxx>

Reviewed-by: Shakeel Butt <shakeelb@xxxxxxxxxx>