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

> On 12 Aug 2026, at 3:00 PM, Adrian Hunter <[email protected]> wrote:
> 
> 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 <[email protected]>
> 
> A couple of cosmetic / very minor comments.
> 
> Nevertheless:
> 
> Reviewed-by: Adrian Hunter <[email protected]>
> 
>> ---
>> 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;

Sure, I will address this in next version

Thanks for review Adrian.
I will wait for feedback on the remaining patches and post a v6 incorporating all the changes together.

Athira

> 
>> 
>> /*
>>  * 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?
Ok, I will check this
> 
>> +#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