From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753545Ab0DAHFd (ORCPT ); Thu, 1 Apr 2010 03:05:33 -0400 Received: from mga11.intel.com ([192.55.52.93]:30819 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752774Ab0DAHF1 (ORCPT ); Thu, 1 Apr 2010 03:05:27 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.51,346,1267430400"; d="scan'208";a="785683674" Subject: Re: [PATCH 0/3] perf/core, x86: unify perfctr bitmasks From: Lin Ming To: Cyrill Gorcunov Cc: Peter Zijlstra , Robert Richter , Stephane Eranian , Ingo Molnar , LKML In-Reply-To: <1270088048.8575.38.camel@minggr.sh.intel.com> References: <20100330134145.GI11907@erda.amd.com> <1269961255.5258.221.camel@laptop> <20100330155949.GJ11907@erda.amd.com> <1269968113.5258.442.camel@laptop> <20100330182906.GD5211@lenovo> <1269975840.5258.609.camel@laptop> <20100331161523.GA9058@lenovo> <20100331162647.GA15288@lenovo> <20100331170517.GB15288@lenovo> <1270088048.8575.38.camel@minggr.sh.intel.com> Content-Type: text/plain; charset="UTF-8" Date: Thu, 01 Apr 2010 14:47:22 +0800 Message-Id: <1270104442.3456.11.camel@minggr.sh.intel.com> Mime-Version: 1.0 X-Mailer: Evolution 2.24.1 (2.24.1-2.fc10) Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2010-04-01 at 10:14 +0800, Lin Ming wrote: > On Thu, 2010-04-01 at 01:05 +0800, Cyrill Gorcunov wrote: > > On Wed, Mar 31, 2010 at 08:26:47PM +0400, Cyrill Gorcunov wrote: > > > On Wed, Mar 31, 2010 at 08:15:23PM +0400, Cyrill Gorcunov wrote: > > > > On Tue, Mar 30, 2010 at 09:04:00PM +0200, Peter Zijlstra wrote: > > > > > On Tue, 2010-03-30 at 22:29 +0400, Cyrill Gorcunov wrote: > > > > [...] > > > > > > > > > [...] > > > > +static inline bool p4_is_odd_cpl(u32 escr) > > > > +{ > > > > + unsigned int t0 = (escr & P4_ESCR_T0_ANY) << 0; > > > > + unsigned int t1 = (escr & P4_ESCR_T1_ANY) << 2; > > > > + > > > > + if ((t0 ^ t1) != t0) > > > > + return true; > > > > > > /me in shame: This is bogus, Peter don't take it yet. > > > > > > > Updated > > > > -- Cyrill > > --- > > x86, perf: P4 PMU -- check for permission granted on ANY event v2 > > > > In case if a caller (user) asked us to count events with > > some weird mask we should check if this priviledge has been > > granted since this could be a mix of bitmasks we not like > > which but allow if caller insist. > > > > By ANY event term the combination of USR/OS bits in ESCR > > register is assumed. > > I'll test this patch. > Does it need to be applied on top of Robert's patch? I tested this patch on top of Peter's and Robert's patch. Works well on P4. Only some build error, fixed by below patch. In file included from arch/x86/kernel/cpu/perf_event.c:1326: arch/x86/kernel/cpu/perf_event_p6.c:94: error: ‘x86_hw_config’ undeclared here (not in a function) arch/x86/kernel/cpu/perf_event_p6.c:99: error: unknown field ‘raw_event’ specified in initializer arch/x86/kernel/cpu/perf_event_p6.c:99: error: ‘x86_pmu_raw_event’ undeclared here (not in a function) make[3]: *** [arch/x86/kernel/cpu/perf_event.o] Error 1 diff --git a/arch/x86/kernel/cpu/perf_event_p6.c b/arch/x86/kernel/cpu/perf_event_p6.c index 626abc0..4ec9680 100644 --- a/arch/x86/kernel/cpu/perf_event_p6.c +++ b/arch/x86/kernel/cpu/perf_event_p6.c @@ -91,12 +91,11 @@ static __initconst struct x86_pmu p6_pmu = { .enable_all = p6_pmu_enable_all, .enable = p6_pmu_enable_event, .disable = p6_pmu_disable_event, - .hw_config = x86_hw_config, + .hw_config = x86_pmu_hw_config, .schedule_events = x86_schedule_events, .eventsel = MSR_P6_EVNTSEL0, .perfctr = MSR_P6_PERFCTR0, .event_map = p6_pmu_event_map, - .raw_event = x86_pmu_raw_event, .max_events = ARRAY_SIZE(p6_perfmon_event_map), .apic = 1, .max_period = (1ULL << 31) - 1, Lin Ming > > Lin Ming > > > > > CC: Peter Zijlstra > > Signed-off-by: Cyrill Gorcunov > > --- > > arch/x86/include/asm/perf_event_p4.h | 17 +++++++++++++++++ > > arch/x86/kernel/cpu/perf_event_p4.c | 24 +++++++++++++++++++++--- > > 2 files changed, 38 insertions(+), 3 deletions(-) > > > > Index: linux-2.6.git/arch/x86/include/asm/perf_event_p4.h > > ===================================================================== > > --- linux-2.6.git.orig/arch/x86/include/asm/perf_event_p4.h > > +++ linux-2.6.git/arch/x86/include/asm/perf_event_p4.h > > @@ -33,6 +33,9 @@ > > #define P4_ESCR_T1_OS 0x00000002U > > #define P4_ESCR_T1_USR 0x00000001U > > > > +#define P4_ESCR_T0_ANY (P4_ESCR_T0_OS | P4_ESCR_T0_USR) > > +#define P4_ESCR_T1_ANY (P4_ESCR_T1_OS | P4_ESCR_T1_USR) > > + > > #define P4_ESCR_EVENT(v) ((v) << P4_ESCR_EVENT_SHIFT) > > #define P4_ESCR_EMASK(v) ((v) << P4_ESCR_EVENTMASK_SHIFT) > > #define P4_ESCR_TAG(v) ((v) << P4_ESCR_TAG_SHIFT) > > @@ -134,6 +137,20 @@ > > #define P4_CONFIG_HT_SHIFT 63 > > #define P4_CONFIG_HT (1ULL << P4_CONFIG_HT_SHIFT) > > > > +/* > > + * typically we set USR or/and OS bits for one of the > > + * threads only at once, any other option is treated > > + * as "any" > > + */ > > +static inline bool p4_is_any_cpl(u32 escr) > > +{ > > + if ((escr & P4_ESCR_T0_ANY) && > > + (escr & P4_ESCR_T1_ANY)) > > + return true; > > + > > + return false; > > +} > > + > > static inline bool p4_is_event_cascaded(u64 config) > > { > > u32 cccr = p4_config_unpack_cccr(config); > > Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c > > ===================================================================== > > --- linux-2.6.git.orig/arch/x86/kernel/cpu/perf_event_p4.c > > +++ linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c > > @@ -443,13 +443,18 @@ static int p4_hw_config(struct perf_even > > return 0; > > > > /* > > + * a caller may ask for something definitely weird and > > + * screwed, sigh... > > + */ > > + escr = p4_config_unpack_escr(event->attr.config); > > + if (p4_is_any_cpl(escr) && perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN)) > > + return -EACCES; > > + > > + /* > > * We don't control raw events so it's up to the caller > > * to pass sane values (and we don't count the thread number > > * on HT machine but allow HT-compatible specifics to be > > * passed on) > > - * > > - * XXX: HT wide things should check perf_paranoid_cpu() && > > - * CAP_SYS_ADMIN > > */ > > event->hw.config |= event->attr.config & > > (p4_config_pack_escr(P4_ESCR_MASK_HT) | > > @@ -630,6 +635,19 @@ static void p4_pmu_swap_config_ts(struct > > escr = p4_config_unpack_escr(hwc->config); > > cccr = p4_config_unpack_cccr(hwc->config); > > > > + /* > > + * for non-standart configs we don't clobber cpl > > + * related bits so it's preferred the caller don't > > + * use this mode > > + */ > > + if (unlikely(p4_is_any_cpl(escr))) { > > + if (p4_ht_thread(cpu)) > > + hwc->config |= P4_CONFIG_HT; > > + else > > + hwc->config &= ~P4_CONFIG_HT; > > + return; > > + } > > + > > if (p4_ht_thread(cpu)) { > > cccr &= ~P4_CCCR_OVF_PMI_T0; > > cccr |= P4_CCCR_OVF_PMI_T1;