Re: [PATCH v5 29/32] mm: memcontrol: prepare for reparenting non-hierarchical stats

[Yosry Ahmed <yosry@kernel.org>]

[..]
> >> @@ -506,12 +517,10 @@ void reparent_memcg_lruvec_state_local(struct
> >> mem_cgroup *memcg,
> >>          for_each_node(nid) {
> >>                  struct lruvec *child_lruvec = mem_cgroup_lruvec(memcg,
> >> NODE_DATA(nid));
> >>                  struct lruvec *parent_lruvec =
> >> mem_cgroup_lruvec(parent, NODE_DATA(nid));
> >> -               struct mem_cgroup_per_node *parent_pn;
> >>                  unsigned long value =
> >> lruvec_page_state_local(child_lruvec, idx);
> >>
> >> -               parent_pn = container_of(parent_lruvec, struct
> >> mem_cgroup_per_node, lruvec);
> >> -
> >> -               atomic_long_add(value,
> >> &(parent_pn->lruvec_stats->state_local[i]));
> >> +               mod_memcg_lruvec_state(child_lruvec, idx, -value);
> >
> > We can't use mod_memcg_lruvec_state() here, because child memcg has
> > already been set CSS_DYING. So in mod_memcg_lruvec_state(), we will
> > get parent memcg.
> >
> > It seems we need to reimplement a function or add a parameter to
> > mod_memcg_lruvec_state() to solve the problem. What do you think?
>
> Since child memcg is about to disappear, perhaps we can just add value
> to parent memcg without handling the child memcg. Make sense?

Ugh yes, I missed that, thanks.

I don't think we can just leave the child's memcg wrong. Aside from
the fact that I would be nervous if access to those stats is still
possible after it's offlined (e.g. can userspace already have the
stats file open, or maybe some in-kernel code uses it), there's a
bigger issue.

When the child cgroup is released, css_release_work_fn() will flush
its stats and then it will be double counted at the parent.

Maybe refactor the part sof mod_memcg_lruvec_state() and
mod_memcg_state () without get_non_dying_memcg_{start/end}() into
helpers, and call that directly from the reparenting functions? Adding
a boolean argument to mod_memcg_lruvec_state() and mod_memcg_state()
will add a lot of churn, and naked boolean arguments are not ideal.