From patchwork Tue Mar 10 14:05:23 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Suzuki K Poulose X-Patchwork-Id: 5978001 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id A9A599F399 for ; Tue, 10 Mar 2015 14:08:58 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D906E2021F for ; Tue, 10 Mar 2015 14:08:54 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 10633201F5 for ; Tue, 10 Mar 2015 14:08:54 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1YVKnI-0002xw-4s; Tue, 10 Mar 2015 14:06:00 +0000 Received: from service87.mimecast.com ([91.220.42.44]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1YVKnA-0002mH-9E for linux-arm-kernel@lists.infradead.org; Tue, 10 Mar 2015 14:05:57 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by service87.mimecast.com; Tue, 10 Mar 2015 14:05:27 +0000 Received: from [10.1.209.22] ([10.1.255.212]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Tue, 10 Mar 2015 14:05:24 +0000 Message-ID: <54FEFA23.7050307@arm.com> Date: Tue, 10 Mar 2015 14:05:23 +0000 From: "Suzuki K. Poulose" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Peter Zijlstra , Mark Rutland Subject: Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs References: <1425905192-10509-1-git-send-email-suzuki.poulose@arm.com> <1425905192-10509-2-git-send-email-suzuki.poulose@arm.com> <20150310112723.GY2896@worktop.programming.kicks-ass.net> <20150310120521.GD28168@leverpostej> <20150310125351.GD2896@worktop.programming.kicks-ass.net> <20150310130041.GC11574@worktop.ger.corp.intel.com> In-Reply-To: <20150310130041.GC11574@worktop.ger.corp.intel.com> X-OriginalArrivalTime: 10 Mar 2015 14:05:24.0491 (UTC) FILETIME=[40F795B0:01D05B3B] X-MC-Unique: 115031014052700801 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150310_070552_667039_3A13982D X-CRM114-Status: GOOD ( 13.28 ) X-Spam-Score: -2.3 (--) Cc: "linux@arm.linux.org.uk" , Pawel Moll , Punit Agrawal , Will Deacon , "linux-kernel@vger.kernel.org" , "acme@kernel.org" , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 10/03/15 13:00, Peter Zijlstra wrote: > On Tue, Mar 10, 2015 at 01:53:51PM +0100, Peter Zijlstra wrote: >>> It would be nicer if we could prevent this in the core so we're not >>> reliant on every PMU driver doing the same verification. My initial >>> thought was that seemed like unnecessary duplication of the ctx checking >>> above, but if we're going to end up shoving it into several drivers >>> anyway perhaps it's the lesser evil. >> >> Again, agreed, that would be better and less error prone. But I'm not >> entirely sure how to go about doing it :/ I'll have to go think about >> that; and conferences are not the best place for that. >> >> Suggestions on that are welcome of course ;) > > So the problem is that event_init() is what will return the pmu, so we > cannot make decisions on it until after that returns. > > Maybe we can pull out the validate step into its own funciton; > pmu->validate() or whatnot, to be called slightly later. > I think we could still solve this problem by deferring the 'context' validation to the core. The PMUs could validate the group, within its context. i.e, if it can accommodate its events as a group, during event_init. The problem we face now, is encountering an event from a different PMU, which we could leave it to the core as we do already. i.e the fix could look like (and similarly for other cases): Thoughts ? Suzuki diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c index 557e128..b3af19b 100644 --- a/arch/arm/kernel/perf_event.c +++ b/arch/arm/kernel/perf_event.c @@ -259,20 +259,28 @@ out: } static int -validate_event(struct pmu_hw_events *hw_events, - struct perf_event *event) +validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events, + struct perf_event *event) { - struct arm_pmu *armpmu = to_arm_pmu(event->pmu); + struct arm_pmu *armpmu; if (is_software_event(event)) return 1; + /* + * We are only worried if we can accommodate the events + * from this pmu in this group. + */ + if (event->pmu != pmu) + return 1; + if (event->state < PERF_EVENT_STATE_OFF) return 1; if (event->state == PERF_EVENT_STATE_OFF && !event->attr.enable_on_exec) return 1; + armpmu = to_arm_pmu(event->pmu); return armpmu->get_event_idx(hw_events, event) >= 0; } @@ -288,15 +296,15 @@ validate_group(struct perf_event *event) */ memset(&fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask)); - if (!validate_event(&fake_pmu, leader)) + if (!validate_event(event->pmu, &fake_pmu, leader)) return -EINVAL; list_for_each_entry(sibling, &leader->sibling_list, group_entry) { - if (!validate_event(&fake_pmu, sibling)) + if (!validate_event(event->pmu, &fake_pmu, sibling)) return -EINVAL; } - if (!validate_event(&fake_pmu, event)) + if (!validate_event(event->pmu, &fake_pmu, event)) return -EINVAL; return 0;