From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755501Ab1KVNqO (ORCPT ); Tue, 22 Nov 2011 08:46:14 -0500 Received: from casper.infradead.org ([85.118.1.10]:34600 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752608Ab1KVNqN convert rfc822-to-8bit (ORCPT ); Tue, 22 Nov 2011 08:46:13 -0500 Message-ID: <1321969559.14799.7.camel@twins> Subject: RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec From: Peter Zijlstra To: "Zhu, DengCheng" Cc: "Barzilay, Eyal" , "Fortuna, Zenon" , Paul Mackerras , Ingo Molnar , Arnaldo Carvalho de Melo , "ralf@linux-mips.org" , LKML Date: Tue, 22 Nov 2011 14:45:59 +0100 In-Reply-To: References: <1321932601-21128-1-git-send-email-dczhu@mips.com> ,<1321959115.5148.22.camel@twins> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT X-Mailer: Evolution 3.2.1- Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.