Re: [PATCH V5 1/6] tools/perf: Move powerpc VPA-DTL auxtrace init into a separate file

[Adrian Hunter <adrian.hunter@intel.com>]

On 07/08/2026 17:41, Athira Rajeev wrote:
> The powerpc auxtrace dispatch lives entirely in
> arch/powerpc/util/auxtrace.c.  As new PMUs such as HTM are added, this
> file would grow to contain the recording logic for all of them.
> 
> Factor out the VPA-DTL recording initializer into its own file,
> arch/powerpc/util/vpa-dtl.c, and reduce auxtrace.c to a thin dispatch
> layer.  auxtrace_record__init() now detects the PMU by name and calls
> the appropriate per-PMU init function:
> 
>   - vpa_dtl_recording_init() for VPA-DTL events (unchanged behaviour)
>   - Further PMU entries will follow in subsequent patches
> 
> This makes room in auxtrace_record__init() for the HTM recording path
> added in the next patch without growing a single monolithic file.
> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>

A couple of cosmetic / very minor comments.

Nevertheless:

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> Changes in V5:
> - Add forward declarations for struct evsel and struct auxtrace_record
>   in powerpc-vpadtl.h before the vpa_dtl_recording_init() prototype.
>   Without them, a translation unit that includes the header before the
>   full definitions are visible may produce implicit-declaration warnings
>   on strict compilers.
> 
> Changes in V4:
> - No changes from V3.
> 
> Changes in V3:
> Add #include <linux/zalloc.h> to vpa-dtl.c; without it the compiler
> treats zalloc() as implicitly returning int, truncating the upper
> 32 bits of the returned pointer on 64-bit PowerPC.
> 
> Changes in V2:
> - Renamed the destination file from arch/powerpc/util/vpa-dtl.c (same
>   name, unchanged) but the subject and commit message are reworded to
>   clearly state that the goal is to make auxtrace_record__init() a thin
>   per-PMU dispatcher, not merely to "allow multiple PMUs to use auxtrace".
> - Handle failure from memory allocation
> - Included stdlib and limits.h
> - No functional change to the VPA-DTL path itself.
> - Patch is now 1/6 instead of 1/9.
> 
>  tools/perf/arch/powerpc/util/Build      |  1 +
>  tools/perf/arch/powerpc/util/auxtrace.c | 84 +++-------------------
>  tools/perf/arch/powerpc/util/vpa-dtl.c  | 96 +++++++++++++++++++++++++
>  tools/perf/util/powerpc-vpadtl.h        |  3 +
>  4 files changed, 108 insertions(+), 76 deletions(-)
>  create mode 100644 tools/perf/arch/powerpc/util/vpa-dtl.c
> 
> diff --git a/tools/perf/arch/powerpc/util/Build b/tools/perf/arch/powerpc/util/Build
> index ae928050e07a..7819c8f5af2d 100644
> --- a/tools/perf/arch/powerpc/util/Build
> +++ b/tools/perf/arch/powerpc/util/Build
> @@ -7,3 +7,4 @@ perf-util-y += evsel.o
>  perf-util-$(CONFIG_LIBDW) += skip-callchain-idx.o
>  
>  perf-util-y += auxtrace.o
> +perf-util-y += vpa-dtl.o
> diff --git a/tools/perf/arch/powerpc/util/auxtrace.c b/tools/perf/arch/powerpc/util/auxtrace.c
> index 4600a1661b4f..e04a0bd61755 100644
> --- a/tools/perf/arch/powerpc/util/auxtrace.c
> +++ b/tools/perf/arch/powerpc/util/auxtrace.c
> @@ -13,63 +13,12 @@
>  #include "../../util/auxtrace.h"
>  #include "../../util/powerpc-vpadtl.h"
>  #include "../../util/record.h"
> -#include <internal/lib.h> // page_size
> -
> -#define KiB(x) ((x) * 1024)
> -
> -static int
> -powerpc_vpadtl_recording_options(struct auxtrace_record *ar __maybe_unused,
> -			struct evlist *evlist __maybe_unused,
> -			struct record_opts *opts)
> -{
> -	opts->full_auxtrace = true;
> -
> -	/*
> -	 * Set auxtrace_mmap_pages to minimum
> -	 * two pages
> -	 */
> -	if (!opts->auxtrace_mmap_pages) {
> -		opts->auxtrace_mmap_pages = KiB(128) / page_size;
> -		if (opts->mmap_pages == UINT_MAX)
> -			opts->mmap_pages = KiB(256) / page_size;
> -	}
> -
> -	return 0;
> -}
> -
> -static size_t powerpc_vpadtl_info_priv_size(struct auxtrace_record *itr __maybe_unused,
> -					struct evlist *evlist __maybe_unused)
> -{
> -	return VPADTL_AUXTRACE_PRIV_SIZE;
> -}
> -
> -static int
> -powerpc_vpadtl_info_fill(struct auxtrace_record *itr __maybe_unused,
> -		struct perf_session *session __maybe_unused,
> -		struct perf_record_auxtrace_info *auxtrace_info,
> -		size_t priv_size __maybe_unused)
> -{
> -	auxtrace_info->type = PERF_AUXTRACE_VPA_DTL;
> -
> -	return 0;
> -}
> -
> -static void powerpc_vpadtl_free(struct auxtrace_record *itr)
> -{
> -	free(itr);
> -}
> -
> -static u64 powerpc_vpadtl_reference(struct auxtrace_record *itr __maybe_unused)
> -{
> -	return 0;
> -}
>  
>  struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
>  						int *err)
>  {
> -	struct auxtrace_record *aux;
>  	struct evsel *pos;
> -	int found = 0;
> +	struct evsel *vpa_dtl_evsel = NULL;

Ordering local definitions by descending line length is nicer e.g.

	struct evsel *vpa_dtl_evsel = NULL;
  	struct evsel *pos;

>  
>  	/*
>  	 * Set err value to zero here. Any fail later
> @@ -78,33 +27,16 @@ struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
>  	*err = 0;
>  
>  	evlist__for_each_entry(evlist, pos) {
> -		if (strstarts(pos->name, "vpa_dtl")) {
> -			found = 1;
> +		if (pos->name && strstarts(pos->name, "vpa_dtl")) {
>  			pos->needs_auxtrace_mmap = true;
> -			break;
> +			/* Remember the first matching VPA DTL event */
> +			if (!vpa_dtl_evsel)
> +				vpa_dtl_evsel = pos;
>  		}
>  	}
>  
> -	if (!found)
> -		return NULL;
> -
> -	/*
> -	 * To obtain the auxtrace buffer file descriptor, the auxtrace event
> -	 * must come first.
> -	 */
> -	evlist__to_front(pos->evlist, pos);
> -
> -	aux = zalloc(sizeof(*aux));
> -	if (aux == NULL) {
> -		pr_debug("aux record is NULL\n");
> -		*err = -ENOMEM;
> -		return NULL;
> -	}
> +	if (vpa_dtl_evsel)
> +		return vpa_dtl_recording_init(vpa_dtl_evsel, err);
>  
> -	aux->recording_options = powerpc_vpadtl_recording_options;
> -	aux->info_priv_size = powerpc_vpadtl_info_priv_size;
> -	aux->info_fill = powerpc_vpadtl_info_fill;
> -	aux->free = powerpc_vpadtl_free;
> -	aux->reference = powerpc_vpadtl_reference;
> -	return aux;
> +	return NULL;
>  }
> diff --git a/tools/perf/arch/powerpc/util/vpa-dtl.c b/tools/perf/arch/powerpc/util/vpa-dtl.c
> new file mode 100644
> index 000000000000..2609b88f61d8
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/vpa-dtl.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VPA DTL AUX tracing support
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/string.h>
> +#include <linux/zalloc.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include "../../util/cpumap.h"

