Re: [RFC] perf arm-spe: Track task context switch for cpu-mode events

[Leo Yan <leo.yan@linaro.org>]

Hi all,

On Fri, Oct 01, 2021 at 11:22:33AM -0700, Stephane Eranian wrote:
> On Fri, Oct 1, 2021 at 3:44 AM James Clark <james.clark@arm.com> wrote:
> > On 30/09/2021 19:47, Stephane Eranian wrote:
> > > On Thu, Sep 23, 2021 at 9:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >> On Thu, Sep 23, 2021 at 7:23 AM Leo Yan <leo.yan@linaro.org> wrote:

[...]

> > >>> Yes, after enabled config PID_IN_CONTEXTIDR, the kernel will always
> > >>> write PID into the system register CONTEXTIDR during process context
> > >>> switching.  Please see the flow:
> > >>>
> > >>>   __switch_to() (arch/arm64/kernel/process.c)
> > >>>     `-> contextidr_thread_switch(next)
> > >>
> > >> Thanks for the info.  I assume it's a light-weight operation.
> > >>
> > > I'd like to understand why it was believed that having SPE record to
> > > PID could be too expensive
> > > vs. what I am seeing with all the tracking of context switches and the
> > > volume of data this generates.
> >
> > I think the justification about it being expensive is that when PID_IN_CONTEXTIDR
> > is set, there is an extra few instructions to write that value on every context
> > switch, whether SPE is enabled or not. So it has a system wide impact.
> 
> You could use a static key to make this conditional to having SPE
> running on the CPU like
> it is done for other PMU features on other architectures.

Good point for using static key to dynamically enable and disable
CONTEXTIDR rather than using config PID_IN_CONTEXTIDR.

For curious, I did a rough testing to compare the performance for using
CONTEXTIDR, there have four testing configurations:
- 'dis': Use the mainline kernel with disabling PID_IN_CONTEXTIDR.
- 'enb': Use the mainline kernel with enablng PID_IN_CONTEXTIDR.
- 'true': Use static key='TRUE' to store PID into CONTEXTIDR.
- 'false': Use static key='FALSE' so don't store PID into CONTEXTIDR.

I used the command 'perf bench sched messaging -t -g 20 -l 1000' as
the testing case, the main reason is this case have total 800 threads
for sending and receiving messages, so it should have huge times for
context switching.

The testing iterates for 5 loops for every configuration, and get the
result for run time (in seconds):

     dis     enb     true    false
     ------------------------------
#1   26.568  26.786  26.056  26.197
#2   26.442  26.457  26.458  26.846
#3   26.719  26.701  27.119  26.281
#4   26.448  27.595  26.953  27.043
#5   27.017  27.263  26.638  26.933
     ------------------------------
avg. 26.638  26.960  26.644   26.66
     ------------------------------
delta pct.    1.21%   0.02%   0.08%

Compared with the base configuration ('dis' with disabling
PID_IN_CONTEXTIDR), we can see the performance is only minor
downgradation in other configurations ('enb': 1.21%, Enable
static key: 0.02%, Disable static key: 0.08%).  So seems to me,
it's feasible to use static key to dynamically control the path for
PID_IN_CONTEXTIDR.

@Catalin, @Will, could you confirm if it is the right direction to
use static key to replace PID_IN_CONTEXTIDR?  Or have any concern for
using the static key, like any secuirty reason?

Also pasted the code for using static key for CONTEXTIDR in the
bottom.

Thanks,
Leo

---8<---

diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
index f4ba93d4ffeb..857e35a8624a 100644
--- a/arch/arm64/include/asm/mmu_context.h
+++ b/arch/arm64/include/asm/mmu_context.h
@@ -26,9 +26,11 @@
 
 extern bool rodata_full;
 
+DECLARE_STATIC_KEY_FALSE(contextidr_used);
+
 static inline void contextidr_thread_switch(struct task_struct *next)
 {
-	if (!IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR))
+	if (!static_branch_unlikely(&contextidr_used))
 		return;
 
 	write_sysreg(task_pid_nr(next), contextidr_el1);
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 40adb8cdbf5a..f6b6e73fac9d 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -487,6 +487,22 @@ void update_sctlr_el1(u64 sctlr)
 	isb();
 }
 
+DEFINE_STATIC_KEY_FALSE(contextidr_used);
+
+static int __init contextidr_enable(char *str)
+{
+	unsigned int val = 0;
+
+	get_option(&str, &val);
+
+        if (val)
+                static_branch_enable(&contextidr_used);
+
+        return 0;
+}
+early_param("contextidr_enable", contextidr_enable);
+
 /*
  * Thread switching.
  */