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:     Ingo Molnar <mingo@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        "acme@kernel.org" <acme@kernel.org>,
        "alexander.shishkin@linux.intel.com" 
        <alexander.shishkin@linux.intel.com>,
        "jolsa@redhat.com" <jolsa@redhat.com>,
        "eranian@google.com" <eranian@google.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "alexey.budankov@linux.intel.com" <alexey.budankov@linux.intel.com>,
        "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/VvvMmSxkC06uIJGGK3pKUZrRIAgAAbn4CAANwqgIAAu/qAgAF8bAA=
Date:   Sat, 13 Oct 2018 08:31:37 +0000
Message-ID: <E4F81A5E-C371-429D-AFEC-6F4B32971F09@fb.com>
References: <20181010104559.GO5728@hirez.programming.kicks-ass.net>
 <D2014055-9B25-4152-A582-7AD42A3837AB@fb.com>
 <20181011092913.GA9848@hirez.programming.kicks-ass.net>
 <70079805-1CAE-4CAA-813A-F8DDB929F22B@fb.com>
 <20181012095001.GG9867@hirez.programming.kicks-ass.net>
In-Reply-To: <20181012095001.GG9867@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: <2807E429505FF14B8FCF2D35B7E5CDE4@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c503643c-be53-4ca3-9396-08d630e64adb
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Oct 2018 08:31:37.3837
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1406
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-10-13_05:,,
 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 12, 2018, at 2:50 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>=20
>=20
> Can we please not top-post?
>=20
> On Thu, Oct 11, 2018 at 10:37:14PM +0000, Song Liu wrote:
>> Thanks Peter! These are really really helpful.=20
>>=20
>> I am trying to think through the case of a group of two events on two=20
>> separate hardware PMUs. In current implementation, this will not trigger
>> move_group,
>=20
> Right, currently this is disallowed (or should be, I'll need to double
> check the code).
>=20
>> so they will not be able to rotate together? And actually,=20
>> they may not be able to run at all? Maybe this case is never supported?=
=20
>=20
> Indeed, we do not allow mixing events of different PMUs, with the
> explicit exception of software events. Since software events must always
> schedule, they're allowed to be fitted into any group.
>=20
>> On the other hand, would something like this work:
>>=20
>>    perf_cpu_context <-[1:2]-> perf_event_context <-[1:n]-> perf_event=20
>>              |                                                |
>>              `----[1:n]---->     pmu    <----- [1:n]----------'=20
>>=20

After reading the code more, I think my idea in the figure above is=20
similar to this patch. The "pmu" in the figure is actually =20
perf_cpu_pmu_context. And I was thinking about something similar to=20
current pmu (not in the figure above).=20

I spent about two hours right here try to explain my idea. I ended=20
up delete everything I typed and agree almost all your design=20
decisions.=20

I just realized that, if we don't allow group of events on two=20
different hardware PMUs, the design of this patch works very well.=20
Rotation of multiple PMUs at the same time is not necessary.=20

The only suggestion I have right now is on which struct owns which
data:

1. perf_cpu_context owns two perf_event_context: ctx and *task_ctx.=20
   This is the same as right now.=20
2. perf_event_context owns multiple perf_event_pmu_context:=20
   One perf_event_pmu_context for software groups;
   One perf_event_pmu_context for each hardware PMU.
3. perf_event_pmu_context owns RB tree of events. Since we don't=20
   need rotation across multiple hardware PMUs, the rotation is=20
   within same perf_event_pmu_context. =20
4. perf_cpu_context owns multiple perf_cpu_pmu_context:
   One perf_cpu_pmu_context for each hardware PMU.
   perf_cpu_pmu_context is tot needed for software only groups(?).
5. perf_cpu_pmu_context has two pointers of perf_event_pmu_context.


The following diff (on top of this patch) shows the idea above.=20
I don't think it changes any mechanism. But it feels simpler to me.=20

Thanks,
Song

diff --git i/include/linux/perf_event.h w/include/linux/perf_event.h
index 462315239f8f..b15e679d4802 100644
--- i/include/linux/perf_event.h
+++ w/include/linux/perf_event.h
@@ -762,10 +762,7 @@ struct perf_event_context {
        struct mutex                    mutex;

        struct list_head                pmu_ctx_list;
-
-       struct perf_event_groups        pinned_groups;
-       struct perf_event_groups        flexible_groups;
-       struct list_head                event_list;
+       struct perf_event_pmu_context   sw_ctx;

        int                             nr_events;
        int                             nr_active;
@@ -806,7 +803,7 @@ struct perf_event_context {
 #define PERF_NR_CONTEXTS       4

 struct perf_cpu_pmu_context {
-       struct perf_event_pmu_context   epc;
+       struct perf_event_pmu_context   *epc;  /* I am still debating this =
one */
        struct perf_event_pmu_context   *task_epc;

        struct list_head                sched_cb_entry;
@@ -827,6 +824,7 @@ struct perf_cpu_pmu_context {
 struct perf_cpu_context {
        struct perf_event_context       ctx;
        struct perf_event_context       *task_ctx;
+       struct list_head                list_of_perf_cpu_pmu_context; /* ma=
y be removed? */

 #ifdef CONFIG_CGROUP_PERF
        struct perf_cgroup              *cgrp;
@@ -834,6 +832,10 @@ struct perf_cpu_context {
 #endif

        int                             online;
+
+       struct perf_event_groups        pinned_groups;
+       struct perf_event_groups        flexible_groups;
+       struct list_head                event_list;
 };

 struct perf_output_handle {