diff mbox series

[v1,1/2] mm: memcg: print out cgroup name in the memcg tracepoints

Message ID 20231101102837.25205-2-ddrokosov@salutedevices.com
State New
Headers
Series mm: memcg: improve vmscan tracepoints |

Commit Message

Dmitry Rokosov Nov. 1, 2023, 10:28 a.m. UTC 
  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(-)

Comments

Andrii Nakryiko Nov. 1, 2023, 5:08 p.m. UTC | #1 
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))
>  );

[...]
Dmitry Rokosov Nov. 1, 2023, 6 p.m. UTC | #2 
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 mbox series

Patch

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;