Re: [PATCH 07/14] tools/perf: Add basic CONFIG_AUXTRACE support for VPA pmu on powerpc

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 27 Aug 2025, at 10:57 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
> On 15/08/2025 11:34, Athira Rajeev wrote:
>> The powerpc PMU collecting Dispatch Trace Log (DTL) entries makes use of
>> AUX support in perf infrastructure. The PMU driver has the functionality
>> to collect trace entries in the aux buffer. On the tools side, this data
>> is made available as PERF_RECORD_AUXTRACE records. This record is
>> generated by "perf record" command. To enable the creation of
>> PERF_RECORD_AUXTRACE, add functions to initialize auxtrace records ie
>> "auxtrace_record__init()". Fill in fields for other callbacks like
>> info_priv_size, info_fill, free, recording options etc. Define
>> auxtrace_type as PERF_AUXTRACE_VPA_PMU. Add header file to define vpa
>> dtl pmu specific details.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
>> ---
>> tools/perf/arch/powerpc/util/Build      |   1 +
>> tools/perf/arch/powerpc/util/auxtrace.c | 122 ++++++++++++++++++++++++
>> tools/perf/util/auxtrace.c              |   2 +
>> tools/perf/util/auxtrace.h              |   1 +
>> tools/perf/util/powerpc-vpadtl.h        |  26 +++++
>> 5 files changed, 152 insertions(+)
>> create mode 100644 tools/perf/arch/powerpc/util/auxtrace.c
>> create mode 100644 tools/perf/util/powerpc-vpadtl.h
>> 
>> diff --git a/tools/perf/arch/powerpc/util/Build b/tools/perf/arch/powerpc/util/Build
>> index fdd6a77a3432..a5b0babd307e 100644
>> --- a/tools/perf/arch/powerpc/util/Build
>> +++ b/tools/perf/arch/powerpc/util/Build
>> @@ -10,3 +10,4 @@ perf-util-$(CONFIG_LIBDW) += skip-callchain-idx.o
>> 
>> perf-util-$(CONFIG_LIBUNWIND) += unwind-libunwind.o
>> perf-util-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o
>> +perf-util-$(CONFIG_AUXTRACE) += auxtrace.o
>> diff --git a/tools/perf/arch/powerpc/util/auxtrace.c b/tools/perf/arch/powerpc/util/auxtrace.c
>> new file mode 100644
>> index 000000000000..ec8ec601fd08
>> --- /dev/null
>> +++ b/tools/perf/arch/powerpc/util/auxtrace.c
>> @@ -0,0 +1,122 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * VPA support
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/types.h>
>> +#include <linux/bitops.h>
>> +#include <linux/log2.h>
>> +#include <time.h>
>> +
>> +#include "../../util/cpumap.h"
>> +#include "../../util/evsel.h"
>> +#include "../../util/evlist.h"
>> +#include "../../util/session.h"
>> +#include "../../util/util.h"
>> +#include "../../util/pmu.h"
>> +#include "../../util/debug.h"
>> +#include "../../util/auxtrace.h"
>> +#include "../../util/powerpc-vpadtl.h"
> 
> It would be better to only add #includes when they are needed
> 
>> +#include "../../util/record.h"
>> +#include <internal/lib.h> // page_size
>> +
>> +#define KiB(x) ((x) * 1024)
>> +
>> +static int
>> +powerpc_vpadtl_parse_snapshot_options(struct auxtrace_record *itr __maybe_unused,
>> + struct record_opts *opts __maybe_unused,
>> + const char *str __maybe_unused)
>> +{
>> + return 0;
>> +}
>> +
>> +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 0;
> 
> 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 __maybe_unused,
> 
> auxtrace_info is not __maybe_unused
> 
>> + size_t priv_size __maybe_unused)
>> +{
>> + auxtrace_info->type = PERF_AUXTRACE_VPA_PMU;
>> +
>> + return 0;
>> +}
>> +
>> +static u64 powerpc_vpadtl_reference(struct auxtrace_record *itr __maybe_unused)
>> +{
>> + return 0;
>> +}
>> +
>> +static void powerpc_vpadtl_free(struct auxtrace_record *itr)
>> +{
>> + free(itr);
>> +}
>> +
>> +struct auxtrace_record *auxtrace_record__init(struct evlist *evlist __maybe_unused,
> 
> evlist is not __maybe_unused
> 
>> + int *err)
>> +{
>> + struct auxtrace_record *aux;
>> + struct evsel *pos;
>> + char *pmu_name;
>> + int found = 0;
>> +
>> + evlist__for_each_entry(evlist, pos) {
>> + pmu_name = strdup(pos->name);
>> + pmu_name = strtok(pmu_name, "/");
>> + if (!strcmp(pmu_name, "vpa_dtl")) {
> 
> pmu_name is leaked but strstarts() could be used instead
> of above
> 
>> + found = 1;
>> + pos->needs_auxtrace_mmap = true;
>> + break;
>> + }
>> + }
>> +
>> + 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;
>> + }
>> +
>> + aux->parse_snapshot_options = powerpc_vpadtl_parse_snapshot_options;
> 
> Doesn't look like snapshot mode is supported, so
> powerpc_vpadtl_parse_snapshot_options() is not needed
> 
>> + 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;
> 
> reference is optional.  powerpc_vpadtl_reference() stub is not needed

