Messages in this thread | | | Date | Tue, 22 Oct 2019 11:43:00 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v4 4/4] perf/core,x86: synchronize PMU task contexts on optimized context switches |
| |
On Tue, Oct 22, 2019 at 09:01:11AM +0300, Alexey Budankov wrote:
> swap(ctx->task_ctx_data, next_ctx->task_ctx_data); > > + /* > + * PMU specific parts of task perf context can require > + * additional synchronization which makes sense only if > + * both next_ctx->task_ctx_data and ctx->task_ctx_data > + * pointers are allocated. As an example of such > + * synchronization see implementation details of Intel > + * LBR call stack data profiling; > + */ > + if (ctx->task_ctx_data && next_ctx->task_ctx_data) > + pmu->sync_task_ctx(next_ctx->task_ctx_data, > + ctx->task_ctx_data);
This still does not check if pmu->sync_task_ctx is set. If any other arch ever uses task_ctx_data without then also supplying this method things will go *bang*.
Also, I think I prefer the variant I gave you yesterday:
https://lkml.kernel.org/r/20191021103745.GF1800@hirez.programming.kicks-ass.net
if (pmu->swap_task_ctx) pmu->swap_task_ctx(ctx, next_ctx); else swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
That also unconfuses the argument order in your above patch (where you have to undo thw swap).
Alternatively, since there currently is no other arch using task_ctx_data, we can make the pmu::swap_task_ctx() thing mandatory when having it and completely replace the swap(), write it like so:
- swap(ctx->task_ctx_data, next_ctx->task_ctx_data); + if (pmu->swap_task_ctx) + pmu->swap_task_ctx(ctx, next_ctx);
Hmm?
| |