Re: [RFC v2] perf: Rewrite core context handling

From: Ravi Bangoria <ravi.bangoria@amd.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: acme@kernel.org, alexander.shishkin@linux.intel.com,
	jolsa@redhat.com, namhyung@kernel.org, songliubraving@fb.com,
	eranian@google.com, alexey.budankov@linux.intel.com,
	ak@linux.intel.com, mark.rutland@arm.com, megha.dey@intel.com,
	frederic@kernel.org, maddy@linux.ibm.com, irogers@google.com,
	kim.phillips@amd.com, linux-kernel@vger.kernel.org,
	santosh.shukla@amd.com, ravi.bangoria@amd.com
Subject: Re: [RFC v2] perf: Rewrite core context handling
Date: Thu, 25 Aug 2022 16:33:51 +0530	[thread overview]
Message-ID: <05ba363d-12e4-83ca-fb20-203c93a27f3d@amd.com> (raw)
In-Reply-To: <YwY8u7gx6bO+RBcg@worktop.programming.kicks-ass.net>

On 24-Aug-22 8:29 PM, Peter Zijlstra wrote:
> On Wed, Aug 24, 2022 at 02:15:13PM +0200, Peter Zijlstra wrote:
>> On Mon, Jun 13, 2022 at 04:35:11PM +0200, Peter Zijlstra wrote:
>>>  void x86_pmu_update_cpu_context(struct pmu *pmu, int cpu)
>>>  {
>>> -	struct perf_cpu_context *cpuctx;
>>> +	/* XXX: Don't need this quirk anymore */
>>> +	/*struct perf_cpu_context *cpuctx;
>>>  
>>>  	if (!pmu->pmu_cpu_context)
>>>  		return;
>>>  
>>>  	cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
>>> -	cpuctx->ctx.pmu = pmu;
>>> +	cpuctx->ctx.pmu = pmu;*/
>>>  }
>>
>> Confirmed; my ADL seems to work fine without all that.
> 
> Additionally; this doesn't insta crash.

While collating this I came across armv8pmu_start() which does:

	struct perf_event_context *task_ctx =
		this_cpu_ptr(cpu_pmu->pmu.pmu_cpu_context)->task_ctx;

	if (sysctl_perf_user_access && task_ctx && task_ctx->nr_user)

Not sure why it does not lock task_ctx. Should it be changed to
something like below? Untested:

---

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 016072a89f8f..747415a5f2b2 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -806,10 +806,19 @@ static void armv8pmu_disable_event(struct perf_event *event)
 
 static void armv8pmu_start(struct arm_pmu *cpu_pmu)
 {
-	struct perf_event_context *task_ctx =
-		this_cpu_ptr(cpu_pmu->pmu.pmu_cpu_context)->task_ctx;
+	struct perf_event_context *ctx;
+	int nr_user = 0;
+
+	rcu_read_lock();
+	ctx = rcu_dereference(current->perf_event_ctxp);
+	if (ctx) {
+		raw_spin_lock(&ctx->lock);
+		nr_user = ctx->nr_user;
+		raw_spin_unlock(&ctx->lock);
+	}
+	rcu_read_unlock();
 
-	if (sysctl_perf_user_access && task_ctx && task_ctx->nr_user)
+	if (sysctl_perf_user_access && nr_user)
 		armv8pmu_enable_user_access(cpu_pmu);
 	else
 		armv8pmu_disable_user_access();
---

Thanks,
Ravi
