From: German Gomez <german.gomez@arm.com> To: Leo Yan <leo.yan@linaro.org> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, mark.rutland@arm.com, james.clark@arm.com Subject: Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX Date: Wed, 16 Feb 2022 15:16:01 +0000 [thread overview] Message-ID: <7d50f7ba-d8e5-1fbc-e9e6-749a24e26555@arm.com> (raw) In-Reply-To: <20220216132257.GB56419@leoy-ThinkPad-X240s> Hi Leo, Thanks for the review On 16/02/2022 13:22, Leo Yan wrote: > On Tue, Feb 15, 2022 at 02:29:27PM +0000, German Gomez wrote: > > [...] > >> Thanks for the suggestion. I recorded the following stacktrace: >> >> perf-323841 [052] d.... 3996.528812: arm_spe_pmu_setup_aux: (arm_spe_pmu_setup_aux+0x60/0x1c0 [arm_spe_pmu]) >> perf-323841 [052] d.... 3996.528813: <stack trace> > Yeah, this show arm_spe_pmu_setup_aux() is called in perf process. > >> => kprobe_dispatcher >> => kprobe_breakpoint_handler >> => call_break_hook >> => brk_handler >> => do_debug_exception >> => el1_dbg >> => el1h_64_sync_handler >> => el1h_64_sync >> => arm_spe_pmu_setup_aux >> => perf_mmap >> => mmap_region >> => do_mmap >> => vm_mmap_pgoff >> => ksys_mmap_pgoff >> => __arm64_sys_mmap >> => invoke_syscall >> => el0_svc_common.constprop.0 >> => do_el0_svc >> => el0_svc >> => el0t_64_sync_handler >> => el0t_64_sync >> >> So for a v2 I may include something like this: > The change looks good to me, please see below minor comment. > >> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c >> index d44bcc29d..aadec5a0e 100644 >> --- a/drivers/perf/arm_spe_pmu.c >> +++ b/drivers/perf/arm_spe_pmu.c >> @@ -45,6 +45,7 @@ struct arm_spe_pmu_buf { >> int nr_pages; >> bool snapshot; >> void *base; >> + u64 pmscr; >> }; >> >> struct arm_spe_pmu { >> @@ -748,7 +749,7 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags) >> write_sysreg_s(reg, SYS_PMSICR_EL1); >> } >> >> - reg = arm_spe_event_to_pmscr(event); >> + reg = ((struct arm_spe_pmu_buf *) perf_get_aux(handle))->pmscr; >> isb(); >> write_sysreg_s(reg, SYS_PMSCR_EL1); > Just nitpick (or it's just my preferring coding style), we can define > a local pointer variable 'buf': > > struct arm_spe_pmu_buf *buf = perf_get_aux(handle); I need to make sure perf_get_aux(..) is called between perf_aux_output_begin and *_end though (so, after arm_spe_perf_aux_output_begin(..)): buf = perf_get_aux(handle); reg = buf->pmscr; isb(); write_sysreg_s(buf, SYS_PMSCR_EL1); Alternatively, we set the register inside of perf_aux_output_begin. It might be confusing for casual readers because the function handles a case where perf_get_aux(..) returns NULL. Alternatively, we could also wrap perf_get_aux(..) in a static inline function that returns the correct type and do: reg = arm_spe_get_aux(handle)->pmscr; isb(); write_sysreg_s(reg, SYS_PMSCR_EL1); So that it looks cleaner. > > ... > > isb(); > write_sysreg_s(buf->pmscr, SYS_PMSCR_EL1); > > Thanks, > Leo > >> } >> @@ -855,6 +856,8 @@ static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages, >> if (!pglist) >> goto out_free_buf; >> >> + buf->pmscr = arm_spe_event_to_pmscr(event); >> + >> for (i = 0; i < nr_pages; ++i) >> pglist[i] = virt_to_page(pages[i]); >> >>> Thanks, >>> Leo
WARNING: multiple messages have this Message-ID (diff)
From: German Gomez <german.gomez@arm.com> To: Leo Yan <leo.yan@linaro.org> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, mark.rutland@arm.com, james.clark@arm.com Subject: Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX Date: Wed, 16 Feb 2022 15:16:01 +0000 [thread overview] Message-ID: <7d50f7ba-d8e5-1fbc-e9e6-749a24e26555@arm.com> (raw) In-Reply-To: <20220216132257.GB56419@leoy-ThinkPad-X240s> Hi Leo, Thanks for the review On 16/02/2022 13:22, Leo Yan wrote: > On Tue, Feb 15, 2022 at 02:29:27PM +0000, German Gomez wrote: > > [...] > >> Thanks for the suggestion. I recorded the following stacktrace: >> >> perf-323841 [052] d.... 3996.528812: arm_spe_pmu_setup_aux: (arm_spe_pmu_setup_aux+0x60/0x1c0 [arm_spe_pmu]) >> perf-323841 [052] d.... 3996.528813: <stack trace> > Yeah, this show arm_spe_pmu_setup_aux() is called in perf process. > >> => kprobe_dispatcher >> => kprobe_breakpoint_handler >> => call_break_hook >> => brk_handler >> => do_debug_exception >> => el1_dbg >> => el1h_64_sync_handler >> => el1h_64_sync >> => arm_spe_pmu_setup_aux >> => perf_mmap >> => mmap_region >> => do_mmap >> => vm_mmap_pgoff >> => ksys_mmap_pgoff >> => __arm64_sys_mmap >> => invoke_syscall >> => el0_svc_common.constprop.0 >> => do_el0_svc >> => el0_svc >> => el0t_64_sync_handler >> => el0t_64_sync >> >> So for a v2 I may include something like this: > The change looks good to me, please see below minor comment. > >> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c >> index d44bcc29d..aadec5a0e 100644 >> --- a/drivers/perf/arm_spe_pmu.c >> +++ b/drivers/perf/arm_spe_pmu.c >> @@ -45,6 +45,7 @@ struct arm_spe_pmu_buf { >> int nr_pages; >> bool snapshot; >> void *base; >> + u64 pmscr; >> }; >> >> struct arm_spe_pmu { >> @@ -748,7 +749,7 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags) >> write_sysreg_s(reg, SYS_PMSICR_EL1); >> } >> >> - reg = arm_spe_event_to_pmscr(event); >> + reg = ((struct arm_spe_pmu_buf *) perf_get_aux(handle))->pmscr; >> isb(); >> write_sysreg_s(reg, SYS_PMSCR_EL1); > Just nitpick (or it's just my preferring coding style), we can define > a local pointer variable 'buf': > > struct arm_spe_pmu_buf *buf = perf_get_aux(handle); I need to make sure perf_get_aux(..) is called between perf_aux_output_begin and *_end though (so, after arm_spe_perf_aux_output_begin(..)): buf = perf_get_aux(handle); reg = buf->pmscr; isb(); write_sysreg_s(buf, SYS_PMSCR_EL1); Alternatively, we set the register inside of perf_aux_output_begin. It might be confusing for casual readers because the function handles a case where perf_get_aux(..) returns NULL. Alternatively, we could also wrap perf_get_aux(..) in a static inline function that returns the correct type and do: reg = arm_spe_get_aux(handle)->pmscr; isb(); write_sysreg_s(reg, SYS_PMSCR_EL1); So that it looks cleaner. > > ... > > isb(); > write_sysreg_s(buf->pmscr, SYS_PMSCR_EL1); > > Thanks, > Leo > >> } >> @@ -855,6 +856,8 @@ static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages, >> if (!pglist) >> goto out_free_buf; >> >> + buf->pmscr = arm_spe_event_to_pmscr(event); >> + >> for (i = 0; i < nr_pages; ++i) >> pglist[i] = virt_to_page(pages[i]); >> >>> Thanks, >>> Leo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-02-16 15:16 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-17 12:44 [RFC PATCH 0/2] perf: arm_spe: Fix consistency of CONTEXT packets in SPE driver German Gomez 2022-01-17 12:44 ` German Gomez 2022-01-17 12:44 ` [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX German Gomez 2022-01-17 12:44 ` German Gomez 2022-01-18 10:07 ` Will Deacon 2022-01-18 10:07 ` Will Deacon 2022-01-18 14:04 ` German Gomez 2022-01-18 14:04 ` German Gomez 2022-01-19 11:27 ` German Gomez 2022-01-19 11:27 ` German Gomez 2022-01-18 16:28 ` James Clark 2022-01-18 16:28 ` James Clark 2022-02-05 15:39 ` Leo Yan 2022-02-05 15:39 ` Leo Yan 2022-02-07 12:06 ` German Gomez 2022-02-07 12:06 ` German Gomez 2022-02-08 13:00 ` Leo Yan 2022-02-08 13:00 ` Leo Yan 2022-02-10 17:23 ` German Gomez 2022-02-10 17:23 ` German Gomez 2022-02-11 10:45 ` Leo Yan 2022-02-11 10:45 ` Leo Yan 2022-02-15 14:29 ` German Gomez 2022-02-15 14:29 ` German Gomez 2022-02-16 13:22 ` Leo Yan 2022-02-16 13:22 ` Leo Yan 2022-02-16 15:16 ` German Gomez [this message] 2022-02-16 15:16 ` German Gomez 2022-01-17 12:44 ` [RFC PATCH 2/2] perf: arm_spe: Enable CONTEXT packets in SPE traces if the profiler runs in CPU mode German Gomez 2022-01-17 12:44 ` German Gomez 2022-01-17 14:04 ` German Gomez 2022-01-17 14:04 ` German Gomez 2022-01-18 9:52 ` Will Deacon 2022-01-18 9:52 ` Will Deacon 2022-01-18 14:13 ` German Gomez 2022-01-18 14:13 ` German Gomez
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=7d50f7ba-d8e5-1fbc-e9e6-749a24e26555@arm.com \ --to=german.gomez@arm.com \ --cc=james.clark@arm.com \ --cc=leo.yan@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.