| Message ID | 20201214084502.19954-1-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drivers/perf: Enable PID_IN_CONTEXTIDR with SPE | expand |
On Mon, Dec 14, 2020 at 10:45:02AM +0200, James Clark wrote: > Enable PID_IN_CONTEXTIDR by default when Arm SPE is enabled. > This flag is required to get PID data in the SPE trace. Without > it the perf tool will report 0 for PID which isn't very useful, > especially when doing system wide profiling or profiling > applications that fork. > > There is a small performance overhead when enabling > PID_IN_CONTEXTIDR, but SPE itself is optional and not enabled by > default so the impact is minimised. > > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Al Grant <al.grant@arm.com> > Cc: Leo Yan <leo.yan@linaro.org> > Cc: John Garry <john.garry@huawei.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: James Clark <james.clark@arm.com> > --- > arch/arm64/Kconfig.debug | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug > index 265c4461031f..b030bb21a0bb 100644 > --- a/arch/arm64/Kconfig.debug > +++ b/arch/arm64/Kconfig.debug > @@ -2,6 +2,7 @@ > > config PID_IN_CONTEXTIDR > bool "Write the current PID to the CONTEXTIDR register" > + default y if ARM_SPE_PMU > help > Enabling this option causes the kernel to write the current PID to > the CONTEXTIDR register, at the expense of some additional Given that PID_IN_CONTEXTIDR doesn't take PID namespacing into account, IIUC it's kinda broken today (and arguably removing that support would be better). Can we not track the (namespaced) PID in thte main ringbuffer regardless of PID_IN_CONTEXTIDR, and leave PID_IN_CONTEXTIDR as an external debug aid only? Making this default y is ARM_SPE_PMU implies it'll be on in all distro kernels, and I think we need to think harder before doing that. Thanks, Mark.
> From: Mark Rutland <mark.rutland@arm.com> > Sent: 06 January 2021 10:24 > To: James Clark <James.Clark@arm.com> > Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux- > perf-users@vger.kernel.org; will@kernel.org; leo.yan@linaro.org; Al Grant > <Al.Grant@arm.com>; John Garry <john.garry@huawei.com>; Suzuki Poulose > <Suzuki.Poulose@arm.com>; Mathieu Poirier <mathieu.poirier@linaro.org>; > Catalin Marinas <Catalin.Marinas@arm.com> > Subject: Re: [PATCH v2] drivers/perf: Enable PID_IN_CONTEXTIDR with SPE > > On Mon, Dec 14, 2020 at 10:45:02AM +0200, James Clark wrote: > > Enable PID_IN_CONTEXTIDR by default when Arm SPE is enabled. > > This flag is required to get PID data in the SPE trace. Without it the > > perf tool will report 0 for PID which isn't very useful, especially > > when doing system wide profiling or profiling applications that fork. > > > > There is a small performance overhead when enabling PID_IN_CONTEXTIDR, > > but SPE itself is optional and not enabled by default so the impact is > > minimised. > > > > Cc: Will Deacon <will@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Al Grant <al.grant@arm.com> > > Cc: Leo Yan <leo.yan@linaro.org> > > Cc: John Garry <john.garry@huawei.com> > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Signed-off-by: James Clark <james.clark@arm.com> > > --- > > arch/arm64/Kconfig.debug | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug index > > 265c4461031f..b030bb21a0bb 100644 > > --- a/arch/arm64/Kconfig.debug > > +++ b/arch/arm64/Kconfig.debug > > @@ -2,6 +2,7 @@ > > > > config PID_IN_CONTEXTIDR > > bool "Write the current PID to the CONTEXTIDR register" > > + default y if ARM_SPE_PMU > > help > > Enabling this option causes the kernel to write the current PID to > > the CONTEXTIDR register, at the expense of some additional > > Given that PID_IN_CONTEXTIDR doesn't take PID namespacing into account, > IIUC it's kinda broken today (and arguably removing that support would be > better). > > Can we not track the (namespaced) PID in thte main ringbuffer regardless of > PID_IN_CONTEXTIDR, and leave PID_IN_CONTEXTIDR as an external debug aid > only? The (namespaced) PID is already tracked in other perf records; the point of putting PID in CONTEXTIDR is that SPE and ETM will capture it into the AUX buffer, and this can be used to correlate AUX buffer events with other perf events when the AUX buffer doesn't have hardware timestamps that are sufficiently precise and can be converted to kernel time. Right now, a kernel may be enabling PID_IN_CONTEXTIDR for other reasons, and in a SPE or ETM perf session run from within a container, the AUX buffer will be capturing root-namespace PIDs. This is wrong, as a container, and an events created in a container's non-root PID namespace, should not have visibility of root-namespace PIDs. The solution is for the SPE and ETM PMU drivers to disable CONTEXTIDR capture if the event's PID namespace is non-root. For SPE and ETM4, this is straightforward - both trace formats are self-describing and perf's decoder can cope with CONTEXTIDR being absent even if it was requested. For ETM3 and PTM (on older 32-bit cores) this might not be the case. So it may be better to have perf_event_open fail and have userspace retry with contextid sampling disabled. This does mean perf sessions from within a container will lack PIDs in AUX buffers (so correlation relying on that will fail), but that's better than having these sessions expose root-namespace PIDs as they do now. perf sessions from the root namespace just work. (The alternative, of adjusting CONTEXTIDR to trace non-root-namespace PIDs, doesn't work in general - the namespace belongs to the event tracing the process, not the process being traced. Worst case, you might have a process subject to an SPE event in one namespace and an ETM event in another - there is no value of CONTEXTIDR that works for both.) Al > Making this default y is ARM_SPE_PMU implies it'll be on in all distro kernels, and > I think we need to think harder before doing that.
diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug index 265c4461031f..b030bb21a0bb 100644 --- a/arch/arm64/Kconfig.debug +++ b/arch/arm64/Kconfig.debug @@ -2,6 +2,7 @@ config PID_IN_CONTEXTIDR bool "Write the current PID to the CONTEXTIDR register" + default y if ARM_SPE_PMU help Enabling this option causes the kernel to write the current PID to the CONTEXTIDR register, at the expense of some additional
Enable PID_IN_CONTEXTIDR by default when Arm SPE is enabled. This flag is required to get PID data in the SPE trace. Without it the perf tool will report 0 for PID which isn't very useful, especially when doing system wide profiling or profiling applications that fork. There is a small performance overhead when enabling PID_IN_CONTEXTIDR, but SPE itself is optional and not enabled by default so the impact is minimised. Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Al Grant <al.grant@arm.com> Cc: Leo Yan <leo.yan@linaro.org> Cc: John Garry <john.garry@huawei.com> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Signed-off-by: James Clark <james.clark@arm.com> --- arch/arm64/Kconfig.debug | 1 + 1 file changed, 1 insertion(+)