Received-SPF: pass (google.com: domain of linux-kernel+bounces-126689-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: kan.liang@linux.intel.com
To: peterz@infradead.org,
	mingo@kernel.org,
	linux-kernel@vger.kernel.org
Cc: acme@kernel.org,
	namhyung@kernel.org,
	eranian@google.com,
	ak@linux.intel.com,
	Kan Liang <kan.liang@linux.intel.com>,
	stable@vger.kernel.org
Subject: [PATCH] perf/x86/intel/ds: Don't clear the pebs_data_cfg for the last PEBS event
Date: Mon,  1 Apr 2024 06:33:20 -0700
Message-Id: <20240401133320.703971-1-kan.liang@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

From: Kan Liang <kan.liang@linux.intel.com>

The MSR_PEBS_DATA_CFG is to configure which data groups should be
generated into a PEBS record. It's shared among counters. If there are
different configurations among counters, perf combines all the
configurations.

The first perf command as below requires a complete PEBS record
(including memory info, GPRs, XMMs, and LBRs). The second perf command
only requires a basic group. However, after the second perf command is
running, the MSR_PEBS_DATA_CFG is cleared. Only a basic group is
generated in a PEBS record, which is wrong. The required information
for the first perf command is missed.

 $perf record --intr-regs=AX,SP,XMM0 -a -C 8 -b -W -d -c 100000003
  -o /dev/null -e cpu/event=0xd0,umask=0x81/upp &
 $sleep 5
 $perf record  --per-thread  -c 1  -e cycles:pp --no-timestamp --no-tid
  taskset -c 8 ./noploop 1000

The first PEBS event is a system-wide PEBS event. The second PEBS event
is a per-thread event. When the thread is scheduled out, the
intel_pmu_pebs_del() is invoked to update the PEBS state. Since the
system-wide event is still available, the cpuc->n_pebs is 1. The
cpuc->pebs_data_cfg is cleared. The data configuration for the
system-wide PEBS event is lost.

The (cpuc->n_pebs == 1) was introduced in the commit b6a32f023fcc
("perf/x86: Fix PEBS threshold initialization"). At that time, it indeed
didn't hurt whether the state was updated during the removal. Because
only the threshold is updated. The calculation of the threshold takes
the last PEBS event into account. However, the commit b752ea0c28e3
("perf/x86/intel/ds: Flush PEBS DS when changing PEBS_DATA_CFG") delay
the threshold update, and clears the PEBS data config, which brings the
issue.

The PEBS data config update should not be shrink in the removal.

Fixes: b752ea0c28e3 ("perf/x86/intel/ds: Flush PEBS DS when changing PEBS_DATA_CFG")
Reported-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Cc: stable@vger.kernel.org
---
 arch/x86/events/intel/ds.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index 2641ba620f12..20ddfed3e721 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -1237,11 +1237,11 @@ pebs_update_state(bool needed_cb, struct cpu_hw_events *cpuc,
 	struct pmu *pmu = event->pmu;
 
 	/*
-	 * Make sure we get updated with the first PEBS
-	 * event. It will trigger also during removal, but
-	 * that does not hurt:
+	 * Make sure we get updated with the first PEBS event.
+	 * During removal, the pebs_data_cfg is still valid for
+	 * the last PEBS event. Don't clear it.
 	 */
-	if (cpuc->n_pebs == 1)
+	if ((cpuc->n_pebs == 1) && add)
 		cpuc->pebs_data_cfg = PEBS_UPDATE_DS_SW;
 
 	if (needed_cb != pebs_needs_sched_cb(cpuc)) {
-- 
2.35.1