From: Jiri Olsa <jolsa@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Jiri Olsa <jolsa@kernel.org>,
linux-kernel@vger.kernel.org, Andi Kleen <andi@firstfloor.org>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Corey Ashford <cjashfor@linux.vnet.ibm.com>,
David Ahern <dsahern@gmail.com>,
Frederic Weisbecker <fweisbec@gmail.com>,
Ingo Molnar <mingo@kernel.org>,
"Jen-Cheng(Tommy) Huang" <tommy24@gatech.edu>,
Namhyung Kim <namhyung@kernel.org>,
Paul Mackerras <paulus@samba.org>,
Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH 1/9] perf: Remove redundant parent context check from context_equiv
Date: Mon, 8 Sep 2014 18:45:24 +0200 [thread overview]
Message-ID: <20140908164524.GJ17728@krava.brq.redhat.com> (raw)
In-Reply-To: <20140908151305.GU3588@twins.programming.kicks-ass.net>
On Mon, Sep 08, 2014 at 05:13:05PM +0200, Peter Zijlstra wrote:
> On Mon, Sep 08, 2014 at 03:34:28PM +0200, Peter Zijlstra wrote:
> > On Mon, Sep 08, 2014 at 02:01:41PM +0200, Jiri Olsa wrote:
> > > On Mon, Sep 08, 2014 at 12:01:22PM +0200, Peter Zijlstra wrote:
> > > > On Mon, Sep 08, 2014 at 11:48:55AM +0200, Peter Zijlstra wrote:
> > > >
> > > > > > The thing is; I don't understand those reasons. That commit log doesn't
> > > > > > explain.
> > > > >
> > > > > Ah wait, I finally see. I think we want to fix that exit path, not
> > > > > disallow the cloning.
> > > > >
> > > > > The thing is, by not allowing this optimization simple things like eg.
> > > > > pipe-test say very expensive.
> > > >
> > > > So its 179033b3e064 ("perf: Add PERF_EVENT_STATE_EXIT state for events
> > > > with exited task") that introduces the problem. Before that things would
> > > > work correctly afaict.
> > >
> > > hum, I dont think so.. because the perf_remove_from_context set event
> > > to PERF_EVENT_STATE_OFF state anyway.. thus making any new cloned events
> > > disabled
> >
> > Urgh, see I knew I was missing something.
> >
> > Can't we fix that? Lemme check to see what relies on this.
>
> 2e2af50b1fab ("perf_events: Disable events when we detach them")
>
> Seems to be about it. And I think we should solve that differently, but
> the best I can come up with ties into the event->ctx mess we have in
> that other thread.
>
> The thing is, IOC_ENABLE/DISABLE and read() and such should act
> (sanely and) independent from the attached state.
>
> Its just that the whole event->ctx migration mess is making this
> somewhat hard atm.
>
> So things like perf_event_read() should not only check ctx->is_active
> but also worry about event->attach_state & PERF_ATTACH_CONTEXT.
>
> Now the biggest problem is that we cannot tell if its a temporary state
> (move_group / migrate_context) or permanent (exit)...
>
> Urgh
I just noticed that we initialize the child state with base parent
state not the real (immediate) parent.. which is what we want IMO
I wonder attached patch could fix the issue mentioned in:
1f9a726 perf: Do not allow optimized switch for non-cloned events
now I need to recall what I used to test this ;-)
jirka
---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index e4d6924..561a4ea 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7794,6 +7794,7 @@ inherit_event(struct perf_event *parent_event,
struct perf_event *group_leader,
struct perf_event_context *child_ctx)
{
+ enum perf_event_active_state parent_state = parent_event->state;
struct perf_event *child_event;
unsigned long flags;
@@ -7827,7 +7828,7 @@ inherit_event(struct perf_event *parent_event,
* not its attr.disabled bit. We hold the parent's mutex,
* so we won't race with perf_event_{en, dis}able_family.
*/
- if (parent_event->state >= PERF_EVENT_STATE_INACTIVE)
+ if (parent_state >= PERF_EVENT_STATE_INACTIVE)
child_event->state = PERF_EVENT_STATE_INACTIVE;
else
child_event->state = PERF_EVENT_STATE_OFF;
next prev parent reply other threads:[~2014-09-08 16:46 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-25 14:45 [RFCv2 0/9] perf: Allow leader sampling on inherited events Jiri Olsa
2014-08-25 14:45 ` [PATCH 1/9] perf: Remove redundant parent context check from context_equiv Jiri Olsa
2014-09-02 10:50 ` Peter Zijlstra
2014-09-08 9:43 ` Jiri Olsa
2014-09-08 9:45 ` Peter Zijlstra
2014-09-08 9:48 ` Peter Zijlstra
2014-09-08 10:01 ` Peter Zijlstra
2014-09-08 11:39 ` Peter Zijlstra
2014-09-08 12:19 ` Jiri Olsa
2014-09-08 13:20 ` Peter Zijlstra
2014-09-08 12:32 ` Jiri Olsa
2014-09-08 12:01 ` Jiri Olsa
2014-09-08 13:34 ` Peter Zijlstra
2014-09-08 15:13 ` Peter Zijlstra
2014-09-08 16:45 ` Jiri Olsa [this message]
2014-09-09 10:20 ` Peter Zijlstra
2014-09-10 13:57 ` Peter Zijlstra
2014-09-10 14:35 ` Jiri Olsa
2014-09-24 14:58 ` [tip:perf/core] Revert "perf: Do not allow optimized switch for non-cloned events" tip-bot for Jiri Olsa
2014-08-25 14:45 ` [PATCH 2/9] perf: Deny optimized switch for events read by PERF_SAMPLE_READ Jiri Olsa
2014-09-02 10:52 ` Peter Zijlstra
2014-09-08 10:00 ` Jiri Olsa
2014-09-08 10:11 ` Peter Zijlstra
2014-09-08 16:39 ` Jiri Olsa
2014-08-25 14:45 ` [PATCH 3/9] perf: Allow PERF_FORMAT_GROUP format on inherited events Jiri Olsa
2014-08-25 14:45 ` [PATCH 4/9] perf tools: Add support to traverse xyarrays Jiri Olsa
2014-08-25 14:45 ` [PATCH 5/9] perf tools: Add pr_warning_once debug macro Jiri Olsa
2014-08-25 14:45 ` [PATCH 6/9] perf tools: Add hash of periods for struct perf_sample_id Jiri Olsa
2014-08-25 14:45 ` [PATCH 7/9] perf tools: Allow PERF_FORMAT_GROUP for inherited events Jiri Olsa
2014-08-25 14:45 ` [PATCH 8/9] perf script: Add period data column Jiri Olsa
2014-08-27 14:33 ` David Ahern
2014-10-17 16:10 ` Jiri Olsa
2014-10-17 18:22 ` Arnaldo Carvalho de Melo
2014-10-18 7:07 ` [tip:perf/urgent] " tip-bot for Jiri Olsa
2014-08-25 14:45 ` [PATCH 9/9] perf script: Add period as a default output column Jiri Olsa
2014-08-27 14:40 ` David Ahern
2014-10-17 16:11 ` Jiri Olsa
2014-10-18 7:07 ` [tip:perf/urgent] " tip-bot for Jiri Olsa
2014-08-25 14:51 ` [RFCv2 0/9] perf: Allow leader sampling on inherited events Jiri Olsa
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=20140908164524.GJ17728@krava.brq.redhat.com \
--to=jolsa@redhat.com \
--cc=acme@kernel.org \
--cc=andi@firstfloor.org \
--cc=cjashfor@linux.vnet.ibm.com \
--cc=dsahern@gmail.com \
--cc=eranian@google.com \
--cc=fweisbec@gmail.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=namhyung@kernel.org \
--cc=paulus@samba.org \
--cc=peterz@infradead.org \
--cc=tommy24@gatech.edu \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).