Messages in this thread | | | Subject | RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec | From | Peter Zijlstra <> | Date | Tue, 22 Nov 2011 14:45:59 +0100 |
| |
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().
> > The below is a somewhat larger patch that avoids the recursion (and does > > a small cleanup by eradicating all those useless ctx arguments). Quick > > testing seems to indicate it works, but please confirm. > > I have no objection of deleting the redundant ctx arguments, but that's > another topic.
Yeah, I should probably split that into a separate patch.
| |