From: "Liang, Kan" <kan.liang@intel.com>
To: Jiri Olsa <jolsa@redhat.com>
CC: "peterz@infradead.org" <peterz@infradead.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "eranian@google.com" <eranian@google.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "alexander.shishkin@linux.intel.com" 
        <alexander.shishkin@linux.intel.com>,
        "acme@redhat.com" <acme@redhat.com>,
        "torvalds@linux-foundation.org" <torvalds@linux-foundation.org>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "vincent.weaver@maine.edu" <vincent.weaver@maine.edu>,
        "ak@linux.intel.com" <ak@linux.intel.com>
Subject: RE: [PATCH V2 1/2] perf/x86/intel: enable CPU ref_cycles for GP
 counter
Thread-Topic: [PATCH V2 1/2] perf/x86/intel: enable CPU ref_cycles for GP
 counter
Thread-Index: AQHS4U1VW/Bjj/rXMEe9XMQK1mskEKIghsuAgACsArA=
Date: Mon, 12 Jun 2017 13:07:35 +0000
Message-ID: <37D7C6CF3E00A74B8858931C1DB2F0775370B837@SHSMSX103.ccr.corp.intel.com>
References: <1497029283-3332-1-git-send-email-kan.liang@intel.com>
 <20170612103631.GA6099@krava>
In-Reply-To: <20170612103631.GA6099@krava>
Accept-Language: zh-CN, en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 10.0.102.7
dlp-reaction: no-action
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2466
Lines: 57

> 
> On Fri, Jun 09, 2017 at 10:28:02AM -0700, kan.liang@intel.com wrote:
> 
> SNIP
> 
> > +	/*
> > +	 * Correct the count number if applying ref_cycles replacement.
> > +	 * There is a constant ratio between ref_cycles (event A) and
> > +	 * ref_cycles replacement (event B). The delta result is from event B.
> > +	 * To get accurate event A count, the real delta result must be
> > +	 * multiplied with the constant ratio.
> > +	 *
> > +	 * It is handled differently for sampling and counting.
> > +	 * - Fixed period Sampling: The period is already adjusted in
> > +	 *   scheduling for event B. The period_left is the remaining period
> > +	 *   for event B. Don't need to modify period_left.
> > +	 *   It's enough to only adjust event->count here.
> > +	 *
> > +	 * - Fixed freq Sampling: The adaptive frequency algorithm needs
> > +	 *   the last_period and event counts for event A to estimate the next
> > +	 *   period for event A. So the period_left is the remaining period
> > +	 *   for event A. It needs to multiply the result with the ratio.
> > +	 *   However, the period_left is also used to reload the event counter
> > +	 *   for event B in x86_perf_event_set_period. It has to be adjusted to
> > +	 *   event B's remaining period there.
> > +	 *
> > +	 * - Counting: It's enough to only adjust event->count for perf stat.
> > +	 *
> > +	 * - RDPMC: User can also read the counter directly by rdpmc/mmap.
> > +	 *   Users have to handle the adjustment themselves.
> > +	 *   For kernel, it only needs to guarantee that the offset is correct.
> > +	 *   In x86's arch_perf_update_userpage, the offset will be corrected
> > +	 *   if event B is used.
> > +	 */
> > +	adjust_delta = delta;
> > +	if (hwc->flags & PERF_X86_EVENT_REF_CYCLES_REP) {
> > +		adjust_delta = delta * x86_pmu.ref_cycles_factor;
> > +
> > +		if (is_sampling_event(event) && event->attr.freq)
> > +			local64_sub(adjust_delta, &hwc->period_left);
> > +		else
> > +			local64_sub(delta, &hwc->period_left);
> 
> shouldn't you use adjust_delta in here also ^^^ ?

No, we shouldn't use adjust_delta for all cases. Only fixed freq sampling is
enough.

For fixed period sampling, we already adjust the period in scheduling.
So the period_left here is already adjusted. We should not do it twice.
For RDPMC, user will handle the adjustment. The kernel should not do
any changes here.
For counting (perf stat), it doesn't matter. We only care about the
event->count.
   
Thanks,
Kan