Message ID | 20231101102837.25205-2-ddrokosov@salutedevices.com (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
Series | mm: memcg: improve vmscan tracepoints | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Not a local patch |
On Wed, Nov 1, 2023 at 3:29 AM Dmitry Rokosov <ddrokosov@salutedevices.com> wrote: > > Sometimes it is necessary to understand in which memcg tracepoint event > occurred. The function cgroup_name() is a useful tool for this purpose. > To integrate cgroup_name() into the existing memcg tracepoints, this > patch introduces a new tracepoint template for the begin() and end() > events, utilizing static __array() to store the cgroup name. > > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com> > --- > include/trace/events/vmscan.h | 77 +++++++++++++++++++++++++++++------ > mm/vmscan.c | 8 ++-- > 2 files changed, 69 insertions(+), 16 deletions(-) > > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h > index d2123dd960d5..124bc22866c8 100644 > --- a/include/trace/events/vmscan.h > +++ b/include/trace/events/vmscan.h > @@ -141,19 +141,47 @@ DEFINE_EVENT(mm_vmscan_direct_reclaim_begin_template, mm_vmscan_direct_reclaim_b > ); > > #ifdef CONFIG_MEMCG > -DEFINE_EVENT(mm_vmscan_direct_reclaim_begin_template, mm_vmscan_memcg_reclaim_begin, > > - TP_PROTO(int order, gfp_t gfp_flags), > +DECLARE_EVENT_CLASS(mm_vmscan_memcg_reclaim_begin_template, > > - TP_ARGS(order, gfp_flags) > + TP_PROTO(const struct mem_cgroup *memcg, int order, gfp_t gfp_flags), > + > + TP_ARGS(memcg, order, gfp_flags), By adding memcg in front of existing tracepoint arguments, you unnecessarily break everyone who currently has some scripts based on this tracepoint. Given there is no reason why memcg has to be the very first argument, it would be nice if you can just append it at the end to make it nicely backwards compatible. Same for other tracepoints below. Tracepoints are not an ABI, but there is also no point in arbitrarily breaking all current scripts for such a trivial reason. > + > + TP_STRUCT__entry( > + __field(int, order) > + __field(unsigned long, gfp_flags) > + __array(char, name, NAME_MAX + 1) > + ), > + > + TP_fast_assign( > + __entry->order = order; > + __entry->gfp_flags = (__force unsigned long)gfp_flags; > + cgroup_name(memcg->css.cgroup, > + __entry->name, > + sizeof(__entry->name)); > + ), > + > + TP_printk("memcg=%s order=%d gfp_flags=%s", > + __entry->name, > + __entry->order, > + show_gfp_flags(__entry->gfp_flags)) > ); [...]
Hello Andrii! Thank you for quick feedback! On Wed, Nov 01, 2023 at 10:08:34AM -0700, Andrii Nakryiko wrote: > On Wed, Nov 1, 2023 at 3:29 AM Dmitry Rokosov > <ddrokosov@salutedevices.com> wrote: > > > > Sometimes it is necessary to understand in which memcg tracepoint event > > occurred. The function cgroup_name() is a useful tool for this purpose. > > To integrate cgroup_name() into the existing memcg tracepoints, this > > patch introduces a new tracepoint template for the begin() and end() > > events, utilizing static __array() to store the cgroup name. > > > > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com> > > --- > > include/trace/events/vmscan.h | 77 +++++++++++++++++++++++++++++------ > > mm/vmscan.c | 8 ++-- > > 2 files changed, 69 insertions(+), 16 deletions(-) > > > > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h > > index d2123dd960d5..124bc22866c8 100644 > > --- a/include/trace/events/vmscan.h > > +++ b/include/trace/events/vmscan.h > > @@ -141,19 +141,47 @@ DEFINE_EVENT(mm_vmscan_direct_reclaim_begin_template, mm_vmscan_direct_reclaim_b > > ); > > > > #ifdef CONFIG_MEMCG > > -DEFINE_EVENT(mm_vmscan_direct_reclaim_begin_template, mm_vmscan_memcg_reclaim_begin, > > > > - TP_PROTO(int order, gfp_t gfp_flags), > > +DECLARE_EVENT_CLASS(mm_vmscan_memcg_reclaim_begin_template, > > > > - TP_ARGS(order, gfp_flags) > > + TP_PROTO(const struct mem_cgroup *memcg, int order, gfp_t gfp_flags), > > + > > + TP_ARGS(memcg, order, gfp_flags), > > By adding memcg in front of existing tracepoint arguments, you > unnecessarily break everyone who currently has some scripts based on > this tracepoint. Given there is no reason why memcg has to be the very > first argument, it would be nice if you can just append it at the end > to make it nicely backwards compatible. Same for other tracepoints > below. > > Tracepoints are not an ABI, but there is also no point in arbitrarily > breaking all current scripts for such a trivial reason. > You are absolutely correct. I didn't consider the scripts that rely on these tracepoints, because tracepoints are not an ABI, as you mentioned. Additionally, I added the memcg parameter as the first argument based on my personal programming patterns, where the context object should always come first :) I apologize for that and will prepare new version. > > + > > + TP_STRUCT__entry( > > + __field(int, order) > > + __field(unsigned long, gfp_flags) > > + __array(char, name, NAME_MAX + 1) > > + ), > > + > > + TP_fast_assign( > > + __entry->order = order; > > + __entry->gfp_flags = (__force unsigned long)gfp_flags; > > + cgroup_name(memcg->css.cgroup, > > + __entry->name, > > + sizeof(__entry->name)); > > + ), > > + > > + TP_printk("memcg=%s order=%d gfp_flags=%s", > > + __entry->name, > > + __entry->order, > > + show_gfp_flags(__entry->gfp_flags)) > > ); > > [...]
diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h index d2123dd960d5..124bc22866c8 100644 --- a/include/trace/events/vmscan.h +++ b/include/trace/events/vmscan.h @@ -141,19 +141,47 @@ DEFINE_EVENT(mm_vmscan_direct_reclaim_begin_template, mm_vmscan_direct_reclaim_b ); #ifdef CONFIG_MEMCG -DEFINE_EVENT(mm_vmscan_direct_reclaim_begin_template, mm_vmscan_memcg_reclaim_begin, - TP_PROTO(int order, gfp_t gfp_flags), +DECLARE_EVENT_CLASS(mm_vmscan_memcg_reclaim_begin_template, - TP_ARGS(order, gfp_flags) + TP_PROTO(const struct mem_cgroup *memcg, int order, gfp_t gfp_flags), + + TP_ARGS(memcg, order, gfp_flags), + + TP_STRUCT__entry( + __field(int, order) + __field(unsigned long, gfp_flags) + __array(char, name, NAME_MAX + 1) + ), + + TP_fast_assign( + __entry->order = order; + __entry->gfp_flags = (__force unsigned long)gfp_flags; + cgroup_name(memcg->css.cgroup, + __entry->name, + sizeof(__entry->name)); + ), + + TP_printk("memcg=%s order=%d gfp_flags=%s", + __entry->name, + __entry->order, + show_gfp_flags(__entry->gfp_flags)) ); -DEFINE_EVENT(mm_vmscan_direct_reclaim_begin_template, mm_vmscan_memcg_softlimit_reclaim_begin, +DEFINE_EVENT(mm_vmscan_memcg_reclaim_begin_template, mm_vmscan_memcg_reclaim_begin, - TP_PROTO(int order, gfp_t gfp_flags), + TP_PROTO(const struct mem_cgroup *memcg, int order, gfp_t gfp_flags), - TP_ARGS(order, gfp_flags) + TP_ARGS(memcg, order, gfp_flags) +); + +DEFINE_EVENT(mm_vmscan_memcg_reclaim_begin_template, mm_vmscan_memcg_softlimit_reclaim_begin, + + TP_PROTO(const struct mem_cgroup *memcg, int order, gfp_t gfp_flags), + + TP_ARGS(memcg, order, gfp_flags) ); + #endif /* CONFIG_MEMCG */ DECLARE_EVENT_CLASS(mm_vmscan_direct_reclaim_end_template, @@ -181,19 +209,44 @@ DEFINE_EVENT(mm_vmscan_direct_reclaim_end_template, mm_vmscan_direct_reclaim_end ); #ifdef CONFIG_MEMCG -DEFINE_EVENT(mm_vmscan_direct_reclaim_end_template, mm_vmscan_memcg_reclaim_end, - TP_PROTO(unsigned long nr_reclaimed), +DECLARE_EVENT_CLASS(mm_vmscan_memcg_reclaim_end_template, - TP_ARGS(nr_reclaimed) + TP_PROTO(const struct mem_cgroup *memcg, unsigned long nr_reclaimed), + + TP_ARGS(memcg, nr_reclaimed), + + TP_STRUCT__entry( + __field(unsigned long, nr_reclaimed) + __array(char, name, NAME_MAX + 1) + ), + + TP_fast_assign( + __entry->nr_reclaimed = nr_reclaimed; + cgroup_name(memcg->css.cgroup, + __entry->name, + sizeof(__entry->name)); + ), + + TP_printk("memcg=%s nr_reclaimed=%lu", + __entry->name, + __entry->nr_reclaimed) ); -DEFINE_EVENT(mm_vmscan_direct_reclaim_end_template, mm_vmscan_memcg_softlimit_reclaim_end, +DEFINE_EVENT(mm_vmscan_memcg_reclaim_end_template, mm_vmscan_memcg_reclaim_end, - TP_PROTO(unsigned long nr_reclaimed), + TP_PROTO(const struct mem_cgroup *memcg, unsigned long nr_reclaimed), - TP_ARGS(nr_reclaimed) + TP_ARGS(memcg, nr_reclaimed) ); + +DEFINE_EVENT(mm_vmscan_memcg_reclaim_end_template, mm_vmscan_memcg_softlimit_reclaim_end, + + TP_PROTO(const struct mem_cgroup *memcg, unsigned long nr_reclaimed), + + TP_ARGS(memcg, nr_reclaimed) +); + #endif /* CONFIG_MEMCG */ TRACE_EVENT(mm_shrink_slab_start, diff --git a/mm/vmscan.c b/mm/vmscan.c index 1080209a568b..4309eaf188b4 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -7088,7 +7088,7 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg, sc.gfp_mask = (gfp_mask & GFP_RECLAIM_MASK) | (GFP_HIGHUSER_MOVABLE & ~GFP_RECLAIM_MASK); - trace_mm_vmscan_memcg_softlimit_reclaim_begin(sc.order, + trace_mm_vmscan_memcg_softlimit_reclaim_begin(memcg, sc.order, sc.gfp_mask); /* @@ -7100,7 +7100,7 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg, */ shrink_lruvec(lruvec, &sc); - trace_mm_vmscan_memcg_softlimit_reclaim_end(sc.nr_reclaimed); + trace_mm_vmscan_memcg_softlimit_reclaim_end(memcg, sc.nr_reclaimed); *nr_scanned = sc.nr_scanned; @@ -7134,13 +7134,13 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg, struct zonelist *zonelist = node_zonelist(numa_node_id(), sc.gfp_mask); set_task_reclaim_state(current, &sc.reclaim_state); - trace_mm_vmscan_memcg_reclaim_begin(0, sc.gfp_mask); + trace_mm_vmscan_memcg_reclaim_begin(memcg, 0, sc.gfp_mask); noreclaim_flag = memalloc_noreclaim_save(); nr_reclaimed = do_try_to_free_pages(zonelist, &sc); memalloc_noreclaim_restore(noreclaim_flag); - trace_mm_vmscan_memcg_reclaim_end(nr_reclaimed); + trace_mm_vmscan_memcg_reclaim_end(memcg, nr_reclaimed); set_task_reclaim_state(current, NULL); return nr_reclaimed;
Sometimes it is necessary to understand in which memcg tracepoint event occurred. The function cgroup_name() is a useful tool for this purpose. To integrate cgroup_name() into the existing memcg tracepoints, this patch introduces a new tracepoint template for the begin() and end() events, utilizing static __array() to store the cgroup name. Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com> --- include/trace/events/vmscan.h | 77 +++++++++++++++++++++++++++++------ mm/vmscan.c | 8 ++-- 2 files changed, 69 insertions(+), 16 deletions(-)