[1/2] perf kmem: Support legacy tracepoints

  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(-)
  

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
  

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
  

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
  

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