Message-ID: <ZxX_gvuo8hhPlzvb@tiehlicka>
Date: Mon, 21 Oct 2024 09:15:14 +0200
From: Michal Hocko <mhocko@...e.com>
To: Joshua Hahn <joshua.hahnjy@...il.com>
Cc: Shakeel Butt <shakeel.butt@...ux.dev>,
	Johannes Weiner <hannes@...xchg.org>, nphamcs@...il.com,
	roman.gushchin@...ux.dev, muchun.song@...ux.dev,
	akpm@...ux-foundation.org, cgroups@...r.kernel.org,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, lnyng@...a.com
Subject: Re: [PATCH 0/1] memcg/hugetlb: Adding hugeTLB counters to memory
 controller

On Fri 18-10-24 14:38:48, Joshua Hahn wrote:
> On Fri, Oct 18, 2024 at 2:11 PM Shakeel Butt <shakeel.butt@...ux.dev> wrote:
> > On Fri, Oct 18, 2024 at 03:42:13PM GMT, Michal Hocko wrote:
[...]
> > > and it would be great to have an explanation why the lack of tracking
> > > has proven problematic. Also the above doesn't really explain why those
> > > who care cannot really enabled hugetlb controller to gain the
> > > consumption information.
> >
> > Let me give my take on this. The reason is the ease and convenience to
> > see what is happening when I see unexpectedly large memory.current
> > value. Logically I would look at memory.stat to make sense of it.
> > Without this I have to remember that the user might have hugetlb memcg
> > accounting option enabled and they are use hugetlb cgroup to find the
> > answer. If they have not enabled hugetlb cgroup then I am in dark.

Yes, I thought that this was an acceptable limitation of the accounting.
If that is not the case then it is really preferable to mention reasons
in the changelog. The reasoning was that hugetlb controller which would
be a natural source of that information is not really great because of
an overhead which hasn't really been evaluated - hence my questioning.

[...]

> Aside from consistency between the two files, we can see benefits in
> observability. There are many reasons userspace might be intersted in
> understanding the hugeTLB footprint of cgroups. To name a few, system
> administrators might want to verify that hugeTLB usage is distributed as
> expected across tasks: i.e. memory-intensive tasks are using more hugeTLB
> pages than tasks that don't consume a lot of memory, or is seen to fault
> frequently. Note that this is separate from wanting to inspect the
> distribution for limiting purposes (in that case, it makes sense to use
> the controller)

Please add this information into the changelog.

> 2. Why can't you enable the hugeTLB controller, if tracking is so important?
> 
> By turning on the hugeTLB controller, we gain all of the observability
> that I mentioned above; users can just check the respective hugetlb files.
> However, the discrepancy between memory.stat and memory.current is still
> there. When I check memory.current, I expect to be able to explain the usage
> by looking at memory.stat and trying to understand the breakdown, not by going
> into the various hugetlb controller files to check how/if the memory is
> accounted for.

Well, as mentioned in the previous response this has been an acceptable
limitation of the hugetlb accounting. It is fair to reconsider this
based on existing experience but that should be a part of the changelog.

> But even if we are okay with this, I think it might be overkill to
> enable the hugeTLB controller for the convenience of being able to inspect
> the hugeTLB usage for cgroups. This is especially true in workloads where
> we can predict what usage patterns will be like, and we do not need to enforce
> specific limits on hugeTLB usage.

I am sorry but I do not understand the overkill part of the argument.
Is there any runtime or configuration cost that is visible?

> 3. What if CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING is disabled?
> 
> This is a great point. The way the patch is currently implemented, it
> should still do the accounting to memory.stat, even if
> CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING is disabled. This would give us the reverse
> problem where hugeTLB usage that is reported in the statistics are no longer
> accounted for in current...

Exactly! And this is a problem.

> I think it makes sense to show hugeTLB statistics in memory.stat only if
> hugeTLB is accounted for in memory.current as well (i.e. check if
> CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING is enabled before doing the accounting,
> or move the accounting from hugeTLB alloc/free --> hugeTLB charge/uncharge,
> which should only happen if hugeTLBs are accounted for in memory.current).
> 
> What do you think?

yes, not only shown but also accounted only if the feature is enabled
because we do not want to introduce any (even tiny) overhead for feature
that is not enabled.

TL;DR
1) you need to make the stats accounting aligned with the existing
   charge accounting.
2) make the stat visible only when feature is enabled
3) work more on the justification - i.e. changelog part and give us a
   better insight why a) hugetlb cgroup is seen is a bad choice and b) why
   the original limitation hasn't proven good since the feature was
   introduced.

Makes sense?
-- 
Michal Hocko
SUSE Labs

Powered by blists - more mailing lists