From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759694AbdEVQ4T convert rfc822-to-8bit (ORCPT ); Mon, 22 May 2017 12:56:19 -0400 Received: from mga02.intel.com ([134.134.136.20]:27116 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758581AbdEVQ4M (ORCPT ); Mon, 22 May 2017 12:56:12 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,377,1491289200"; d="scan'208";a="971713469" From: "Liang, Kan" To: Peter Zijlstra CC: "mingo@redhat.com" , "eranian@google.com" , "linux-kernel@vger.kernel.org" , "alexander.shishkin@linux.intel.com" , "acme@redhat.com" , "jolsa@redhat.com" , "torvalds@linux-foundation.org" , "tglx@linutronix.de" , "vincent.weaver@maine.edu" , "ak@linux.intel.com" Subject: RE: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Thread-Topic: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Thread-Index: AQHS0MiHfyjRMknVHkW8awSHhRlbV6H/kUoAgAEBC1A= Date: Mon, 22 May 2017 16:55:47 +0000 Message-ID: <37D7C6CF3E00A74B8858931C1DB2F077536F079F@SHSMSX103.ccr.corp.intel.com> References: <1495213582-3635-1-git-send-email-kan.liang@intel.com> <20170522091916.3gydvflk4fnqkzw5@hirez.programming.kicks-ass.net> In-Reply-To: <20170522091916.3gydvflk4fnqkzw5@hirez.programming.kicks-ass.net> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMmY2NWRmZTUtYWQ0My00ZDE0LWI0NGYtYmJiYjIxYTQ1OGMyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IkxBZUN1NWRzYXIyc2FCendkaWVkcEdsUERLUk9EZnpYTmlyVGxNOGJOUVE9In0= x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote: > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index > > 580b60f..e8b2326 100644 > > --- a/arch/x86/events/core.c > > +++ b/arch/x86/events/core.c > > @@ -101,6 +101,10 @@ u64 x86_perf_event_update(struct perf_event > *event) > > delta = (new_raw_count << shift) - (prev_raw_count << shift); > > delta >>= shift; > > > > + /* Correct the count number if applying ref_cycles replacement */ > > + if (!is_sampling_event(event) && > > + (hwc->flags & PERF_X86_EVENT_REF_CYCLES_REP)) > > + delta *= x86_pmu.ref_cycles_factor; > > That condition seems wrong, why only correct for !sampling events? > For sampling, it's either fixed freq mode or fixed period mode. - In the fixed freq mode, we should do nothing, because the adaptive frequency algorithm will handle it. - In the fixed period mode, we have already adjusted the period in ref_cycles_rep(). Therefore, we should only handle !sampling events here. > > local64_add(delta, &event->count); > > local64_sub(delta, &hwc->period_left); > > > > > > @@ -934,6 +938,21 @@ int x86_schedule_events(struct cpu_hw_events > *cpuc, int n, int *assign) > > for (i = 0; i < n; i++) { > > e = cpuc->event_list[i]; > > e->hw.flags |= PERF_X86_EVENT_COMMITTED; > > + > > + /* > > + * 0x0300 is pseudo-encoding for REF_CPU_CYCLES. > > + * It indicates that fixed counter 2 should be used. > > + * > > + * If fixed counter 2 is occupied and a GP counter > > + * is assigned, an alternative event which can be > > + * counted in GP counter will be used to replace > > + * the pseudo-encoding REF_CPU_CYCLES event. > > + */ > > + if (((e->hw.config & X86_RAW_EVENT_MASK) == > 0x0300) && > > + (assign[i] < INTEL_PMC_IDX_FIXED) && > > + x86_pmu.ref_cycles_rep) > > + x86_pmu.ref_cycles_rep(e); > > + > > if (x86_pmu.commit_scheduling) > > x86_pmu.commit_scheduling(cpuc, i, > assign[i]); > > } > > This looks dodgy, this is the branch were we managed to schedule all events. > Why would we need to consider anything here? > > I was expecting a retry if there are still unscheduled events and one of the > events was our 0x0300 event. In that case you have to reset the event and > retry the whole scheduling thing. Will do it. Thanks, Kan