Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231101102837.25205-1-ddrokosov@salutedevices.com> <20231101102837.25205-2-ddrokosov@salutedevices.com>
In-Reply-To: <20231101102837.25205-2-ddrokosov@salutedevices.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Wed, 1 Nov 2023 10:08:34 -0700
Message-ID: <CAEf4BzZ0p-k15XLf2QdHNN6TodjRBtRKk2mvsttCj=GUi4Or3A@mail.gmail.com>
Subject: Re: [PATCH v1 1/2] mm: memcg: print out cgroup name in the memcg tracepoints
To:     Dmitry Rokosov <ddrokosov@salutedevices.com>
Cc:     rostedt@goodmis.org, mhiramat@kernel.org, hannes@cmpxchg.org,
        mhocko@kernel.org, roman.gushchin@linux.dev, shakeelb@google.com,
        muchun.song@linux.dev, akpm@linux-foundation.org,
        kernel@sberdevices.ru, rockosov@gmail.com, cgroups@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Nov 1, 2023 at 3:29=E2=80=AFAM 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_templat=
e, mm_vmscan_direct_reclaim_b
>  );
>
>  #ifdef CONFIG_MEMCG
> -DEFINE_EVENT(mm_vmscan_direct_reclaim_begin_template, mm_vmscan_memcg_re=
claim_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_fla=
gs),
> +
> +       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 =3D order;
> +               __entry->gfp_flags =3D (__force unsigned long)gfp_flags;
> +               cgroup_name(memcg->css.cgroup,
> +                       __entry->name,
> +                       sizeof(__entry->name));
> +       ),
> +
> +       TP_printk("memcg=3D%s order=3D%d gfp_flags=3D%s",
> +               __entry->name,
> +               __entry->order,
> +               show_gfp_flags(__entry->gfp_flags))
>  );

[...]