Messages in this thread | | | From | "Zhu, DengCheng" <> | Subject | RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec | Date | Tue, 22 Nov 2011 14:20:21 +0000 |
| |
> ________________________________________ > From: Peter Zijlstra [a.p.zijlstra@chello.nl] > Sent: Tuesday, November 22, 2011 9:45 PM > To: Zhu, DengCheng > Cc: Barzilay, Eyal; Fortuna, Zenon; Paul Mackerras; Ingo Molnar; Arnaldo Carvalho de Melo; ralf@linux-mips.org; LKML > Subject: RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec > > On Tue, 2011-11-22 at 13:24 +0000, Zhu, DengCheng wrote: >> > ________________________________________ >> > From: Peter Zijlstra [a.p.zijlstra@chello.nl] >> > Sent: Tuesday, November 22, 2011 6:51 PM >> > To: Zhu, DengCheng >> > Cc: Barzilay, Eyal; Fortuna, Zenon; Paul Mackerras; Ingo Molnar; Arnaldo Carvalho de Melo; ralf@linux-mips.org; LKML >> > Subject: Re: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec >> > >> > On Tue, 2011-11-22 at 11:30 +0800, Deng-Cheng Zhu wrote: >> >> Currently, when grouped events are created disabled and enable-on-exec, the >> >> siblings won't be enabled on exec in fact. The problem looks like this: >> > >> > Arguably that's a daft thing to do, since if the leader is disabled the >> > group won't get scheduled anyway. But I guess we should at least try to >> > deal with it when people do do it. >> >> Well, by "grouped events" I mean "all of the grouped events", not only the >> group leader. In fact the leader (and only the leader) will be enabled by >> going through ctx->flexible_groups in perf_event_enable_on_exec(). > > Right, I understood that. What I said was daft was to tag the > non-leaders as enabled_on_exec,disabled. They wouldn't get scheduled > anyway for as long as the leader is off. > >> > Seems perf-stat is a bit daft this way. >> > >> >> This patch fixes it. >> > >> > I guess it does, but its not pretty, event_enable_on_exec() already >> > calls __perf_event_mark_enable(), now this recursion is limited because >> > siblings can't have a sibling list of their own, but still. >> >> I did it like this just by reading the code comment of >> __perf_event_mark_enabled(): "Enabling the leader of a group effectively >> enables all the group members that aren't explicitly disabled ... Note: >> this works for group members as well as group leaders since the non-leader >> members' sibling_lists will be empty." >> >> So I suppose dealing with siblings' state in this traversal is the right >> thing to do and introduces minimal code turmoil, although the latter is by >> no means critical. > > Yeah, I just don't really like the recursion thing... Also, there's more > ways to get to __perf_event_mark_enabled() and not all those want to > actually do enable_on_exec().
Yep, two other functions call it. And whether doing enable_on_exec() in __perf_event_mark_enabled() depends on how we interpret the meaning of the latter. And if we do enable_on_exec() in it, uninterested events will be filtered out in enable_on_exec().
One thing in your patch is uncertain to me:
> @@ -2463,11 +2461,25 @@ static int event_enable_on_exec(struct p > if (event->state >= PERF_EVENT_STATE_INACTIVE) > return 0; > > - __perf_event_mark_enabled(event, ctx); > + event->state = PERF_EVENT_STATE_INACTIVE; > > return 1; > }
By simply setting the event state in here, we bypass time stamp stuff as a result. This might lead to inaccuracies...
Deng-Cheng
| |