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

[Song Liu <songliubraving@fb.com>]

Hi Peter,

> On Oct 15, 2018, at 3:09 PM, Song Liu <songliubraving@fb.com> wrote:
> 
> 
> 
>> On Oct 15, 2018, at 1:34 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> 
>> On Mon, Oct 15, 2018 at 10:26:06AM +0300, Alexey Budankov wrote:
>>> Hi,
>>> 
>>> On 10.10.2018 13:45, Peter Zijlstra wrote:
>>>> Hi all,
>>>> 
>>>> There have been various issues and limitations with the way perf uses
>>>> (task) contexts to track events. Most notable is the single hardware PMU
>>>> task context, which has resulted in a number of yucky things (both
>>>> proposed and merged).
>>>> 
>>>> Notably:
>>>> 
>>>> - HW breakpoint PMU
>>>> - ARM big.little PMU
>>>> - Intel Branch Monitoring PMU
>>>> 
>>>> Since we now track the events in RB trees, we can 'simply' add a pmu
>>>> order to them and have them grouped that way, reducing to a single
>>>> context. Of course, reality never quite works out that simple, and below
>>>> ends up adding an intermediate data structure to bridge the context ->
>>>> pmu mapping.
>>>> 
>>>> Something a little like:
>>>> 
>>>>             ,------------------------[1:n]---------------------.
>>>>             V                                                  V
>>>>   perf_event_context <-[1:n]-> perf_event_pmu_context <--- perf_event
>>>>             ^                      ^     |                     |
>>>>             `--------[1:n]---------'     `-[n:1]-> pmu <-[1:n]-'
>>>> 
>>>> This patch builds (provided you disable CGROUP_PERF), boots and survives
>>>> perf-top without the machine catching fire.
>>>> 
>>>> There's still a fair bit of loose ends (look for XXX), but I think this
>>>> is the direction we should be going.
>>>> 
>>>> Comments?
>>>> 
>>>> Not-Quite-Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>>> ---
>>>> arch/powerpc/perf/core-book3s.c |    4 
>>>> arch/x86/events/core.c          |    4 
>>>> arch/x86/events/intel/core.c    |    6 
>>>> arch/x86/events/intel/ds.c      |    6 
>>>> arch/x86/events/intel/lbr.c     |   16 
>>>> arch/x86/events/perf_event.h    |    6 
>>>> include/linux/perf_event.h      |   80 +-
>>>> include/linux/sched.h           |    2 
>>>> kernel/events/core.c            | 1412 ++++++++++++++++++++--------------------
>>>> 9 files changed, 815 insertions(+), 721 deletions(-)
>>> 
>>> Rewrite is impressive however it doesn't result in code base reduction as it is.
>> 
>> Yeah.. that seems to be nature of these things ..
>> 
>>> Nonetheless there is a clear demand for per pmu events groups tracking and rotation 
>>> in single cpu context (HW breakpoints, ARM big.little, Intel LBRs) and there is 
>>> a supply thru groups ordering on RB-tree.
>>> 
>>> This might be driven into the kernel by some new Perf features that would base on 
>>> that RB-tree groups ordering or by refactoring of existing code but in the way it 
>>> would result in overall code base reduction thus lowering support cost.
>> 
>> If you have a concrete suggestion on how to reduce complexity? I tried,
>> but couldn't find any (without breaking something).
>> 
>> The active lists and pmu_ctx_list could arguably be replaced with
>> (slower) iteratons over the RB tree, but you'll still need the per pmu
>> nr_events/nr_active counts to determine if rotation is required at all.
>> 
>> And like you know, performance is quite important here too. I'd love to
>> reduce complexity while maintaining or improve performance, but that
>> rarely if ever happens :/
> 
> How about this: 
> 
> 1. Keep multiple perf_cpu_context per CPU, just like before this patch. 
> 
> 2. For perf_event_context, add PMU as an order for the RB tree. 
> 
> 3. (hw) pmu->perf_cpu_context->ctx only has events for this PMU (and sw 
>   events moved to this context).
> 
> 4. task->perf_event_ctxp has events for all PMUs. 
> 
> With this path, we keep the existing perf_cpu_context/perf_event_context
> logic as-is, which I think is simp\x10ler than the new logic (with extra
> *_pmu_context). And it should also solve the problem. 
> 
> Does this make sense? If this doesn't look too broken, I am happy to
> draft RFC for it. 
> 

I am not sure whether you missed this one, or found it totally insane. 
Could you please share your comments on it? My gut feeling is that this 
would be a simpler patch to solve the problem (two hw PMUs). (It might 
be less efficient though). 

Thanks,
Song