Re: [PATCH] memcg: Remove memcg_cgroup::id from IDR on mem_cgroup_css_alloc() failure

On 01.08.2018 19:22, Vladimir Davydov wrote:

On Wed, Aug 01, 2018 at 11:55:52AM -0400, Johannes Weiner wrote:

quoted

On Tue, Jul 31, 2018 at 04:39:08PM -0700, Andrew Morton wrote:

quoted

On Mon, 30 Jul 2018 11:31:13 -0400 Johannes Weiner [off-list ref] wrote:

quoted

Subject: [PATCH] mm: memcontrol: simplify memcg idr allocation and error
 unwinding

The memcg ID is allocated early in the multi-step memcg creation
process, which needs 2-step ID allocation and IDR publishing, as well
as two separate IDR cleanup/unwind sites on error.

Defer the IDR allocation until the last second during onlining to
eliminate all this complexity. There is no requirement to have the ID
and IDR entry earlier than that. And the root reference to the ID is
put in the offline path, so this matches nicely.

This patch isn't aware of Kirill's later "mm, memcg: assign memcg-aware
shrinkers bitmap to memcg", which altered mem_cgroup_css_online():

@@ -4356,6 +4470,11 @@ static int mem_cgroup_css_online(struct
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
 
+	if (memcg_alloc_shrinker_maps(memcg)) {
+		mem_cgroup_id_remove(memcg);
+		return -ENOMEM;
+	}
+
 	/* Online state pins memcg ID, memcg ID pins CSS */
 	atomic_set(&memcg->id.ref, 1);
 	css_get(css);

Hm, that looks out of place too. The bitmaps are allocated for the
entire lifetime of the css, not just while it's online.

Any objections to the following fixup to that patch?

That would be incorrect. Memory cgroups that haven't been put online
are invisible to for_each_mem_cgroup(), which is used for expanding
shrinker maps of all cgroups - see memcg_expand_shrinker_maps(). So if
memcg_expand_shrinker_maps() is called between css_alloc and css_online,
it will miss this cgroup and its shrinker_map won't be reallocated to
fit the new id. Allocating the shrinker map in css_online guarantees
that it won't happen.

Yes, doubtless.

Looks like this code lacks a comment...

Ok.