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/VvvMmSxkC06uIJGGK3pKUf754AgAATMgCAAOOBAIABVJuA
Date:   Tue, 16 Oct 2018 18:28:10 +0000
Message-ID: <FE66AE20-9785-4CF7-8D8D-CF2A9C696923@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>
 <AF0FCD08-E014-4D29-B023-EA780056DBB5@fb.com>
In-Reply-To: <AF0FCD08-E014-4D29-B023-EA780056DBB5@fb.com>
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: <F52FC9D74A06AE4BB926C7BA09EC52B1@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 4aec9290-f09f-475c-50dc-08d633952082
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Oct 2018 18:28:10.6757
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1616
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-10-16_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

Hi Peter,

> On Oct 15, 2018, at 3:09 PM, Song Liu <songliubraving@fb.com> wrote:
>=20
>=20
>=20
>> 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 P=
MU
>>>> 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 bel=
ow
>>>> 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 surviv=
es
>>>> perf-top without the machine catching fire.
>>>>=20
>>>> There's still a fair bit of loose ends (look for XXX), but I think thi=
s
>>>> 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 =
as 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 =
and rotation=20
>>> in single cpu context (HW breakpoints, ARM big.little, Intel LBRs) and =
there is=20
>>> a supply thru groups ordering on RB-tree.
>>>=20
>>> This might be driven into the kernel by some new Perf features that wou=
ld base on=20
>>> that RB-tree groups ordering or by refactoring of existing code but in =
the 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 :/
>=20
> How about this:=20
>=20
> 1. Keep multiple perf_cpu_context per CPU, just like before this patch.=20
>=20
> 2. For perf_event_context, add PMU as an order for the RB tree.=20
>=20
> 3. (hw) pmu->perf_cpu_context->ctx only has events for this PMU (and sw=20
>   events moved to this context).
>=20
> 4. task->perf_event_ctxp has events for all PMUs.=20
>=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
>=20
> Does this make sense? If this doesn't look too broken, I am happy to
> draft RFC for it.=20
>=20

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

Thanks,
Song=20