Re: [BUG] perf kmem: Broken because of missing tracepoints

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Jan 06, 2023 at 02:27:54PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Jan 06, 2023 at 06:34:32PM +0530, Ravi Bangoria escreveu:
> > On 06-Jan-23 10:52 AM, Leo Yan wrote:
> > > Hi Ravi, Arnaldo,
> > > 
> > > On Thu, Jan 05, 2023 at 01:27:30PM -0300, Arnaldo Carvalho de Melo wrote:
> > >> Em Thu, Jan 05, 2023 at 07:03:25PM +0530, Ravi Bangoria escreveu:
> > >>> Hello,
> > >>>
> > >>> Two tracepoints, kmem:kmalloc_node and kmem:kmem_cache_alloc_node used by perf-
> > >>> kmem tool were removed from kernel by commit 11e9734bcb6a7 ("mm/slab_common:
> > >>> unify NUMA and UMA version of tracepoints"). This causes issue while running
> > >>> perf kmem on latest kernel:
> > >>>
> > >>>   $ sudo ./perf kmem record
> > >>>   event syntax error: 'kmem:kmalloc_node'
> > >>>                        \___ unknown tracepoint
> > >>>
> > >>>   $ sudo ./perf kmem record
> > >>>   event syntax error: 'kmem:kmem_cache_alloc_node'
> > >>>                        \___ unknown tracepoint
> > >>>
> > >>> Haven't got a chance to debug further. Anyone aware of this?
> > >>
> > >> I didn't notice this so far, would be good to have a fix soon.
> > > 
> > > I looked a bit for the issue, so wrote a fix and verified on my
> > > Arm64 machine.  Please let me know if this works for you or not,
> > > thanks!
> > > 
> > > From b7d26f5543401924ae4ad93daeba66cd7d58eb39 Mon Sep 17 00:00:00 2001
> > > From: Leo Yan <leo.yan@linaro.org>
> > > Date: Fri, 6 Jan 2023 11:16:27 +0800
> > > Subject: [PATCH] perf kmem: Fix tracepoints breakage
> > > 
> > > Commit 11e9734bcb6a ("mm/slab_common: unify NUMA and UMA version of
> > > tracepoints") removed tracepoints 'kmalloc_node' and
> > > 'kmem_cache_alloc_node'; the tracepoints 'kmalloc'
> > > and 'kmem_cache_alloc' add an extra field 'node' to present the
> > > memory node to be allocated and replace the two removed tracepoints
> > > respectively.
> > > 
> > > The commit introduces ABI breakage, both for the dropped tracepoints and
> > > for the updated tracepoints.
> > > 
> > > To fix this issue, perf tool removes the support for the tracepoints
> > > 'kmalloc_node' and 'kmem_cache_alloc_node'; for the two updated
> > > tracepoints this patch reads out the new field 'node', if its value is
> > > NUMA_NO_NODE (-1), we doesn't take it as a cross memory allocation.
> > > 
> > > Reported-by: Ravi Bangoria <ravi.bangoria@amd.com>
> > > Fixes: 11e9734bcb6a ("mm/slab_common: unify NUMA and UMA version of tracepoints")

But I'm removing this Fixes: tag (leaving the reference in the 1st
commit log message paragraph tho), and removing the term "ABI", as AFAIK
tracepoints can change and tools have to adapt, right?

Lemme see...

Documentation/bpf/bpf_design_QA.rst

Q: Are tracepoints part of the stable ABI?
------------------------------------------
A: NO. Tracepoints are tied to internal implementation details hence they are
subject to change and can break with newer kernels. BPF programs need to change
accordingly when this happens.

---



> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > 
> > Thanks Leo. A quick test shows it fixes the issue.
> > 
> > Tested-by: Ravi Bangoria <ravi.bangoria@amd.com>
> 
> Thanks, applied.

And I retract that, we need to query for the existence of these
tracepoints and use it if available, as we expect a new perf tool to
work on an older kernel, one where the removed tracepoints may be
available.

Leo, can you try to rework this?

If you look at tools/perf/builtin-sched.c it queries for the existence
of:

static bool schedstat_events_exposed(void)
{
        /*
         * Select "sched:sched_stat_wait" event to check
         * whether schedstat tracepoints are exposed.
         */
        return IS_ERR(trace_event__tp_format("sched", "sched_stat_wait")) ?
                false : true;
}

        /*
         * The tracepoints trace_sched_stat_{wait, sleep, iowait}
         * are not exposed to user if CONFIG_SCHEDSTATS is not set,
         * to prevent "perf sched record" execution failure, determine
         * whether to record schedstat events according to actual situation.
         */
        const char * const schedstat_args[] = {
                "-e", "sched:sched_stat_wait",
                "-e", "sched:sched_stat_sleep",
                "-e", "sched:sched_stat_iowait",
        };
        unsigned int schedstat_argc = schedstat_events_exposed() ?
                ARRAY_SIZE(schedstat_args) : 0;

So its something along those lines.

Thanks,

- Arnaldo