Re: [RESEND] [PATCH v1 1/3] Add basic infrastructure for memcg hotplug support

[Vladimir Davydov <vdavydov@tarantool.org>]

On Thu, Nov 17, 2016 at 11:28:12AM +1100, Balbir Singh wrote:
> >> @@ -5773,6 +5771,59 @@ static int __init cgroup_memory(char *s)
> >>  }
> >>  __setup("cgroup.memory=", cgroup_memory);
> >>  
> >> +static void memcg_node_offline(int node)
> >> +{
> >> +	struct mem_cgroup *memcg;
> >> +
> >> +	if (node < 0)
> >> +		return;
> > 
> > Is this possible?
> 
> Yes, please see node_states_check_changes_online/offline

OK, I see.

> 
> > 
> >> +
> >> +	for_each_mem_cgroup(memcg) {
> >> +		free_mem_cgroup_per_node_info(memcg, node);
> >> +		mem_cgroup_may_update_nodemask(memcg);
> > 
> > If memcg->numainfo_events is 0, mem_cgroup_may_update_nodemask() won't
> > update memcg->scan_nodes. Is it OK?
> > 
> >> +	}
> > 
> > What if a memory cgroup is created or destroyed while you're walking the
> > tree? Should we probably use get_online_mems() in mem_cgroup_alloc() to
> > avoid that?
> > 
> 
> The iterator internally takes rcu_read_lock() to avoid any side-effects
> of cgroups added/removed. I suspect you are also suggesting using get_online_mems()
> around each call to for_each_online_node
> 
> My understanding so far is
> 
> 1. invalidate_reclaim_iterators should be safe (no bad side-effects)
> 2. mem_cgroup_free - should be safe as well
> 3. mem_cgroup_alloc - needs protection
> 4. mem_cgroup_init - needs protection
> 5. mem_cgroup_remove_from_tress - should be safe

I'm not into the memory hotplug code, but my understanding is that if
memcg offline happens to race with node unplug, it's possible that

 - mem_cgroup_free() doesn't free the node's data, because it sees the
   node as already offline
 - memcg hotplug code doesn't free the node's data either, because it
   sees the cgroup as offline

May be, we should surround all the loops over online nodes with
get/put_online_mems() to be sure that nothing wrong can happen.
They are slow path, anyway.

Thanks,
Vladimir