Message-ID: <CAN+CAwOHE_J3yO=uMjAGamNKHFc7WXETDutvU=uWzNv5d33zYg@mail.gmail.com>
Date: Fri, 18 Oct 2024 14:38:48 -0400
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: Shakeel Butt <shakeel.butt@...ux.dev>
Cc: Michal Hocko <mhocko@...e.com>, 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, 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:
> > On Fri 18-10-24 08:31:22, Johannes Weiner wrote:
> > > On Fri, Oct 18, 2024 at 12:12:00PM +0200, Michal Hocko wrote:
> > > > On Thu 17-10-24 09:04:37, Joshua Hahn wrote:
> > > > > HugeTLB usage is a metric that can provide utility for monitors hoping
> > > > > to get more insight into the memory usage patterns in cgroups. It also
> > > > > helps identify if large folios are being distributed efficiently across
> > > > > workloads, so that tasks that can take most advantage of reduced TLB
> > > > > misses are prioritized.
> > > >
> > > > This seems really confusing because memcg controller is not responsible
> > > > for the hugetlb memory. Could you be more specific why enabling hugetlb
> > > > controller is not really desirable when the actual per-group tracking is
> > > > needed?
> > >
> > > However, we now have potentially a sizable portion of memory in
> > > memory.current that doesn't show up in memory.stat. Joshua's patch
> > > addresses that, so userspace can understand its memory footprint.
> > >
> > > I hope that makes sense.
> >
> > 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.
>
> >
> > Also what happens if CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING is disabled.
> > Should we report potentially misleading data?
>
> I think with what Johannes has suggested (to use lruvec_stat_mod_folio),
> the metric will only get updated when hugetlb memcg accounting is
> enabled and zero otherwise.

Hi Michal, Johannes, and Shakeel,

Thank you all for taking the time to review my patch.

I was writing my response when Shakeel responded, and I think it includes
an important point. I am sending out this message in the hopes that I can
gather insight on what direction would make most sense for everyone.

Michal -- You brought up several points in your response, so I'll do my
best to answer them below.

1. Why is the lack of tracking hugeTLB problematic?

The biggest problem that I see is that if CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING
is enabled, then there is a discrepancy between what is reported in
memory.stat and memory.current, as Johannes explained in his response above.

As Shakeel expanded as well, it is just convenient to have the value
explicitly there, so users don't have to go through and remember where
hugeTLB pages might be used and where they might not be used.

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)

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.

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.

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...

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?

Joshua

Powered by blists - more mailing lists