On 22.10.2019 12:43, Peter Zijlstra wrote: > 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*.
Argh, and that is why it is documented as the optional one. Undoubtedly, we have to avoid crashes on other architectures. So "if (pmu->sync_task_ctx)" has to be a part of v5. > > Also, I think I prefer the variant I gave you yesterday: > > > https://lkml.kernel.org/r/[email protected] > > 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). I would name the method sync_task_ctx not swap_task_ctx because sync reserves broader meaning, IMHO. ~Alexey
