Re: [PATCH v2 1/2] mm: memcg: print out cgroup name in the memcg tracepoints

[Dmitry Rokosov]

Hello Shakeel,

Thank you for quick review!
Please find my thoughts below.

On Thu, Nov 23, 2023 at 07:21:26AM +0000, Shakeel Butt wrote:
> On Wed, Nov 22, 2023 at 01:01:55PM +0300, Dmitry Rokosov 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@xxxxxxxxxxxxxxxxx>
> > ---
> >  include/trace/events/vmscan.h | 77 +++++++++++++++++++++++++++++------
> >  mm/vmscan.c                   | 10 ++---
> >  2 files changed, 70 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > index d2123dd960d5..9b49cd120ae9 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(int order, gfp_t gfp_flags, const struct mem_cgroup *memcg),
> > +
> > +	TP_ARGS(order, gfp_flags, memcg),
> > +
> > +	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));
> 
> Any reason not to use cgroup_ino? cgroup_name may conflict and be
> ambiguous.

I actually didn't consider it, as the cgroup name serves as a clear tag
for filtering the appropriate cgroup in the entire trace file. However,
you are correct that there might be conflicts with cgroup names.
Therefore, it might be better to display both tags: ino and name. What
do you think on this?

-- 
Thank you,
Dmitry