[PATCH 1/2] perf kmem: Support legacy tracepoints

[PATCH 1/2] perf kmem: Support legacy tracepoints

[Leo Yan]

Commit 11e9734bcb6a ("mm/slab_common: unify NUMA and UMA version of
tracepoints") removed tracepoints 'kmalloc_node' and
'kmem_cache_alloc_node', these two tracepoints have disappeared in the
latest kernel, but we also need to consider the tool should be backward
compatible with old kernels.

If detects the tracepoint "kmem:kmalloc_node" is existed on a system,
this patch enables the legacy tracepoints, otherwise, it will ignore
them for the new kernels.

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 | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index e20656c431a4..50a3df5dc18a 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -1824,6 +1824,19 @@ static int parse_line_opt(const struct option *opt __maybe_unused,
 	return 0;
 }
 
+static bool slab_legacy_tp_is_exposed(void)
+{
+	/*
+	 * The tracepoints "kmem:kmalloc_node" and
+	 * "kmem:kmem_cache_alloc_node" have been removed on the latest
+	 * kernel, if the tracepoint "kmem:kmalloc_node" is existed it
+	 * means the tool is running on an old kernel, we need to
+	 * rollback to support these legacy tracepoints.
+	 */
+	return IS_ERR(trace_event__tp_format("kmem", "kmalloc_node")) ?
+		false : true;
+}
+
 static int __cmd_record(int argc, const char **argv)
 {
 	const char * const record_args[] = {
@@ -1831,22 +1844,28 @@ 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 slab_legacy_events[] = {
+	"-e", "kmem:kmalloc_node",
+	"-e", "kmem:kmem_cache_alloc_node",
+	};
 	const char * const page_events[] = {
 	"-e", "kmem:mm_page_alloc",
 	"-e", "kmem:mm_page_free",
 	};
 	unsigned int rec_argc, i, j;
 	const char **rec_argv;
+	unsigned int slab_legacy_tp_exposed = slab_legacy_tp_is_exposed();
 
 	rec_argc = ARRAY_SIZE(record_args) + argc - 1;
-	if (kmem_slab)
+	if (kmem_slab) {
 		rec_argc += ARRAY_SIZE(slab_events);
+		if (slab_legacy_tp_exposed)
+			rec_argc += ARRAY_SIZE(slab_legacy_events);
+	}
 	if (kmem_page)
 		rec_argc += ARRAY_SIZE(page_events) + 1; /* for -g */
 
@@ -1861,6 +1880,10 @@ static int __cmd_record(int argc, const char **argv)
 	if (kmem_slab) {
 		for (j = 0; j < ARRAY_SIZE(slab_events); j++, i++)
 			rec_argv[i] = strdup(slab_events[j]);
+		if (slab_legacy_tp_exposed) {
+			for (j = 0; j < ARRAY_SIZE(slab_legacy_events); j++, i++)
+				rec_argv[i] = strdup(slab_legacy_events[j]);
+		}
 	}
 	if (kmem_page) {
 		rec_argv[i++] = strdup("-g");
-- 
2.34.1

[PATCH 2/2] perf kmem: Support field "node" in evsel__process_alloc_event()

[Leo Yan]

Commit 11e9734bcb6a ("mm/slab_common: unify NUMA and UMA version of
tracepoints") adds the field "node" into the tracepoints 'kmalloc' and
'kmem_cache_alloc', so this patch modifies the event process function to
support the field "node".

If field "node" is existed by checking function evsel__field(), it stats
the cross allocation.

When the "node" value is NUMA_NO_NODE (-1), it means the memory can be
allocated from any memory node, in this case, we don't account it as a
cross allocation.

After support the field "node" in evsel__process_alloc_event(),
evsel__process_alloc_node_event() is duplicate with the previous one,
so removes evsel__process_alloc_node_event().

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 | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index 50a3df5dc18a..8ae0a1535293 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>
@@ -185,22 +186,33 @@ 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);
+	/*
+	 * Commit 11e9734bcb6a ("mm/slab_common: unify NUMA and UMA
+	 * version of tracepoints") adds the field "node" into the
+	 * tracepoints 'kmalloc' and 'kmem_cache_alloc'.
+	 *
+	 * The legacy tracepoints 'kmalloc_node' and 'kmem_cache_alloc_node'
+	 * also contain the field "node".
+	 *
+	 * If the tracepoint contains the field "node" the tool stats the
+	 * cross allocation.
+	 */
+	if (evsel__field(evsel, "node")) {
+		int node1, node2;
 
-	if (!ret) {
-		int node1 = cpu__get_node((struct perf_cpu){.cpu = sample->cpu}),
-		    node2 = evsel__intval(evsel, sample, "node");
+		node1 = cpu__get_node((struct perf_cpu){.cpu = sample->cpu});
+		node2 = evsel__intval(evsel, sample, "node");
 
-		if (node1 != node2)
+		/*
+		 * 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 +1381,8 @@ 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:kmalloc_node",		evsel__process_alloc_event, },
+		{ "kmem:kmem_cache_alloc_node", evsel__process_alloc_event, },
 		{ "kmem:kfree",			evsel__process_free_event, },
 		{ "kmem:kmem_cache_free",	evsel__process_free_event, },
 		/* page allocator */
-- 
2.34.1

Re: [PATCH 2/2] perf kmem: Support field "node" in evsel__process_alloc_event()

[James Clark]

On 08/01/2023 06:24, Leo Yan wrote:
> Commit 11e9734bcb6a ("mm/slab_common: unify NUMA and UMA version of
> tracepoints") adds the field "node" into the tracepoints 'kmalloc' and
> 'kmem_cache_alloc', so this patch modifies the event process function to
> support the field "node".
> 
> If field "node" is existed by checking function evsel__field(), it stats
> the cross allocation.
> 
> When the "node" value is NUMA_NO_NODE (-1), it means the memory can be
> allocated from any memory node, in this case, we don't account it as a
> cross allocation.
> 
> After support the field "node" in evsel__process_alloc_event(),
> evsel__process_alloc_node_event() is duplicate with the previous one,
> so removes evsel__process_alloc_node_event().
> 
> 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 | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 

Reviewed-by: James Clark <james.clark@arm.com>

Re: [PATCH 1/2] perf kmem: Support legacy tracepoints

[James Clark]

On 08/01/2023 06:23, Leo Yan wrote:
> Commit 11e9734bcb6a ("mm/slab_common: unify NUMA and UMA version of
> tracepoints") removed tracepoints 'kmalloc_node' and
> 'kmem_cache_alloc_node', these two tracepoints have disappeared in the
> latest kernel, but we also need to consider the tool should be backward
> compatible with old kernels.
> 
> If detects the tracepoint "kmem:kmalloc_node" is existed on a system,
> this patch enables the legacy tracepoints, otherwise, it will ignore
> them for the new kernels.
> 
> 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 | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> index e20656c431a4..50a3df5dc18a 100644
> --- a/tools/perf/builtin-kmem.c
> +++ b/tools/perf/builtin-kmem.c
> @@ -1824,6 +1824,19 @@ static int parse_line_opt(const struct option *opt __maybe_unused,
>  	return 0;
>  }
>  
> +static bool slab_legacy_tp_is_exposed(void)
> +{
> +	/*
> +	 * The tracepoints "kmem:kmalloc_node" and
> +	 * "kmem:kmem_cache_alloc_node" have been removed on the latest
> +	 * kernel, if the tracepoint "kmem:kmalloc_node" is existed it
> +	 * means the tool is running on an old kernel, we need to
> +	 * rollback to support these legacy tracepoints.
> +	 */
> +	return IS_ERR(trace_event__tp_format("kmem", "kmalloc_node")) ?
> +		false : true;
> +}
> +
>  static int __cmd_record(int argc, const char **argv)
>  {
>  	const char * const record_args[] = {
> @@ -1831,22 +1844,28 @@ 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 slab_legacy_events[] = {
> +	"-e", "kmem:kmalloc_node",
> +	"-e", "kmem:kmem_cache_alloc_node",
> +	};

Reviewed-by: James Clark <james.clark@arm.com>

This fixes the error with mem:kmalloc_node for me.

I was thinking that it might be best to add all events to the list
conditionally instead of just the legacy ones. That way, the same error
won't happen in the future. But maybe it's best to have an explicit
error again in case the breaking change was unintentional so it's fine
as it is I think.

James

Re: [PATCH 1/2] perf kmem: Support legacy tracepoints

[Arnaldo Carvalho de Melo]

Em Mon, Jan 09, 2023 at 03:11:47PM +0000, James Clark escreveu:
> 
> 
> On 08/01/2023 06:23, Leo Yan wrote:
> > Commit 11e9734bcb6a ("mm/slab_common: unify NUMA and UMA version of
> > tracepoints") removed tracepoints 'kmalloc_node' and
> > 'kmem_cache_alloc_node', these two tracepoints have disappeared in the
> > latest kernel, but we also need to consider the tool should be backward
> > compatible with old kernels.
> > 
> > If detects the tracepoint "kmem:kmalloc_node" is existed on a system,
> > this patch enables the legacy tracepoints, otherwise, it will ignore
> > them for the new kernels.
> > 
> > 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 | 29 ++++++++++++++++++++++++++---
> >  1 file changed, 26 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> > index e20656c431a4..50a3df5dc18a 100644
> > --- a/tools/perf/builtin-kmem.c
> > +++ b/tools/perf/builtin-kmem.c
> > @@ -1824,6 +1824,19 @@ static int parse_line_opt(const struct option *opt __maybe_unused,
> >  	return 0;
> >  }
> >  
> > +static bool slab_legacy_tp_is_exposed(void)
> > +{
> > +	/*
> > +	 * The tracepoints "kmem:kmalloc_node" and
> > +	 * "kmem:kmem_cache_alloc_node" have been removed on the latest
> > +	 * kernel, if the tracepoint "kmem:kmalloc_node" is existed it
> > +	 * means the tool is running on an old kernel, we need to
> > +	 * rollback to support these legacy tracepoints.
> > +	 */
> > +	return IS_ERR(trace_event__tp_format("kmem", "kmalloc_node")) ?
> > +		false : true;
> > +}
> > +
> >  static int __cmd_record(int argc, const char **argv)
> >  {
> >  	const char * const record_args[] = {
> > @@ -1831,22 +1844,28 @@ 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 slab_legacy_events[] = {
> > +	"-e", "kmem:kmalloc_node",
> > +	"-e", "kmem:kmem_cache_alloc_node",
> > +	};
> 
> Reviewed-by: James Clark <james.clark@arm.com>
> 
> This fixes the error with mem:kmalloc_node for me.
> 
> I was thinking that it might be best to add all events to the list
> conditionally instead of just the legacy ones. That way, the same error
> won't happen in the future. But maybe it's best to have an explicit
> error again in case the breaking change was unintentional so it's fine
> as it is I think.

Just applied this, the changes you brains stormed may come as later
patches, thanks,

- Arnaldo

Re: [PATCH 1/2] perf kmem: Support legacy tracepoints

[Leo Yan]

On Mon, Jan 09, 2023 at 12:38:04PM -0300, Arnaldo Carvalho de Melo wrote:

[...]

> > > +	const char * const slab_legacy_events[] = {
> > > +	"-e", "kmem:kmalloc_node",
> > > +	"-e", "kmem:kmem_cache_alloc_node",
> > > +	};
> > 
> > Reviewed-by: James Clark <james.clark@arm.com>
> > 
> > This fixes the error with mem:kmalloc_node for me.

Thanks for reviewing and testing!

> > I was thinking that it might be best to add all events to the list
> > conditionally instead of just the legacy ones. That way, the same error
> > won't happen in the future. But maybe it's best to have an explicit
> > error again in case the breaking change was unintentional so it's fine
> > as it is I think.

Yeah, this is a good idea for refactoring.

James, do you mind to send patches for this?

> Just applied this, the changes you brains stormed may come as later
> patches, thanks,

Thanks, Arnaldo.

Leo

Re: [PATCH 1/2] perf kmem: Support legacy tracepoints

[James Clark]

On 10/01/2023 01:45, Leo Yan wrote:
> On Mon, Jan 09, 2023 at 12:38:04PM -0300, Arnaldo Carvalho de Melo wrote:
> 
> [...]
> 
>>>> +	const char * const slab_legacy_events[] = {
>>>> +	"-e", "kmem:kmalloc_node",
>>>> +	"-e", "kmem:kmem_cache_alloc_node",
>>>> +	};
>>>
>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>
>>> This fixes the error with mem:kmalloc_node for me.
> 
> Thanks for reviewing and testing!
> 
>>> I was thinking that it might be best to add all events to the list
>>> conditionally instead of just the legacy ones. That way, the same error
>>> won't happen in the future. But maybe it's best to have an explicit
>>> error again in case the breaking change was unintentional so it's fine
>>> as it is I think.
> 
> Yeah, this is a good idea for refactoring.
> 
> James, do you mind to send patches for this?

Do you not think there is any value in keeping it as showing an error
for the next time one is removed? I was assuming that was your intention
with this change, and I'm ok with keeping it that way for now. It's
probably quite rare anyway so the refactor could be more effort than the
gain.

James


> 
>> Just applied this, the changes you brains stormed may come as later
>> patches, thanks,
> 
> Thanks, Arnaldo.
> 
> Leo