Re: [PATCH] perf/x86/intel: Add proper condition to run sched_task callbacks

[Jiri Olsa]

On Tue, Jul 18, 2017 at 11:14:44AM +0200, Peter Zijlstra wrote:
> On Mon, Jul 17, 2017 at 05:01:56PM +0200, Jiri Olsa wrote:
> > The x86 pmu currently uses the sched_task callback for 2 functions:
> >   - PEBS drain
> >   - save/restore LBR data
> > 
> > They are both triggered once the x86 pmu is registered with
> > perf_sched_cb_inc call (within pmu::add	callback), regardless
> > if there's actually any PEBS or LBR event configured on the cpu.
> 
> I don't understand. If we do pmu::add() we _are_ on the CPU.
> 
> So you're saying intel_pmu_pebs_{add,del}() are doing it wrong? So why
> not fix those?
> 
> > diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> > index aa62437d1aa1..1f66356d8122 100644
> > --- a/arch/x86/events/intel/core.c
> > +++ b/arch/x86/events/intel/core.c
> > @@ -3265,9 +3265,11 @@ static void intel_pmu_cpu_dying(int cpu)
> >  static void intel_pmu_sched_task(struct perf_event_context *ctx,
> >  				 bool sched_in)
> >  {
> > -	if (x86_pmu.pebs_active)
> > +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> > +
> > +	if (intel_pmu_pebs_needs_sched_cb(cpuc))
> >  		intel_pmu_pebs_sched_task(ctx, sched_in);
> 
> So I'm confused, if we'd not need this, how come we're here in the first
> place?
> 

because we have 2 places using the same callback
  - PEBS drain for free running counters
  - LBR save/store

both of them called from intel_pmu_sched_task

so let's say PEBS drain setup the callback for the event,
but in the callback itself (intel_pmu_sched_task) we will
also run the code for LBR save/restore, which we did not
ask for, but the code in intel_pmu_sched_task does not
check for that

I'm adding conditions to recognize the work that needs
to be done in the callback

another option might be to add support for more x86_pmu::sched_task
callbacks, which might be cleaner

jirka