Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 24 Oct 2021 18:25:30 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     James Clark <james.clark@arm.com>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Kees Cook <keescook@chromium.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Sami Tolvanen <samitolvanen@google.com>,
        Nicholas Piggin <npiggin@gmail.com>,
        James Morse <james.morse@arm.com>,
        Marc Zyngier <maz@kernel.org>, Joey Gouly <joey.gouly@arm.com>,
        Peter Collingbourne <pcc@google.com>,
        Vincenzo Frascino <vincenzo.frascino@arm.com>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Stephane Eranian <eranian@google.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [RFCv1 4/4] perf: arm_spe: Dynamically switch PID tracing to
 contextidr
Message-ID: <20211024102530.GI213960@leoy-ThinkPad-X240s>
References: <20211021134530.206216-1-leo.yan@linaro.org>
 <20211021134530.206216-5-leo.yan@linaro.org>
 <854fb1a2-e5f1-f237-685f-8ddb0557c98b@arm.com>
 <0a53529a-576c-05b0-4fe5-906195faf243@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0a53529a-576c-05b0-4fe5-906195faf243@arm.com>
Precedence: bulk

Hi James,

On Fri, Oct 22, 2021 at 05:23:23PM +0100, James Clark wrote:
> On 22/10/2021 16:36, James Clark wrote:
> > 
> > 
> > On 21/10/2021 14:45, Leo Yan wrote:
> >> Now Arm64 provides API for enabling and disable PID tracing, Arm SPE
> >> driver invokes these functions to dynamically enable it during
> >> profiling when the program runs in root PID name space, and disable PID
> >> tracing when the perf event is stopped.
> >>
> >> Device drivers should not depend on CONFIG_PID_IN_CONTEXTIDR for PID
> >> tracing, so this patch uses the consistent condition for setting bit
> >> EL1_CX for PMSCR.
> > 
> > Hi Leo,
> > 
> > I've been testing this change, but I'm seeing something strange. Not sure
> > if it's a problem on my side or not yet. With this command:
> > 
> >  sudo ./perf record -vvv -e arm_spe//u -- taskset --cpu-list 1 bash -c ls
> > 
> > I'm only seeing 0 values for context:
> > 
> >  sudo ./perf report -D | grep CONTEXT
> > 
> > .  00038dce:  65 00 00 00 00                                  CONTEXT 0x0 el2
> > .  00038e0e:  65 00 00 00 00                                  CONTEXT 0x0 el2

Good catch!  I reproduced this issue at my side and looked into the
flow, the root cause is relevant with timing.

When perf launches the program 'taskset --cpu-list 1 bash -c ls', it
forks a new process and 'ls' program is scheduled in, then function
arm_spe_pmu_start() invokes contextidr_enable() to enable the PID
tracing in contextidr.  Since 'ls' program executes very short and it
simply runs to the end (so in the middle of 'ls' there have no any
context switching on the CPU), there have no any new PID is written
into contextidr and CPU's contextidr keeps zero.  This is the reason
we see the context packets contain zeros for PID.

To fix this issue, we should enable PID tracing when setup AUX ring
buffer, at this phase, the profiled program has not been started yet.
So when the profiled program is scheduled in at the first time, PID
traing is getting ready and we can see the expected context packet in
Arm SPE trace data.   So this patch should be updated as below, I will
apply it in next spin if no objection.

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index c21cf1385cc0..85aa2eab0c2e 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -876,6 +867,13 @@ static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages,
        buf->nr_pages   = nr_pages;
        buf->snapshot   = snapshot;

+       /*
+        * Enable tracing PID to contextidr if profiling program runs in
+        * root PID namespace.
+        */
+       if (perfmon_capable() && (task_active_pid_ns(current) == &init_pid_ns))
+               contextidr_enable();
+
        kfree(pglist);
        return buf;

@@ -890,6 +888,9 @@ static void arm_spe_pmu_free_aux(void *aux)
 {
        struct arm_spe_pmu_buf *buf = aux;

+       if (perfmon_capable() && (task_active_pid_ns(current) == &init_pid_ns))
+               contextidr_disable();
+
        vunmap(buf->base);
        kfree(buf);
 }

Thanks a lot for detailed testing!

Leo