Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <14dd06c2-39f8-ccb5-ce59-f3a1e45c94d0@arm.com>
Date:   Mon, 9 Jan 2023 15:11:47 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH 1/2] perf kmem: Support legacy tracepoints
Content-Language: en-US
To:     Leo Yan <leo.yan@linaro.org>
References: <20230108062400.250690-1-leo.yan@linaro.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Ravi Bangoria <ravi.bangoria@amd.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
From:   James Clark <james.clark@arm.com>
In-Reply-To: <20230108062400.250690-1-leo.yan@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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