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

Ravi Bangoria
2022-08-25T11:04:18Z

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