linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] drivers/perf: Enable PID_IN_CONTEXTIDR with SPE
@ 2020-12-14  8:45 James Clark
  2021-01-06 10:24 ` Mark Rutland
  0 siblings, 1 reply; 3+ messages in thread
From: James Clark @ 2020-12-14  8:45 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, linux-perf-users, will, leo.yan
  Cc: James Clark, Mark Rutland, Al Grant, John Garry,
	Suzuki K Poulose, Mathieu Poirier, Catalin Marinas

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
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] drivers/perf: Enable PID_IN_CONTEXTIDR with SPE
  2020-12-14  8:45 [PATCH v2] drivers/perf: Enable PID_IN_CONTEXTIDR with SPE James Clark
@ 2021-01-06 10:24 ` Mark Rutland
  2021-01-07 18:00   ` Al Grant
  0 siblings, 1 reply; 3+ messages in thread
From: Mark Rutland @ 2021-01-06 10:24 UTC (permalink / raw)
  To: James Clark
  Cc: linux-arm-kernel, linux-kernel, linux-perf-users, will, leo.yan,
	Al Grant, John Garry, Suzuki K Poulose, Mathieu Poirier,
	Catalin Marinas

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* RE: [PATCH v2] drivers/perf: Enable PID_IN_CONTEXTIDR with SPE
  2021-01-06 10:24 ` Mark Rutland
@ 2021-01-07 18:00   ` Al Grant
  0 siblings, 0 replies; 3+ messages in thread
From: Al Grant @ 2021-01-07 18:00 UTC (permalink / raw)
  To: Mark Rutland, James Clark
  Cc: linux-arm-kernel, linux-kernel, linux-perf-users, will, leo.yan,
	John Garry, Suzuki Poulose, Mathieu Poirier, Catalin Marinas

> 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.


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-01-07 18:02 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-14  8:45 [PATCH v2] drivers/perf: Enable PID_IN_CONTEXTIDR with SPE James Clark
2021-01-06 10:24 ` Mark Rutland
2021-01-07 18:00   ` Al Grant

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).