| Message ID | 20220816185801.651091-1-shy828301@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: memcg: export workingset refault stats for cgroup v1 | expand |
On Tue, Aug 16, 2022 at 11:58 AM Yang Shi <shy828301@gmail.com> wrote: > > Workingset refault stats are important and usefule metrics to measure > how well reclaimer and swapping work and how healthy the services are, > but they are just available for cgroup v2. There are still plenty users > with cgroup v1, export the stats for cgroup v1. > > Signed-off-by: Yang Shi <shy828301@gmail.com> > --- > I do understand the development of cgroup v1 is actually stalled and > the community is reluctant to accept new features for v1. However > the workingset refault stats are really quite useful and exporting > two new stats, which have been supported by v2, seems ok IMHO. So > hopefully this patch could be considered. Thanks. > Is just workingset refault good enough for your use-case? What about the other workingset stats? I don't have a strong opinion against adding these to v1 and I think these specific stats should be fine. (There is subtlety in exposing objcg based stats (i.e. reparenting) in v1 due to non-hierarchical stats in v1. I remember Yosry and Muchun were looking into that.)
On Tue, Aug 16, 2022 at 3:06 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Tue, Aug 16, 2022 at 11:58 AM Yang Shi <shy828301@gmail.com> wrote: > > > > Workingset refault stats are important and usefule metrics to measure > > how well reclaimer and swapping work and how healthy the services are, > > but they are just available for cgroup v2. There are still plenty users > > with cgroup v1, export the stats for cgroup v1. > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > --- > > I do understand the development of cgroup v1 is actually stalled and > > the community is reluctant to accept new features for v1. However > > the workingset refault stats are really quite useful and exporting > > two new stats, which have been supported by v2, seems ok IMHO. So > > hopefully this patch could be considered. Thanks. > > > > Is just workingset refault good enough for your use-case? What about > the other workingset stats? I don't have a strong opinion against > adding these to v1 and I think these specific stats should be fine. > (There is subtlety in exposing objcg based stats (i.e. reparenting) in > v1 due to non-hierarchical stats in v1. I remember Yosry and Muchun > were looking into that.) I think only kernel memory stats and zswap stats are objcg-based at this point, right? The workingset refault stats seem to be memcg-based and should not face the reparenting problem.
On Tue, Aug 16, 2022 at 3:06 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Tue, Aug 16, 2022 at 11:58 AM Yang Shi <shy828301@gmail.com> wrote: > > > > Workingset refault stats are important and usefule metrics to measure > > how well reclaimer and swapping work and how healthy the services are, > > but they are just available for cgroup v2. There are still plenty users > > with cgroup v1, export the stats for cgroup v1. > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > --- > > I do understand the development of cgroup v1 is actually stalled and > > the community is reluctant to accept new features for v1. However > > the workingset refault stats are really quite useful and exporting > > two new stats, which have been supported by v2, seems ok IMHO. So > > hopefully this patch could be considered. Thanks. > > > > Is just workingset refault good enough for your use-case? What about > the other workingset stats? I don't have a strong opinion against > adding these to v1 and I think these specific stats should be fine. The workingset refault is good enough for our usercase, but I don't mind adding all the workingset_* stats if nobody has objection. > (There is subtlety in exposing objcg based stats (i.e. reparenting) in > v1 due to non-hierarchical stats in v1. I remember Yosry and Muchun > were looking into that.) The workingset_* stats should have nothing to do with obj based stats IIUC.
On Tue, Aug 16, 2022 at 7:01 PM Yang Shi <shy828301@gmail.com> wrote: > > On Tue, Aug 16, 2022 at 3:06 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > On Tue, Aug 16, 2022 at 11:58 AM Yang Shi <shy828301@gmail.com> wrote: > > > > > > Workingset refault stats are important and usefule metrics to measure > > > how well reclaimer and swapping work and how healthy the services are, > > > but they are just available for cgroup v2. There are still plenty users > > > with cgroup v1, export the stats for cgroup v1. > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > --- > > > I do understand the development of cgroup v1 is actually stalled and > > > the community is reluctant to accept new features for v1. However > > > the workingset refault stats are really quite useful and exporting > > > two new stats, which have been supported by v2, seems ok IMHO. So > > > hopefully this patch could be considered. Thanks. > > > > > > > Is just workingset refault good enough for your use-case? What about > > the other workingset stats? I don't have a strong opinion against > > adding these to v1 and I think these specific stats should be fine. > > The workingset refault is good enough for our usercase, but I don't > mind adding all the workingset_* stats if nobody has objection. For now let's just start with what your use-case needs. If in future there is a need we can add other workingset_* stats as well. > > > (There is subtlety in exposing objcg based stats (i.e. reparenting) in > > v1 due to non-hierarchical stats in v1. I remember Yosry and Muchun > > were looking into that.) > > The workingset_* stats should have nothing to do with obj based stats IIUC. Yeah, that was just FYI for anyone in future who wants to export such stat in v1.
On Tue, Aug 16, 2022 at 7:05 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Tue, Aug 16, 2022 at 7:01 PM Yang Shi <shy828301@gmail.com> wrote: > > > > On Tue, Aug 16, 2022 at 3:06 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > > > On Tue, Aug 16, 2022 at 11:58 AM Yang Shi <shy828301@gmail.com> wrote: > > > > > > > > Workingset refault stats are important and usefule metrics to measure > > > > how well reclaimer and swapping work and how healthy the services are, > > > > but they are just available for cgroup v2. There are still plenty users > > > > with cgroup v1, export the stats for cgroup v1. > > > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > > --- > > > > I do understand the development of cgroup v1 is actually stalled and > > > > the community is reluctant to accept new features for v1. However > > > > the workingset refault stats are really quite useful and exporting > > > > two new stats, which have been supported by v2, seems ok IMHO. So > > > > hopefully this patch could be considered. Thanks. > > > > > > > > > > Is just workingset refault good enough for your use-case? What about > > > the other workingset stats? I don't have a strong opinion against > > > adding these to v1 and I think these specific stats should be fine. > > > > The workingset refault is good enough for our usercase, but I don't > > mind adding all the workingset_* stats if nobody has objection. > > For now let's just start with what your use-case needs. If in future > there is a need we can add other workingset_* stats as well. Sure, works for me. > > > > > > (There is subtlety in exposing objcg based stats (i.e. reparenting) in > > > v1 due to non-hierarchical stats in v1. I remember Yosry and Muchun > > > were looking into that.) > > > > The workingset_* stats should have nothing to do with obj based stats IIUC. > > Yeah, that was just FYI for anyone in future who wants to export such > stat in v1. Thanks, Shakeel. If it looks good to me, would you please ack the patch?
On Tue, Aug 16, 2022 at 11:58 AM Yang Shi <shy828301@gmail.com> wrote: > > Workingset refault stats are important and usefule metrics to measure *useful > how well reclaimer and swapping work and how healthy the services are, > but they are just available for cgroup v2. There are still plenty users > with cgroup v1, export the stats for cgroup v1. > > Signed-off-by: Yang Shi <shy828301@gmail.com> Acked-by: Shakeel Butt <shakeelb@google.com>
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index b69979c9ced5..e300437896dc 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3975,6 +3975,8 @@ static const unsigned int memcg1_stats[] = { NR_FILE_MAPPED, NR_FILE_DIRTY, NR_WRITEBACK, + WORKINGSET_REFAULT_ANON, + WORKINGSET_REFAULT_FILE, MEMCG_SWAP, }; @@ -3988,6 +3990,8 @@ static const char *const memcg1_stat_names[] = { "mapped_file", "dirty", "writeback", + "workingset_refault_anon", + "workingset_refault_file", "swap", }; @@ -4016,7 +4020,8 @@ static int memcg_stat_show(struct seq_file *m, void *v) if (memcg1_stats[i] == MEMCG_SWAP && !do_memsw_account()) continue; nr = memcg_page_state_local(memcg, memcg1_stats[i]); - seq_printf(m, "%s %lu\n", memcg1_stat_names[i], nr * PAGE_SIZE); + seq_printf(m, "%s %lu\n", memcg1_stat_names[i], + nr * memcg_page_state_unit(memcg1_stats[i])); } for (i = 0; i < ARRAY_SIZE(memcg1_events); i++) @@ -4047,7 +4052,7 @@ static int memcg_stat_show(struct seq_file *m, void *v) continue; nr = memcg_page_state(memcg, memcg1_stats[i]); seq_printf(m, "total_%s %llu\n", memcg1_stat_names[i], - (u64)nr * PAGE_SIZE); + (u64)nr * memcg_page_state_unit(memcg1_stats[i])); } for (i = 0; i < ARRAY_SIZE(memcg1_events); i++)
Workingset refault stats are important and usefule metrics to measure how well reclaimer and swapping work and how healthy the services are, but they are just available for cgroup v2. There are still plenty users with cgroup v1, export the stats for cgroup v1. Signed-off-by: Yang Shi <shy828301@gmail.com> --- I do understand the development of cgroup v1 is actually stalled and the community is reluctant to accept new features for v1. However the workingset refault stats are really quite useful and exporting two new stats, which have been supported by v2, seems ok IMHO. So hopefully this patch could be considered. Thanks. mm/memcontrol.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)