Hi Adrian

I have posted a V2 for perf tools side patches here: https://lore.kernel.org/linux-perf-users/20250915072754.99850-1-atrajeev@linux.ibm.com/T/#t

I have incorporated changes based on your comments except these two:

1) For aux->reference
auxtrace_record__reference is called by __auxtrace_mmap__read:

__auxtrace_mmap__read -> auxtrace_record__reference  
This access reference callback as below:

u64 auxtrace_record__reference(struct auxtrace_record *itr)
{                if (itr)
                return itr->reference(itr);
        return 0;
}        

For this patch series, I have added reference stub for vpa dtl, otherwise the record will segfault :

#0  0x0000000000000000 in ?? ()
#1  0x00000000102c5134 in auxtrace_record.reference ()
#2  0x00000000102c52c4 in __auxtrace_mmap__read ()
#3  0x000000001002ae68 in record.mmap_read_evlist ()
#4  0x000000001002fc7c in cmd_record ()
#5  0x00000000100b1dc0 in run_builtin ()
#6  0x00000000100b2310 in handle_internal_command ()
#7  0x000000001000f2f8 in main ()
(gdb) q


2) Change for auxtrace_queues__add_event

auxtrace_queues__process_index is not called for dump_trace. It is called for (!dump_trace)  to add entries to auxtrace queue to process later for perf report/script without -D
When -D is used ( dump_trace is set ) , to dump the auxtrace buffer content , auxtrace_queues__add_event is used to print the buffer content at time of processing PERF_RECORD_AUXTRACE records.
So I have kept that change as part of the patch. Because I want to dump the buffer content at time of processing the AUXTRACE record.

If any comments/suggestions please provide your valuable review comments on V2. 

Thanks
Athira

> 
>> + return aux;
>> +}
>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>> index ebd32f1b8f12..f587d386c5ef 100644
>> --- a/tools/perf/util/auxtrace.c
>> +++ b/tools/perf/util/auxtrace.c
>> @@ -55,6 +55,7 @@
>> #include "hisi-ptt.h"
>> #include "s390-cpumsf.h"
>> #include "util/mmap.h"
>> +#include "powerpc-vpadtl.h"
> 
> Isn't needed yet
> 
>> 
>> #include <linux/ctype.h>
>> #include "symbol/kallsyms.h"
>> @@ -1393,6 +1394,7 @@ int perf_event__process_auxtrace_info(struct perf_session *session,
>> case PERF_AUXTRACE_HISI_PTT:
>> err = hisi_ptt_process_auxtrace_info(event, session);
>> break;
>> + case PERF_AUXTRACE_VPA_PMU:
>> case PERF_AUXTRACE_UNKNOWN:
>> default:
>> return -EINVAL;
>> diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
>> index f001cbb68f8e..1f9ef473af77 100644
>> --- a/tools/perf/util/auxtrace.h
>> +++ b/tools/perf/util/auxtrace.h
>> @@ -50,6 +50,7 @@ enum auxtrace_type {
>> PERF_AUXTRACE_ARM_SPE,
>> PERF_AUXTRACE_S390_CPUMSF,
>> PERF_AUXTRACE_HISI_PTT,
>> + PERF_AUXTRACE_VPA_PMU,
> 
> Everything else is called some variation of vpa dtl, so
> PERF_AUXTRACE_VPA_DTL would seem a more consistent name
> 
>> };
>> 
>> enum itrace_period_type {
>> diff --git a/tools/perf/util/powerpc-vpadtl.h b/tools/perf/util/powerpc-vpadtl.h
>> new file mode 100644
>> index 000000000000..625172adaba5
>> --- /dev/null
>> +++ b/tools/perf/util/powerpc-vpadtl.h
>> @@ -0,0 +1,26 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * VPA DTL PMU Support
>> + */
>> +
>> +#ifndef INCLUDE__PERF_POWERPC_VPADTL_H__
>> +#define INCLUDE__PERF_POWERPC_VPADTL_H__
>> +
>> +#define POWERPC_VPADTL_NAME "powerpc_vpadtl_"
>> +
>> +enum {
>> + POWERPC_VPADTL_TYPE,
>> + VPADTL_PER_CPU_MMAPS,
> 
> VPADTL_PER_CPU_MMAPS is never used
> 
>> + VPADTL_AUXTRACE_PRIV_MAX,
>> +};
>> +
>> +#define VPADTL_AUXTRACE_PRIV_SIZE (VPADTL_AUXTRACE_PRIV_MAX * sizeof(u64))
>> +
>> +union perf_event;
>> +struct perf_session;
>> +struct perf_pmu;
>> +
>> +int powerpc_vpadtl_process_auxtrace_info(union perf_event *event,
>> +   struct perf_session *session);
> 
> None of these definitions are used in this patch, although probably
> VPADTL_AUXTRACE_PRIV_SIZE should be.
> It would be better to add definitions only when they are needed.
> 
>> +
>> +#endif