Re: [PATCH] mm: memcg/slab: enable slab memory accounting atomically

[Roman Gushchin <guro@fb.com>]

On Tue, Nov 10, 2020 at 10:21:43AM -0500, Johannes Weiner wrote:
> On Mon, Nov 09, 2020 at 05:06:15PM -0800, Roman Gushchin wrote:
> > Many kernel memory accounting paths are guarded by the
> > memcg_kmem_enabled_key static key. It changes it's state during
> > the onlining of the first non-root cgroup. However is doesn't
> > happen atomically: before all call sites will become patched
> > some charges/uncharges can be skipped, resulting in an unbalanced
> > charge. The problem is mostly a theoretical issue, unlikely
> > having a noticeable impact ion the real life.
> 
> memcg_online_kmem() is called from .css_alloc rather than .css_online,
> at a time where memcg is brandnew and the css hasn't been set up and
> published yet (the refcount isn't even initialized for css_tryget()).
> 
> I don't really see how charges could race with that.

You're right. I was thinking of it in the context of a patchset causing
the root memory cgroup to be charged as any other cgroup. In that case,
the root memory cgroup can be accessed and charged as soon as we set
root_mem_cgroup, and it happens exactly in mem_cgroup_css_alloc().
As in the current state, it's indeed not a problem.

> 
> We may want to rename memcg_online_kmem() to memcg_alloc_kmem() or
> something.
> 
> > Before the rework of the slab controller we relied at setting
> > kmemcg_id after enabling the memcg_kmem_enabled_key static key.
> > Now we can use the setting of memcg->objcg to enable the slab
> > memory accounting atomically.
> 
> I suspect that code comment has been out of date since commit
> 0b8f73e104285a4badf9d768d1c39b06d77d1f97.

I'll send a patch to cleanup these obsolete comments.

Thanks!