Re: [PATCH 03/22] perf/x86/intel: Support adaptive PEBSv4

[Liang, Kan]

On 3/19/2019 10:47 AM, Peter Zijlstra wrote:
> > @@ -933,6 +998,19 @@ pebs_update_state(bool needed_cb, struct cpu_hw_events *cpuc, struct pmu *pmu)
> >   		update = true;
> >   	}
> >   
> > +	if (x86_pmu.intel_cap.pebs_baseline && add) {
> > +		u64 pebs_data_cfg;
> > +
> > +		pebs_data_cfg = pebs_update_adaptive_cfg(event);
> > +
> > +		/* Update pebs_record_size if new event requires more data. */
> > +		if (pebs_data_cfg & ~cpuc->pebs_data_cfg) {
> > +			cpuc->pebs_data_cfg |= pebs_data_cfg;
> > +			adaptive_pebs_record_size_update();
> > +			update = true;
> > +		}
> > +	}
> > +
> >   	if (update)
> >   		pebs_update_threshold(cpuc);
> >   }
> Hurmph.. this only grows the PEBS record.

Yes, the PEBS record doesn't shrink on the del. Because we have to go 
through all the existing pebs events for an accurate config. I think it 
doesn't worth it. There is no harmful for a bigger PEBS record, except 
little performance impacts. But that's rare case. For most cases, we 
usually apply the same pebs config for all pebs events.

> > @@ -947,7 +1025,7 @@ void intel_pmu_pebs_add(struct perf_event *event)
> >   	if (hwc->flags & PERF_X86_EVENT_LARGE_PEBS)
> >   		cpuc->n_large_pebs++;
> >   
> > -	pebs_update_state(needed_cb, cpuc, event->ctx->pmu);
> > +	pebs_update_state(needed_cb, cpuc, event, true);
> >   }
> >   
> >   void intel_pmu_pebs_enable(struct perf_event *event)
> > @@ -965,6 +1043,14 @@ void intel_pmu_pebs_enable(struct perf_event *event)
> >   	else if (event->hw.flags & PERF_X86_EVENT_PEBS_ST)
> >   		cpuc->pebs_enabled |= 1ULL << 63;
> >   
> > +	if (x86_pmu.intel_cap.pebs_baseline) {
> > +		hwc->config |= ICL_EVENTSEL_ADAPTIVE;
> > +		if (cpuc->pebs_data_cfg != cpuc->active_pebs_data_cfg) {
> > +			wrmsrl(MSR_PEBS_DATA_CFG, cpuc->pebs_data_cfg);
> > +			cpuc->active_pebs_data_cfg = cpuc->pebs_data_cfg;
> > +		}
> > +	}
> > +
> >   	/*
> >   	 * 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.
> > @@ -991,7 +1077,12 @@ void intel_pmu_pebs_del(struct perf_event *event)
> >   	if (hwc->flags & PERF_X86_EVENT_LARGE_PEBS)
> >   		cpuc->n_large_pebs--;
> >   
> > -	pebs_update_state(needed_cb, cpuc, event->ctx->pmu);
> > +	/* Clear both pebs_data_cfg and pebs_record_size for first PEBS. */
> Weird comment..
>
> > +	if (x86_pmu.intel_cap.pebs_baseline && !cpuc->n_pebs) {
> > +		cpuc->pebs_data_cfg = 0;
> > +		cpuc->pebs_record_size = sizeof(struct pebs_basic);
> > +	}
> > +	pebs_update_state(needed_cb, cpuc, event, false);
> Why do we have to reset record_size? That'll be updated in
> pebs_update_state() on the next add.

The record_size should be reset for the first PEBS events.
Right, I can move the reset in pebs_update_state().


Thanks,
Kan