Re: [PATCH V3] perf/x86/intel/ds: Flush the PEBS buffer in PEBS enable

[Peter Zijlstra]

On Mon, Apr 10, 2023 at 11:13:09AM -0700, kan.liang@xxxxxxxxxxxxxxx wrote:

>  arch/x86/events/intel/ds.c | 39 ++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 3a77f4336df7..4639d4c1e98d 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1257,20 +1257,18 @@ pebs_update_state(bool needed_cb, struct cpu_hw_events *cpuc,
>  	if (x86_pmu.intel_cap.pebs_baseline && add) {
>  		u64 pebs_data_cfg;
>  
> -		/* Clear pebs_data_cfg and pebs_record_size for first PEBS. */
> -		if (cpuc->n_pebs == 1) {
> +		/* Clear pebs_data_cfg for first PEBS. */
> +		if (cpuc->n_pebs == 1)
>  			cpuc->pebs_data_cfg = 0;
> -			cpuc->pebs_record_size = sizeof(struct pebs_basic);
> -		}
>  
>  		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) {
> +		/*
> +		 * Only update the pebs_data_cfg here. The pebs_record_size
> +		 * will be updated later when the new pebs_data_cfg takes effect.
> +		 */
> +		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);

Now, pebs_update_threshold() will actually use
->pebs_record_size, but afaict the above now has a path through (for
example for the first event) where update is true but ->pebs_record_size
is unset/stale.

I think it all works out, but it is quite a mess and hard to follow.

> @@ -1331,6 +1329,13 @@ static void intel_pmu_pebs_via_pt_enable(struct perf_event *event)
>  	wrmsrl(base + idx, value);
>  }
>  
> +static inline void intel_pmu_drain_large_pebs(struct cpu_hw_events *cpuc)
> +{
> +	if (cpuc->n_pebs == cpuc->n_large_pebs &&
> +	    cpuc->n_pebs != cpuc->n_pebs_via_pt)
> +		intel_pmu_drain_pebs_buffer();
> +}

Its been a minute since I looked at this code; but why only for large
pebs? Surely flushing is quick when the DS is actually empty and that
stops us having to worry if there's races where there might be a single
entry in.

>  void intel_pmu_pebs_enable(struct perf_event *event)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> @@ -1350,6 +1355,18 @@ void intel_pmu_pebs_enable(struct perf_event *event)
>  	if (x86_pmu.intel_cap.pebs_baseline) {
>  		hwc->config |= ICL_EVENTSEL_ADAPTIVE;
>  		if (cpuc->pebs_data_cfg != cpuc->active_pebs_data_cfg) {
> +			/*
> +			 * A system-wide PEBS event with the large PEBS
> +			 * config may still be enabled when switching the
> +			 * context. Some PEBS records for the system-wide
> +			 * PEBS may be generated while the old event has
> +			 * been scheduled out but the new one hasn't been
> +			 * scheduled in. It's not enough to only flush the
> +			 * buffer when a PEBS event is disable.
> +			 */

Perhaps just:

			/*
			 * drain_pebs() assumes uniform record size;
			 * hence we need to drain when changing said
			 * size.
			 */


> +			intel_pmu_drain_large_pebs(cpuc);
> +			adaptive_pebs_record_size_update();
> +			pebs_update_threshold(cpuc);
>  			wrmsrl(MSR_PEBS_DATA_CFG, cpuc->pebs_data_cfg);
>  			cpuc->active_pebs_data_cfg = cpuc->pebs_data_cfg;
>  		}