Messages in this thread Patch in this message | | | Subject | Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX | From | German Gomez <> | Date | Mon, 7 Feb 2022 12:06:14 +0000 |
| |
Hi Leo,
On 05/02/2022 15:39, Leo Yan wrote: > Hi German, > > On Mon, Jan 17, 2022 at 12:44:31PM +0000, German Gomez wrote: >> [...] >> >> 1. Run a process in the background with capability CAP_SYS_ADMIN in CPU0. >> >> $ taskset --cpu-list 0 sudo dd if=/dev/random of=/dev/null & >> [3] 3806 >> >> 2. Begin a perf session _without_ capabilities (we shouldn't see CONTEXT packets). >> >> $ perf record -e arm_spe_0// -C0 -- sleep 1 >> $ perf report -D | grep CONTEXT >> . 0000000e: 65 df 0e 00 00 CONTEXT 0xedf el2 >> . 0000004e: 65 df 0e 00 00 CONTEXT 0xedf el2 >> . 0000008e: 65 df 0e 00 00 CONTEXT 0xedf el2 >> [...] > Could you confirm if you still can reproduce this issue on the latest > mainline kernel? I cannot reproduce this issue on the latest mainline > kernel, I suspect this is impacted by recent refactoring evlist > patches from Ian Rogers (though I am not for this).
Are you referring to running on the latest kernel or the latest perf tool? I can still reproduce using perf from v5.17-rc3, on a kernel and SPE driver from 5.17.0-rc1 (commands below).
> >> As can be seen, the traces begin showing CONTEXT packets when the pid is >> 0xedf (3807). This happens because the pmu start callback is run when >> the current process is not the owner of the perf session, so the CX >> register bit is set. > I can image a potential issue is: the "dd" program running in background > with capability CAP_SYS_ADMIN on CPU0, and then perf session sends an > IPI remotely from any other CPU to CPU0, the dd process (on CPU0) is > interrupted to handle ioctl PERF_EVENT_IOC_ENABLE, thus perfmon_capable() > returns the capability of dd process, finally it leads to the wrong > setting for PMSCR. > > I reviewed the code and also traced the backtrace for the function > arm_spe_pmu_start(), I can confirm that every time perf session will > execute below flow: > > evlist__enable() > __evlist__enable() > evlist__for_each_cpu() { -> call affinity__set() > evsel__enable_cpu() > } > > We can see the macro evlist__for_each_cpu() will extend to invoke > evlist__cpu_begin() and affinity__set(); affinity__set() will set CPU > affinity to the target CPU, thus perf process will firstly migrate to > the target CPU and enable event on the target CPU. This means perf > will not send remote IPI and it directly runs on target CPU, and the > dd program will not interfere capabilities for perf session. Thank you for looking at this,
I re-tested on the N1SDP (previously I was using a graviton2 instance). I had to adjust the command slightly with "-m,2" to get it consistently this time:
$ taskset --cpu-list 0 sudo dd if=/dev/random of=/dev/null & $ perf record -e arm_spe_0// -C0 -m,2 -- sleep 1 $ perf report -D | grep CONTEXT | head . 0000000e: 65 b5 6e 00 00 CONTEXT 0x6eb5 el2 . 0000004e: 65 b5 6e 00 00 CONTEXT 0x6eb5 el2 . 0000008e: 65 b5 6e 00 00 CONTEXT 0x6eb5 el2 [...]
> > Thanks, > Leo > >> One way to fix this is by caching the value of the CX bit during the >> initialization of the PMU event, so that it remains consistent for the >> duration of the session. >> >> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n275 >> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n713 >> [3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?h=v5.16#n751 >> >> Signed-off-by: German Gomez <german.gomez@arm.com> >> --- >> drivers/perf/arm_spe_pmu.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c >> index d44bcc29d..8515bf85c 100644 >> --- a/drivers/perf/arm_spe_pmu.c >> +++ b/drivers/perf/arm_spe_pmu.c >> @@ -57,6 +57,7 @@ struct arm_spe_pmu { >> u16 pmsver; >> u16 min_period; >> u16 counter_sz; >> + bool pmscr_cx; >> >> #define SPE_PMU_FEAT_FILT_EVT (1UL << 0) >> #define SPE_PMU_FEAT_FILT_TYP (1UL << 1) >> @@ -260,6 +261,7 @@ static const struct attribute_group *arm_spe_pmu_attr_groups[] = { >> static u64 arm_spe_event_to_pmscr(struct perf_event *event) >> { >> struct perf_event_attr *attr = &event->attr; >> + struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu); >> u64 reg = 0; >> >> reg |= ATTR_CFG_GET_FLD(attr, ts_enable) << SYS_PMSCR_EL1_TS_SHIFT; >> @@ -272,7 +274,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event) >> if (!attr->exclude_kernel) >> reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT); >> >> - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable()) >> + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && spe_pmu->pmscr_cx) >> reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT); >> >> return reg; >> @@ -709,10 +711,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event) >> !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT)) >> return -EOPNOTSUPP; >> >> + spe_pmu->pmscr_cx = perfmon_capable(); >> reg = arm_spe_event_to_pmscr(event); >> if (!perfmon_capable() && >> (reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) | >> - BIT(SYS_PMSCR_EL1_CX_SHIFT) | >> BIT(SYS_PMSCR_EL1_PCT_SHIFT)))) >> return -EACCES; >> >> -- >> 2.25.1 >>
| |