On Tue, Apr 23, 2024 at 11:30 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote: > > On Tue, Apr 23, 2024 at 10:44:07AM -0700, Shakeel Butt wrote: > > On Tue, Apr 23, 2024 at 09:58:44AM -0400, Johannes Weiner wrote: > > > On Mon, Apr 22, 2024 at 10:18:23PM -0700, Shakeel Butt wrote: > > > > At the moment the memcg stats are sized based on the size of enum > > > > node_stat_item but not all fields in node_stat_item corresponds to memcg > > > > stats. So, rearrage the contents of node_stat_item such that all the > > > > memcg specific stats are at the top and then the later patches will make > > > > sure that the memcg code will not waste space for non-memcg stats. > > > > > > > > Signed-off-by: Shakeel Butt <shakeel.butt@xxxxxxxxx> > > > > > > This series is a great idea and the savings speak for themselves. > > > > > > But rearranging and splitting vmstats along the memcg-nomemcg line > > > seems like an undue burden on the non-memcg codebase and interface. > > > > > > - It messes with user-visible /proc/vmstat ordering, and sets things > > > up to do so on an ongoing basis as stats are added to memcg. > > > > > > - It also separates related stats (like the workingset ones) in > > > /proc/vmstat when memcg only accounts a subset. > > > > > > Would it make more sense to have a translation table inside memcg? > > > Like we have with memcg1_events. > > > > Thanks for taking a look. I will look into the translation table > > approach. The reason I went with this approach was that I am in parallel > > looking into rearranging fields of important MM structs and also enums > > to improve cache locality. For example, the field NR_SWAPCACHE is always > > accessed together with NR_FILE_PAGES, so it makes sense to have them on > > same cacheline. So, is the rearrangement of vmstats a big NO or a little > > bit here and there is fine unlike what I did with this patch? > > I'm curious what other folks think. > I slightly prefer not to change user visible ordering for no good reason. It is not said the order is carved to stone. It depends on the ROI. > The cache optimization is a stronger argument, IMO, because it > directly benefits the users of /proc/vmstat. And it would be fairly > self contained inside the node_stat_item enum - "ordered for cache". Not sure how much of the cache optimization is measurable here. I suspect it is going to be hard to measure a meaningful difference just from the cache line order alone. > I was more hesitant about imposing a memcg requirement on the generic > vmstat ordering. That is a valid reason. Chris
