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.gf1...@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?
