On Fri, Oct 18, 2024 at 2:11 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> 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
