[BUG] perf kmem: Broken because of missing tracepoints

[BUG] perf kmem: Broken because of missing tracepoints

[Ravi Bangoria]

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?

Thanks,
Ravi

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

[Arnaldo Carvalho de Melo]

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.

Thanks,

- Arnaldo

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

[Leo Yan]

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")
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/builtin-kmem.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index e20656c431a4..c97ec38db521 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -26,6 +26,7 @@
 #include "util/string2.h"
 
 #include <linux/kernel.h>
+#include <linux/numa.h>
 #include <linux/rbtree.h>
 #include <linux/string.h>
 #include <linux/zalloc.h>
@@ -176,6 +177,7 @@ static int evsel__process_alloc_event(struct evsel *evsel, struct perf_sample *s
 		      call_site = evsel__intval(evsel, sample, "call_site");
 	int bytes_req = evsel__intval(evsel, sample, "bytes_req"),
 	    bytes_alloc = evsel__intval(evsel, sample, "bytes_alloc");
+	int node1, node2;
 
 	if (insert_alloc_stat(call_site, ptr, bytes_req, bytes_alloc, sample->cpu) ||
 	    insert_caller_stat(call_site, bytes_req, bytes_alloc))
@@ -185,22 +187,18 @@ static int evsel__process_alloc_event(struct evsel *evsel, struct perf_sample *s
 	total_allocated += bytes_alloc;
 
 	nr_allocs++;
-	return 0;
-}
 
-static int evsel__process_alloc_node_event(struct evsel *evsel, struct perf_sample *sample)
-{
-	int ret = evsel__process_alloc_event(evsel, sample);
+	node1 = cpu__get_node((struct perf_cpu){.cpu = sample->cpu});
+	node2 = evsel__intval(evsel, sample, "node");
 
-	if (!ret) {
-		int node1 = cpu__get_node((struct perf_cpu){.cpu = sample->cpu}),
-		    node2 = evsel__intval(evsel, sample, "node");
-
-		if (node1 != node2)
-			nr_cross_allocs++;
-	}
+	/*
+	 * If the field "node" is NUMA_NO_NODE (-1), we don't take it
+	 * as a cross allocation.
+	 */
+	if ((node2 != NUMA_NO_NODE) && (node1 != node2))
+		nr_cross_allocs++;
 
-	return ret;
+	return 0;
 }
 
 static int ptr_cmp(void *, void *);
@@ -1369,8 +1367,6 @@ static int __cmd_kmem(struct perf_session *session)
 		/* slab allocator */
 		{ "kmem:kmalloc",		evsel__process_alloc_event, },
 		{ "kmem:kmem_cache_alloc",	evsel__process_alloc_event, },
-		{ "kmem:kmalloc_node",		evsel__process_alloc_node_event, },
-		{ "kmem:kmem_cache_alloc_node", evsel__process_alloc_node_event, },
 		{ "kmem:kfree",			evsel__process_free_event, },
 		{ "kmem:kmem_cache_free",	evsel__process_free_event, },
 		/* page allocator */
@@ -1831,10 +1827,8 @@ static int __cmd_record(int argc, const char **argv)
 	};
 	const char * const slab_events[] = {
 	"-e", "kmem:kmalloc",
-	"-e", "kmem:kmalloc_node",
 	"-e", "kmem:kfree",
 	"-e", "kmem:kmem_cache_alloc",
-	"-e", "kmem:kmem_cache_alloc_node",
 	"-e", "kmem:kmem_cache_free",
 	};
 	const char * const page_events[] = {
-- 
2.34.1

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

[Ravi Bangoria]

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")
> 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,
Ravi

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

[Arnaldo Carvalho de Melo]

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")
> > 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.

- Arnaldo

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

[Arnaldo Carvalho de Melo]

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

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

[Leo Yan]

On Fri, Jan 06, 2023 at 02:44:51PM -0300, Arnaldo Carvalho de Melo wrote:

[...]

> > > > 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.

Okay, it's fine for me to remove the term "ABI" in commit log.

But I am confused for removing Fixes tag, this leads us to have no
chance to backport this patch to any stable kernel branches, right?

> ---
> 
> 
> 
> > > > 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?

Sure, will respin the patch.

Thanks a lot for the suggestion and Ravi's testing.

Leo

> 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

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

[Vlastimil Babka]

On 1/7/23 05:13, Leo Yan wrote:
> On Fri, Jan 06, 2023 at 02:44:51PM -0300, Arnaldo Carvalho de Melo wrote:
> 
> [...]
> 
>>>>> 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.
> 
> Okay, it's fine for me to remove the term "ABI" in commit log.
> 
> But I am confused for removing Fixes tag, this leads us to have no
> chance to backport this patch to any stable kernel branches, right?

Agree that leaving Fixes: would be appropriate to help anyone
backporting the original patch to notice they should consider this fix
too (although there's some disconnect between kernel and perf), it can't
hurt.

>> ---
>>
>>
>>
>>>>> 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?
> 
> Sure, will respin the patch.
> 
> Thanks a lot for the suggestion and Ravi's testing.

Thanks everyone for fixing the fallout. I did expect some 3rd party
tools would need update to work with the slab tracepoints change, but
didn't realize it would be perf itself as well.

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

[Leo Yan]

On Sat, Jan 07, 2023 at 07:41:25PM +0100, Vlastimil Babka wrote:

[...]

> > But I am confused for removing Fixes tag, this leads us to have no
> > chance to backport this patch to any stable kernel branches, right?
> 
> Agree that leaving Fixes: would be appropriate to help anyone
> backporting the original patch to notice they should consider this fix
> too (although there's some disconnect between kernel and perf), it can't
> hurt.

Thanks for the confirmation, Vlastimil.

> Thanks everyone for fixing the fallout. I did expect some 3rd party
> tools would need update to work with the slab tracepoints change, but
> didn't realize it would be perf itself as well.

No worries, have sent a patch set to fix perf:
https://lore.kernel.org/lkml/20230108062400.250690-1-leo.yan@linaro.org/

Thanks,
Leo