From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1756365AbcGHWBc (ORCPT <rfc822;w@1wt.eu>);
	Fri, 8 Jul 2016 18:01:32 -0400
Received: from merlin.infradead.org ([205.233.59.134]:37918 "EHLO
	merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1756064AbcGHWB0 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 8 Jul 2016 18:01:26 -0400
Date: Sat, 9 Jul 2016 00:00:47 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Jiri Olsa <jolsa@redhat.com>
Cc: mingo@kernel.org, acme@kernel.org, linux-kernel@vger.kernel.org,
        andi@firstfloor.org, eranian@google.com, jolsa@kernel.org,
        torvalds@linux-foundation.org, davidcc@google.com,
        alexander.shishkin@linux.intel.com, namhyung@kernel.org,
        kan.liang@intel.com, khandual@linux.vnet.ibm.com
Subject: Re: [RFC][PATCH 1/7] perf/x86/intel: Rework the large PEBS setup code
Message-ID: <20160708220047.GK30909@twins.programming.kicks-ass.net>
References: <20160708133059.031522978@infradead.org>
 <20160708134113.290879090@infradead.org>
 <20160708163616.GA2392@krava>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160708163616.GA2392@krava>
User-Agent: Mutt/1.5.23.1 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Jul 08, 2016 at 06:36:16PM +0200, Jiri Olsa wrote:
> On Fri, Jul 08, 2016 at 03:31:00PM +0200, Peter Zijlstra wrote:
> 
> SNIP
> 
> >  	/*
> > -	 * When the event is constrained enough we can use a larger
> > -	 * threshold and run the event with less frequent PMI.
> > +	 * Use auto-reload if possible to save a MSR write in the PMI.
> > +	 * This must be done in pmu::start(), because PERF_EVENT_IOC_PERIOD.
> >  	 */
> > -	if (hwc->flags & PERF_X86_EVENT_FREERUNNING) {
> > -		threshold = ds->pebs_absolute_maximum -
> > -			x86_pmu.max_pebs_events * x86_pmu.pebs_record_size;
> > -
> > -		if (first_pebs)
> > -			perf_sched_cb_inc(event->ctx->pmu);
> > -	} else {
> > -		threshold = ds->pebs_buffer_base + x86_pmu.pebs_record_size;
> > -
> > -		/*
> > -		 * If not all events can use larger buffer,
> > -		 * roll back to threshold = 1
> > -		 */
> > -		if (!first_pebs &&
> > -		    (ds->pebs_interrupt_threshold > threshold))
> > -			perf_sched_cb_dec(event->ctx->pmu);
> > -	}
> 
> hum, the original code switched back the perf_sched_cb,
> in case !feerunning event was detected.. I dont see it
> in the new code.. just the threshold update

> +static inline bool pebs_needs_sched_cb(struct cpu_hw_events *cpuc)
>  {
> +	return cpuc->n_pebs && (cpuc->n_pebs == cpuc->n_large_pebs);
> +}

> +static void intel_pmu_pebs_add(struct perf_event *event)
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +	struct hw_perf_event *hwc = &event->hw;
> +	bool needs_cb = pebs_needs_sched_cb(cpuc);
> +
> +	cpuc->n_pebs++;
> +	if (hwc->flags & PERF_X86_EVENT_FREERUNNING)
> +		cpuc->n_large_pebs++;
> +
> +	if (!needs_cb && pebs_needs_sched_cb(cpuc))
> +		perf_sched_cb_inc(event->ctx->pmu);

Ah, you're saying this,

> +	pebs_update_threshold(cpuc);
>  }

> +static void intel_pmu_pebs_del(struct perf_event *event)
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +	struct hw_perf_event *hwc = &event->hw;
> +	bool needs_cb = pebs_needs_sched_cb(cpuc);
> +
> +	cpuc->n_pebs--;
> +	if (hwc->flags & PERF_X86_EVENT_FREERUNNING)
> +		cpuc->n_large_pebs--;
> +
> +	if (needs_cb && !pebs_needs_sched_cb(cpuc))
> +		perf_sched_cb_dec(event->ctx->pmu);

and this, should also have something like

	if (!needs_cb && pebs_needs_sched_cb(cpuc))
		perf_sched_cb_inc(event->ctx->pmu)

Because the event we just removed was the one inhibiting FREERUNNING and
we can now let it rip again.

Yes, you're right. Let me try and see if I can make that better.

Thanks!

> +
> +	if (cpuc->n_pebs)
> +		pebs_update_threshold(cpuc);
>  }