All of lore.kernel.org
 help / color / mirror / Atom feed
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
Date: Sat, 13 Oct 2018 08:31:37 +0000	[thread overview]
Message-ID: <E4F81A5E-C371-429D-AFEC-6F4B32971F09@fb.com> (raw)
In-Reply-To: <20181012095001.GG9867@hirez.programming.kicks-ass.net>



> On Oct 12, 2018, at 2:50 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> 
> Can we please not top-post?
> 
> On Thu, Oct 11, 2018 at 10:37:14PM +0000, Song Liu wrote:
>> Thanks Peter! These are really really helpful. 
>> 
>> I am trying to think through the case of a group of two events on two 
>> separate hardware PMUs. In current implementation, this will not trigger
>> move_group,
> 
> Right, currently this is disallowed (or should be, I'll need to double
> check the code).
> 
>> so they will not be able to rotate together? And actually, 
>> they may not be able to run at all? Maybe this case is never supported? 
> 
> 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.
> 
>> On the other hand, would something like this work:
>> 
>>    perf_cpu_context <-[1:2]-> perf_event_context <-[1:n]-> perf_event 
>>              |                                                |
>>              `----[1:n]---->     pmu    <----- [1:n]----------' 
>> 

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

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

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

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. 
   This is the same as right now. 
2. perf_event_context owns multiple perf_event_pmu_context: 
   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 
   need rotation across multiple hardware PMUs, the rotation is 
   within same perf_event_pmu_context.  
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. 
I don't think it changes any mechanism. But it feels simpler to me. 

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; /* may 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 {



  parent reply	other threads:[~2018-10-13  8:32 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-10 10:45 [RFC][PATCH] perf: Rewrite core context handling Peter Zijlstra
2018-10-11  7:50 ` Song Liu
2018-10-11  9:29   ` Peter Zijlstra
2018-10-11 22:37     ` Song Liu
2018-10-12  9:50       ` Peter Zijlstra
2018-10-12 14:25         ` Peter Zijlstra
2018-10-13  8:31         ` Song Liu [this message]
2018-10-16  9:50           ` Peter Zijlstra
2018-10-16 16:34             ` Song Liu
2018-10-16 18:10               ` Peter Zijlstra
2018-10-16 18:24                 ` Song Liu
2018-10-12  7:04     ` Alexey Budankov
2018-10-12 11:54       ` Peter Zijlstra
2018-10-15  7:26 ` Alexey Budankov
2018-10-15  8:34   ` Peter Zijlstra
2018-10-15  8:53     ` Peter Zijlstra
2018-10-15 17:29     ` Alexey Budankov
2018-10-15 18:31       ` Stephane Eranian
2018-10-16  6:39         ` Alexey Budankov
2018-10-16  9:32         ` Peter Zijlstra
2018-10-15 22:09     ` Song Liu
2018-10-16 18:28       ` Song Liu
2018-10-17 11:06         ` Peter Zijlstra
2018-10-17 16:43           ` Song Liu
2018-10-17 17:19             ` Peter Zijlstra
2018-10-17 18:33               ` Peter Zijlstra
2018-10-17 18:57                 ` Song Liu
2018-10-16 16:26 ` Mark Rutland
2018-10-16 18:07   ` Peter Zijlstra
2018-10-17  8:57 ` Alexey Budankov
2018-10-17 15:01   ` Alexander Shishkin
2018-10-17 15:58     ` Alexey Budankov
2018-10-17 16:30   ` Peter Zijlstra
2018-10-18  7:05     ` Alexey Budankov
2018-10-22 13:26 ` Alexander Shishkin
2018-10-23  6:13 ` Song Liu
2018-10-23  6:55   ` Peter Zijlstra
2019-05-15 11:17 ` Alexander Shishkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=E4F81A5E-C371-429D-AFEC-6F4B32971F09@fb.com \
    --to=songliubraving@fb.com \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=alexey.budankov@linux.intel.com \
    --cc=eranian@google.com \
    --cc=frederic@kernel.org \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=megha.dey@intel.com \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.