[Devel] [PATCH rh7 v4 4/9] mm/mem_cgroup_iter: Always assign iter->last_visited under rcu

[Kirill Tkhai]

On 24.02.2021 21:55, Konstantin Khorenko wrote:
> It's quite strange to have rcu section in mem_cgroup_iter(),
> but do not use rcu_dereference/rcu_assign for pointers being defended.
> 
> We plan to access/assign '->last_visited' during iterator invalidation,
> so we'll need the protection there anyway.
> 
> https://jira.sw.ru/browse/PSBM-123655
> 
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> ---
>  mm/memcontrol.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8040f09425bf..d0251d27de00 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -194,8 +194,18 @@ struct mem_cgroup_reclaim_iter {
>  	/*
>  	 * last scanned hierarchy member. Valid only if last_dead_count
>  	 * matches memcg->dead_count of the hierarchy root group.
> +	 *
> +	 * Memory pointed by 'last_visited' is freed not earlier than
> +	 * one rcu period after we accessed it:
> +	 *   cgroup_offline_fn()
> +	 *    offline_css()
> +	 *    list_del_rcu()
> +	 *    dput()
> +	 *    ...
> +	 *     cgroup_diput()
> +	 *      call_rcu(&cgrp->rcu_head, cgroup_free_rcu)
>  	 */
> -	struct mem_cgroup *last_visited;
> +	struct mem_cgroup __rcu *last_visited;
>  	unsigned long last_dead_count;
>  
>  	/* scan generation, increased every round-trip */
> @@ -1591,8 +1601,7 @@ mem_cgroup_iter_load(struct mem_cgroup_reclaim_iter *iter,
>  	 */
>  	*sequence = atomic_read(&root->dead_count);
>  	if (iter->last_dead_count == *sequence) {
> -		smp_rmb();
> -		position = iter->last_visited;
> +		position = rcu_dereference(iter->last_visited);
>  
>  		/*
>  		 * We cannot take a reference to root because we might race
> @@ -1620,8 +1629,7 @@ static void mem_cgroup_iter_update(struct mem_cgroup_reclaim_iter *iter,
>  	 * don't lose destruction events in between.  We could have
>  	 * raced with the destruction of @new_position after all.
>  	 */
> -	iter->last_visited = new_position;
> -	smp_wmb();

We can't remove barriers in this patch, this makes the patch wrong.
We should remove barriers somewhere in [9/9].

> +	rcu_assign_pointer(iter->last_visited, new_position);
>  	iter->last_dead_count = sequence;
>  
>  	/* root reference counting symmetric to mem_cgroup_iter_load */
> @@ -1681,7 +1689,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>  			mz = mem_cgroup_zoneinfo(root, nid, zid);
>  			iter = &mz->reclaim_iter[reclaim->priority];
>  			if (prev && reclaim->generation != iter->generation) {
> -				iter->last_visited = NULL;
> +				rcu_assign_pointer(iter->last_visited, NULL);
>  				goto out_unlock;
>  			}
>  
>