Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Song Liu <songliubraving@fb.com>
To:     Peter Zijlstra <peterz@infradead.org>
CC:     Alexey Budankov <alexey.budankov@linux.intel.com>,
        Ingo Molnar <mingo@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        "acme@kernel.org" <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Stephane Eranian <eranian@google.com>,
        "Thomas Gleixner" <tglx@linutronix.de>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "megha.dey@intel.com" <megha.dey@intel.com>,
        "frederic@kernel.org" <frederic@kernel.org>
Subject: Re: [RFC][PATCH] perf: Rewrite core context handling
Thread-Topic: [RFC][PATCH] perf: Rewrite core context handling
Thread-Index: AQHUYIaFJ/VvvMmSxkC06uIJGGK3pKUf754AgAATMgCAAOOBAA==
Date:   Mon, 15 Oct 2018 22:09:05 +0000
Message-ID: <AF0FCD08-E014-4D29-B023-EA780056DBB5@fb.com>
References: <20181010104559.GO5728@hirez.programming.kicks-ass.net>
 <3a738a08-2295-a4e9-dce7-a3e2b2ad794e@linux.intel.com>
 <20181015083448.GN9867@hirez.programming.kicks-ass.net>
In-Reply-To: <20181015083448.GN9867@hirez.programming.kicks-ass.net>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-ID: <BAB7A45BFFE1B548AD425F0BF87214FA@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 9f425410-10f7-4327-e803-08d632ead2c3
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Oct 2018 22:09:05.6651
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1391
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-10-15_13:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Oct 15, 2018, at 1:34 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>=20
> On Mon, Oct 15, 2018 at 10:26:06AM +0300, Alexey Budankov wrote:
>> Hi,
>>=20
>> On 10.10.2018 13:45, Peter Zijlstra wrote:
>>> Hi all,
>>>=20
>>> There have been various issues and limitations with the way perf uses
>>> (task) contexts to track events. Most notable is the single hardware PM=
U
>>> task context, which has resulted in a number of yucky things (both
>>> proposed and merged).
>>>=20
>>> Notably:
>>>=20
>>> - HW breakpoint PMU
>>> - ARM big.little PMU
>>> - Intel Branch Monitoring PMU
>>>=20
>>> 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 belo=
w
>>> ends up adding an intermediate data structure to bridge the context ->
>>> pmu mapping.
>>>=20
>>> Something a little like:
>>>=20
>>>              ,------------------------[1:n]---------------------.
>>>              V                                                  V
>>>    perf_event_context <-[1:n]-> perf_event_pmu_context <--- perf_event
>>>              ^                      ^     |                     |
>>>              `--------[1:n]---------'     `-[n:1]-> pmu <-[1:n]-'
>>>=20
>>> This patch builds (provided you disable CGROUP_PERF), boots and survive=
s
>>> perf-top without the machine catching fire.
>>>=20
>>> There's still a fair bit of loose ends (look for XXX), but I think this
>>> is the direction we should be going.
>>>=20
>>> Comments?
>>>=20
>>> Not-Quite-Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>> ---
>>> arch/powerpc/perf/core-book3s.c |    4=20
>>> arch/x86/events/core.c          |    4=20
>>> arch/x86/events/intel/core.c    |    6=20
>>> arch/x86/events/intel/ds.c      |    6=20
>>> arch/x86/events/intel/lbr.c     |   16=20
>>> arch/x86/events/perf_event.h    |    6=20
>>> include/linux/perf_event.h      |   80 +-
>>> include/linux/sched.h           |    2=20
>>> kernel/events/core.c            | 1412 ++++++++++++++++++++------------=
--------
>>> 9 files changed, 815 insertions(+), 721 deletions(-)
>>=20
>> Rewrite is impressive however it doesn't result in code base reduction a=
s it is.
>=20
> Yeah.. that seems to be nature of these things ..
>=20
>> Nonetheless there is a clear demand for per pmu events groups tracking a=
nd rotation=20
>> in single cpu context (HW breakpoints, ARM big.little, Intel LBRs) and t=
here is=20
>> a supply thru groups ordering on RB-tree.
>>=20
>> This might be driven into the kernel by some new Perf features that woul=
d base on=20
>> that RB-tree groups ordering or by refactoring of existing code but in t=
he way it=20
>> would result in overall code base reduction thus lowering support cost.
>=20
> If you have a concrete suggestion on how to reduce complexity? I tried,
> but couldn't find any (without breaking something).
>=20
> 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.
>=20
> 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:=20

1. Keep multiple perf_cpu_context per CPU, just like before this patch.=20

2. For perf_event_context, add PMU as an order for the RB tree.=20

3. (hw) pmu->perf_cpu_context->ctx only has events for this PMU (and sw=20
   events moved to this context).

4. task->perf_event_ctxp has events for all PMUs.=20

With this path, we keep the existing perf_cpu_context/perf_event_context
logic as-is, which I think is simp=10ler than the new logic (with extra
*_pmu_context). And it should also solve the problem.=20

Does this make sense? If this doesn't look too broken, I am happy to
draft RFC for it.=20

Thanks,
Song