Re: [PATCH 2/7] perf: Generalize task_function_call()ers

[Alexander Shishkin]

Peter Zijlstra <peterz@xxxxxxxxxxxxx> writes:

> On Thu, Dec 17, 2015 at 04:07:32PM +0100, Peter Zijlstra wrote:
>> On Thu, Dec 17, 2015 at 04:25:14PM +0200, Alexander Shishkin wrote:
>> 
>> > That aside, why I brought it up in the first place is because the two
>> > functions are asymmetric: one is called with irqs disabled and the
>> > other -- with ctx::lock held (and not because I'm into bikeshedding or
>> > anything like that). Looking at the pair of them sets off my "that's not
>> > right" trigger and sends me to the event_function_call()
>> > implementation. So in that sense, prepending an extra underscore kind of
>> > made sense. Maybe __perf_remove_from_context_{on,off}()?
>> 
>> You are quite right, and I think I've found more problems because of
>> this. Let me prod at this some more.
>
> So this then...
>
> This fixes, I think, 3 separate bugs:
>
>  - remove_from_context used to clear ->is_active, this is against the
>    update rules from ctx_sched_in() which set ->is_active even though
>    there might be !nr_events
>
>  - install_in_context did bad things to cpuctx->task_ctx; it would not
>    validate that ctx->task == current and could do random things because
>    of that.
>
>  - cpuctx->task_ctx tracking was iffy
>
> It also unifies a lot of the weird and fragile code we had around all
> those IPI calls and adds a bunch of assertions.
>
> It seems to survive a little pounding with 'normal' workloads.
>
> Please have an extra careful look..

I notice that you dropped this from your queue, do you have any plans to
proceed with this, or should I pick it up?

Regards,
--
Alex