Is cpumap.h needed?

> +#include "../../util/evsel.h"
> +#include "../../util/evlist.h"
> +#include "../../util/session.h"
> +#include "../../util/util.h"
> +#include "../../util/debug.h"
> +#include "../../util/auxtrace.h"
> +#include "../../util/powerpc-vpadtl.h"
> +#include "../../util/record.h"
> +#include <internal/lib.h> // page_size
> +
> +#define KiB(x) ((x) * 1024)
> +
> +static int
> +powerpc_vpadtl_recording_options(struct auxtrace_record *ar __maybe_unused,
> +			struct evlist *evlist __maybe_unused,
> +			struct record_opts *opts)
> +{
> +	opts->full_auxtrace = true;
> +
> +	/*
> +	 * Set auxtrace_mmap_pages to minimum
> +	 * two pages
> +	 */
> +	if (!opts->auxtrace_mmap_pages) {
> +		opts->auxtrace_mmap_pages = KiB(128) / page_size;
> +		if (opts->mmap_pages == UINT_MAX)
> +			opts->mmap_pages = KiB(256) / page_size;
> +	}
> +
> +	return 0;
> +}
> +
> +static size_t powerpc_vpadtl_info_priv_size(struct auxtrace_record *itr __maybe_unused,
> +					struct evlist *evlist __maybe_unused)
> +{
> +	return VPADTL_AUXTRACE_PRIV_SIZE;
> +}
> +
> +static int
> +powerpc_vpadtl_info_fill(struct auxtrace_record *itr __maybe_unused,
> +		struct perf_session *session __maybe_unused,
> +		struct perf_record_auxtrace_info *auxtrace_info,
> +		size_t priv_size __maybe_unused)
> +{
> +	auxtrace_info->type = PERF_AUXTRACE_VPA_DTL;
> +
> +	return 0;
> +}
> +
> +static void powerpc_vpadtl_free(struct auxtrace_record *itr)
> +{
> +	free(itr);
> +}
> +
> +static u64 powerpc_vpadtl_reference(struct auxtrace_record *itr __maybe_unused)
> +{
> +	return 0;
> +}
> +
> +struct auxtrace_record *vpa_dtl_recording_init(struct evsel *pos, int *err)
> +{
> +	struct auxtrace_record *aux;
> +
> +	/*
> +	 * To obtain the auxtrace buffer file descriptor, the auxtrace event
> +	 * must come first.
> +	 */
> +	evlist__to_front(pos->evlist, pos);
> +
> +	aux = zalloc(sizeof(*aux));
> +	if (aux == NULL) {
> +		pr_debug("aux record allocation failed (-ENOMEM)\n");
> +		*err = -ENOMEM;
> +		return NULL;
> +	}
> +
> +	aux->recording_options = powerpc_vpadtl_recording_options;
> +	aux->info_priv_size = powerpc_vpadtl_info_priv_size;
> +	aux->info_fill = powerpc_vpadtl_info_fill;
> +	aux->free = powerpc_vpadtl_free;
> +	aux->reference = powerpc_vpadtl_reference;
> +	return aux;
> +}
> diff --git a/tools/perf/util/powerpc-vpadtl.h b/tools/perf/util/powerpc-vpadtl.h
> index ca809660b9bb..68a780c63204 100644
> --- a/tools/perf/util/powerpc-vpadtl.h
> +++ b/tools/perf/util/powerpc-vpadtl.h
> @@ -20,4 +20,7 @@ struct perf_pmu;
>  int powerpc_vpadtl_process_auxtrace_info(union perf_event *event,
>  				  struct perf_session *session);
>  
> +struct evsel;
> +struct auxtrace_record;
> +struct auxtrace_record *vpa_dtl_recording_init(struct evsel *pos, int *err);
>  #endif