Re: [RFC][PATCH 1/7] perf/x86/intel: Rework the large PEBS setup code

[Peter Zijlstra <peterz@infradead.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);
>  }