From: James Clark <james.clark@arm.com>
To: Anshuman Khandual <anshuman.khandual@arm.com>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, will@kernel.org,
catalin.marinas@arm.com, mark.rutland@arm.com
Cc: Mark Brown <broonie@kernel.org>, Rob Herring <robh@kernel.org>,
Marc Zyngier <maz@kernel.org>,
Suzuki Poulose <suzuki.poulose@arm.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
linux-perf-users@vger.kernel.org
Subject: Re: [PATCH V9 10/10] arm64/perf: Implement branch records save on PMU IRQ
Date: Tue, 23 May 2023 15:51:58 +0100 [thread overview]
Message-ID: <641cb79d-fe79-3873-3698-fec66a4e3253@arm.com> (raw)
In-Reply-To: <83cac0ae-7e82-d67e-c854-941c65dae79e@arm.com>
On 23/05/2023 15:39, James Clark wrote:
>
>
> On 15/03/2023 05:14, Anshuman Khandual wrote:
>> This modifies armv8pmu_branch_read() to concatenate live entries along with
>> task context stored entries and then process the resultant buffer to create
>> perf branch entry array for perf_sample_data. It follows the same principle
>> like task sched out.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>
> [...]
>
>> void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event)
>> {
>> struct brbe_hw_attr *brbe_attr = (struct brbe_hw_attr *)cpuc->percpu_pmu->private;
>> + struct arm64_perf_task_context *task_ctx = event->pmu_ctx->task_ctx_data;
>> + struct brbe_regset live[BRBE_MAX_ENTRIES];
>> + int nr_live, nr_store;
>> u64 brbfcr, brbcr;
>> - int idx, loop1_idx1, loop1_idx2, loop2_idx1, loop2_idx2, count;
>>
>> brbcr = read_sysreg_s(SYS_BRBCR_EL1);
>> brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
>> @@ -739,36 +743,13 @@ void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event)
>> write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
>> isb();
>>
>> - /* Determine the indices for each loop */
>> - loop1_idx1 = BRBE_BANK0_IDX_MIN;
>> - if (brbe_attr->brbe_nr <= BRBE_BANK_MAX_ENTRIES) {
>> - loop1_idx2 = brbe_attr->brbe_nr - 1;
>> - loop2_idx1 = BRBE_BANK1_IDX_MIN;
>> - loop2_idx2 = BRBE_BANK0_IDX_MAX;
>> - } else {
>> - loop1_idx2 = BRBE_BANK0_IDX_MAX;
>> - loop2_idx1 = BRBE_BANK1_IDX_MIN;
>> - loop2_idx2 = brbe_attr->brbe_nr - 1;
>> - }
>> -
>> - /* Loop through bank 0 */
>> - select_brbe_bank(BRBE_BANK_IDX_0);
>> - for (idx = 0, count = loop1_idx1; count <= loop1_idx2; idx++, count++) {
>> - if (!capture_branch_entry(cpuc, event, idx))
>> - goto skip_bank_1;
>> - }
>> -
>> - /* Loop through bank 1 */
>> - select_brbe_bank(BRBE_BANK_IDX_1);
>> - for (count = loop2_idx1; count <= loop2_idx2; idx++, count++) {
>> - if (!capture_branch_entry(cpuc, event, idx))
>> - break;
>> - }
>> -
>> -skip_bank_1:
>> - cpuc->branches->branch_stack.nr = idx;
>> - cpuc->branches->branch_stack.hw_idx = -1ULL;
>> + nr_live = capture_brbe_regset(brbe_attr, live);
>> + nr_store = task_ctx->nr_brbe_records;
>> + nr_store = stitch_stored_live_entries(task_ctx->store, live, nr_store,
>> + nr_live, brbe_attr->brbe_nr);
>> + process_branch_entries(cpuc, event, task_ctx->store, nr_store);
>
> Hi Anshuman,
>
> With the following command I get a crash:
>
> perf record --branch-filter any,save_type -a -- ls
>
> [ 101.171822] Unable to handle kernel NULL pointer dereference at
> virtual address 0000000000000600
> ...
> [145380.414654] Call trace:
> [145380.414739] armv8pmu_branch_read+0x7c/0x578
> [145380.414895] armv8pmu_handle_irq+0x104/0x1c0
> [145380.415043] armpmu_dispatch_irq+0x38/0x70
> [145380.415209] __handle_irq_event_percpu+0x124/0x3b8
> [145380.415392] handle_irq_event+0x54/0xc8
> [145380.415567] handle_fasteoi_irq+0x100/0x1e0
> [145380.415718] generic_handle_domain_irq+0x38/0x58
> [145380.415895] gic_handle_irq+0x5c/0x130
> [145380.416025] call_on_irq_stack+0x24/0x58
> [145380.416173] el1_interrupt+0x74/0xc0
> [145380.416321] el1h_64_irq_handler+0x18/0x28
> [145380.416475] el1h_64_irq+0x64/0x68
> [145380.416604] smp_call_function_single+0xe8/0x1f0
> [145380.416745] event_function_call+0xbc/0x1c8
> [145380.416919] _perf_event_enable+0x84/0xa0
> [145380.417069] perf_ioctl+0xe8/0xd68
> [145380.417204] __arm64_sys_ioctl+0x9c/0xe0
> [145380.417353] invoke_syscall+0x4c/0x120
> [145380.417523] el0_svc_common+0xd0/0x120
> [145380.417693] do_el0_svc+0x3c/0xb8
> [145380.417859] el0_svc+0x50/0xc0
> [145380.418004] el0t_64_sync_handler+0x84/0xf0
> [145380.418160] el0t_64_sync+0x190/0x198
>
> When using --branch-filter any,u without -a it seems to be fine so could
> be that task_ctx is null in per-cpu mode, or something to do with the
> userspace only flag?
>
> I'm also wondering if it's possible to collapse some of the last 5
> commits? They seem to mostly modify things in brbe.c which is a new file
> so the history probably isn't important at this point it just makes it a
> bit harder to review.
>
I realised I just tested V9 instead of V10 but I diffed them and don't
see anything that would change this issue so it's probably on both versions.
>> process_branch_aborts(cpuc);
>> + task_ctx->nr_brbe_records = 0;
>>
>> /* Unpause the buffer */
>> write_sysreg_s(brbfcr & ~BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
next prev parent reply other threads:[~2023-05-23 14:52 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-15 5:14 [PATCH V9 00/10] arm64/perf: Enable branch stack sampling Anshuman Khandual
2023-03-15 5:14 ` [PATCH V9 01/10] drivers: perf: arm_pmu: Add new sched_task() callback Anshuman Khandual
2023-03-15 5:14 ` [PATCH V9 02/10] arm64/perf: Add BRBE registers and fields Anshuman Khandual
2023-04-12 8:32 ` Yang Shen
2023-05-15 6:22 ` Anshuman Khandual
2023-03-15 5:14 ` [PATCH V9 03/10] arm64/perf: Add branch stack support in struct arm_pmu Anshuman Khandual
2023-03-15 5:14 ` [PATCH V9 04/10] arm64/perf: Add branch stack support in struct pmu_hw_events Anshuman Khandual
2023-03-15 5:14 ` [PATCH V9 05/10] arm64/perf: Add branch stack support in ARMV8 PMU Anshuman Khandual
2023-03-15 5:14 ` [PATCH V9 06/10] arm64/perf: Enable branch stack events via FEAT_BRBE Anshuman Khandual
2023-03-15 5:14 ` [PATCH V9 07/10] arm64/perf: Add PERF_ATTACH_TASK_DATA to events with has_branch_stack() Anshuman Khandual
2023-03-15 5:14 ` [PATCH V9 08/10] arm64/perf: Add struct brbe_regset helper functions Anshuman Khandual
2023-03-15 5:14 ` [PATCH V9 09/10] arm64/perf: Implement branch records save on task sched out Anshuman Khandual
2023-03-15 5:14 ` [PATCH V9 10/10] arm64/perf: Implement branch records save on PMU IRQ Anshuman Khandual
2023-05-23 14:39 ` James Clark
2023-05-23 14:51 ` James Clark [this message]
2023-05-24 3:10 ` Anshuman Khandual
2023-03-21 19:02 ` [PATCH V9 00/10] arm64/perf: Enable branch stack sampling Mark Brown
2023-03-23 4:25 ` Anshuman Khandual
2023-03-23 12:54 ` Mark Brown
2023-03-24 3:20 ` Anshuman Khandual
2023-03-24 11:40 ` Mark Brown
2023-04-11 13:03 ` Will Deacon
2023-05-15 6:24 ` Anshuman Khandual
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=641cb79d-fe79-3873-3698-fec66a4e3253@arm.com \
--to=james.clark@arm.com \
--cc=acme@kernel.org \
--cc=anshuman.khandual@arm.com \
--cc=broonie@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=maz@kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=robh@kernel.org \
--cc=suzuki.poulose@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).