Message-ID: <CAN+CAwMCbX1BAmfBxFC6t75i5k6GVNKPR_QPCB5DDnYwHeCbnA@mail.gmail.com>
Date: Mon, 21 Oct 2024 10:51:43 -0400
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: Michal Hocko <mhocko@...e.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 Mon, Oct 21, 2024 at 3:15 AM Michal Hocko <mhocko@...e.com> wrote:
> > On Fri 18-10-24 14:38:48, Joshua Hahn wrote:
> > 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?

I think an argument could be made that any amount of incremental overhead
is unnecessary. With that said however, I think a bigger reason why this is
overkill is that a user who wishes to use the hugeTLB counter (which this
patch achieves in ~10 lines) should not have to enable a ~1000 line feature,
as Johannes suggested.

A diligent user will have to spend time learning how the hugeTLB controller
works and figuring out the settings that will basically make the controller
do no enforcing (and basically, the same as if the feature was not enabled).
A not-so-diligent user will not spend the time to make sure that the configs
make sense, and may run into unexpected & unwanted hugeTLB behavior [1].

> 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

Hi Michal,

Thank you for your input. Yes -- this makes sense to me. I apologize, as it
seems that I definitely left a lot to be assumed & inferred, based on my
original patch changelog.

In my next version of this patch, I am planning to add the changes that have
been suggested by Johannes, write code with the try_charge cleanup that
Shakeel suggested in mind, and change the behavior to make sense only when
hugeTLB accounting is enabled, as you suggested as well. I'll also update
the changelog & commit message and add any information that will hopefully
make future reviewers aware of the motivation for this patch.

Please let me know if you have any remaining concerns with the implementation
or motivation, and I will be happy to incorporate your ideas into the next
version as well.

Joshua

[1] Of course, this argument isn't really generalizable to *all* features,
we can't make a separate config for every small feature that a user might
want to enable with the smallest granularity. However, given that the existing
solution of enabling the hugeTLB controller is an imperfect solution that
still leaves a discrepancy between memory.stat and memory.current when hugeTLB
accounting is enabled, I think it is reasonable to isolate this feature.

Powered by blists - more mailing lists