linux-kernel - Re: [PATCH v1 1/2] mm: memcg: print out cgroup name in the memcg tracepoints

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzZ0p-k15XLf2QdHNN6TodjRBtRKk2mvsttCj=GUi4Or3A@mail.gmail.com>
Date:   Wed, 1 Nov 2023 10:08:34 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Dmitry Rokosov <ddrokosov@...utedevices.com>
Cc:     rostedt@...dmis.org, mhiramat@...nel.org, hannes@...xchg.org,
        mhocko@...nel.org, roman.gushchin@...ux.dev, shakeelb@...gle.com,
        muchun.song@...ux.dev, akpm@...ux-foundation.org,
        kernel@...rdevices.ru, rockosov@...il.com, cgroups@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH v1 1/2] mm: memcg: print out cgroup name in the memcg tracepoints

On Wed, Nov 1, 2023 at 3:29 AM Dmitry Rokosov
<ddrokosov@...utedevices.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@...utedevices.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))
>  );

[...]