From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757920AbdEVJT3 (ORCPT ); Mon, 22 May 2017 05:19:29 -0400 Received: from merlin.infradead.org ([205.233.59.134]:46158 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757890AbdEVJT1 (ORCPT ); Mon, 22 May 2017 05:19:27 -0400 Date: Mon, 22 May 2017 11:19:16 +0200 From: Peter Zijlstra To: kan.liang@intel.com 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 Message-ID: <20170522091916.3gydvflk4fnqkzw5@hirez.programming.kicks-ass.net> References: <1495213582-3635-1-git-send-email-kan.liang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1495213582-3635-1-git-send-email-kan.liang@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) 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? > 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.