x86 perf bug fixes and optimization

Quoted 5 lines

arch/x86/events/intel/ds.c +4 -0

 		event = cpuc->events[bit];
+		/*
+		 * perf_event_overflow() called by below __intel_pmu_pebs_last_event()
+		 * could trigger interrupt throttle and clear all event pointers of the
+		 * group in cpuc->events[] to NULL. So need to re-check if cpuc->events[*]

arch/x86/events/intel/ds.c static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d · line 2675

Quoted 6 lines

arch/x86/events/intel/ds.c +6 -0

+		 * is NULL, if so it indicates the event has been throttled (stopped) and
+		 * the corresponding last PEBS records have been processed in stopping
+		 * event, don't need to process it again.
+		 */
+		if (!event)
+			continue;

Then we silently ignore the overflow. Would be better to log at least an overflow packet or something like that.

-Andi

Andi, I didn't fully get the exact meaning about the "log" here. When throttle is triggered, perf_event_throttle() has already called perf_log_throttle() to log the throttle event although only for the group leader. Is it enough?

Quoted 4 lines

Andi, I didn't fully get the exact meaning about the "log" here. When throttle is triggered, perf_event_throttle() has already called perf_log_throttle() to log the throttle event although only for the group leader. Is it enough?

Throttle normally doesn't involve data loss, just less samples. But this is data loss, so it's an overflow.

-Andi

Quoted 4 lines · Andi Kleen

Quoted 4 lines

Andi, I didn't fully get the exact meaning about the "log" here. When throttle is triggered, perf_event_throttle() has already called perf_log_throttle() to log the throttle event although only for the group leader. Is it enough?

Quoted 2 lines

Throttle normally doesn't involve data loss, just less samples. But this is data loss, so it's an overflow.

IIUC, there should be no data loss, the unprocessed PEBS records of these throttled events would be still processed eventually by calling intel_pmu_drain_pebs_buffer() when stopping the event.

When intel_pmu_drain_pebs_icl() is called to drain PEBS records, the perf_event_overflow() could be called to process the last PEBS record.

While perf_event_overflow() could trigger the interrupt throttle and stop all events of the group, like what the below call-chain shows.

perf_event_overflow()
-> __perf_event_overflow()

    ->__perf_event_account_interrupt()
      -> perf_event_throttle_group()
        -> perf_event_throttle()
          -> event->pmu->stop()
            -> x86_pmu_stop()

The side effect of stopping the events is that all corresponding event pointers in cpuc->events[] array are cleared to NULL.

Assume there are two PEBS events (event a and event b) in a group. When intel_pmu_drain_pebs_icl() calls perf_event_overflow() to process the last PEBS record of PEBS event a, interrupt throttle is triggered and all pointers of event a and event b are cleared to NULL. Then intel_pmu_drain_pebs_icl() tries to process the last PEBS record of event b and encounters NULL pointer access.

Since the left PEBS records have been processed when stopping the event, check and skip to process the last PEBS record if cpuc->events[*] is NULL.

Reported-by kernel test robot <[email protected]>

Closes https://lore.kernel.org/oe-lkp/[email protected] open here

Fixes 9734e25fbf5a ("perf: Fix the throttle logic for a group")

Signed-off-by Dapeng Mi <[email protected]>

Tested-by kernel test robot <[email protected]>

---
 arch/x86/events/intel/ds.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/x86/events/intel/ds.c +3 -0

index c0b7ac1c7594..dcf29c099ad2 100644
@@ -2663,6 +2663,16 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
 			continue;
 
 		event = cpuc->events[bit];
+		/*
+		 * perf_event_overflow() called by below __intel_pmu_pebs_last_event()
+		 * could trigger interrupt throttle and clear all event pointers of the

Quoted 48 lines · Dapeng Mi

When intel_pmu_drain_pebs_icl() is called to drain PEBS records, the perf_event_overflow() could be called to process the last PEBS record.

While perf_event_overflow() could trigger the interrupt throttle and stop all events of the group, like what the below call-chain shows.

perf_event_overflow()
-> __perf_event_overflow()

    ->__perf_event_account_interrupt()
      -> perf_event_throttle_group()
        -> perf_event_throttle()
          -> event->pmu->stop()
            -> x86_pmu_stop()

The side effect of stopping the events is that all corresponding event pointers in cpuc->events[] array are cleared to NULL.

Assume there are two PEBS events (event a and event b) in a group. When intel_pmu_drain_pebs_icl() calls perf_event_overflow() to process the last PEBS record of PEBS event a, interrupt throttle is triggered and all pointers of event a and event b are cleared to NULL. Then intel_pmu_drain_pebs_icl() tries to process the last PEBS record of event b and encounters NULL pointer access.

Since the left PEBS records have been processed when stopping the event, check and skip to process the last PEBS record if cpuc->events[*] is NULL.

Reported-by kernel test robot <[email protected]>

Closes https://lore.kernel.org/oe-lkp/[email protected] open here

Fixes 9734e25fbf5a ("perf: Fix the throttle logic for a group")

Signed-off-by Dapeng Mi <[email protected]>

Tested-by kernel test robot <[email protected]>

---
 arch/x86/events/intel/ds.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/x86/events/intel/ds.c +3 -0

index c0b7ac1c7594..dcf29c099ad2 100644
@@ -2663,6 +2663,16 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
 			continue;
 
 		event = cpuc->events[bit];
+		/*
+		 * perf_event_overflow() called by below __intel_pmu_pebs_last_event()
+		 * could trigger interrupt throttle and clear all event pointers of the

arch/x86/events/intel/ds.c static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d · line 2678

Quoted 10 lines

arch/x86/events/intel/ds.c +7 -0

+		 * group in cpuc->events[] to NULL. So need to re-check if cpuc->events[*]
+		 * is NULL, if so it indicates the event has been throttled (stopped) and
+		 * the corresponding last PEBS records have been processed in stopping
+		 * event, don't need to process it again.
+		 */
+		if (!event)
+			continue;
 
 		__intel_pmu_pebs_last_event(event, iregs, regs, data, last[bit],
 					    counts[bit], setup_pebs_adaptive_sample_data);

So if this is due to __intel_pmu_pebs_last_event() calling into perf_event_overflow(); then isn't intel_pmu_drain_pebs_nhm() similarly affected?

And worse, the _nhm() version would loose all events for that counter, not just the last.

I'm really thinking this isn't the right thing to do.

How about we audit the entirety of arch/x86/events/ for cpuc->events[] usage and see if we can get away with changing x86_pmu_stop() to simply not clearing that field.

Or perhaps move the setting and clearing into x86_pmu_{add,del}() rather than x86_pmu_{start,stop}(). After all, the latter don't affect the counter placement, they just stop/start the event.

Quoted 57 lines · Peter Zijlstra

Quoted 56 lines · Dapeng Mi

When intel_pmu_drain_pebs_icl() is called to drain PEBS records, the perf_event_overflow() could be called to process the last PEBS record.

While perf_event_overflow() could trigger the interrupt throttle and stop all events of the group, like what the below call-chain shows.

perf_event_overflow()
-> __perf_event_overflow()

    ->__perf_event_account_interrupt()
      -> perf_event_throttle_group()
        -> perf_event_throttle()
          -> event->pmu->stop()
            -> x86_pmu_stop()

The side effect of stopping the events is that all corresponding event pointers in cpuc->events[] array are cleared to NULL.

Assume there are two PEBS events (event a and event b) in a group. When intel_pmu_drain_pebs_icl() calls perf_event_overflow() to process the last PEBS record of PEBS event a, interrupt throttle is triggered and all pointers of event a and event b are cleared to NULL. Then intel_pmu_drain_pebs_icl() tries to process the last PEBS record of event b and encounters NULL pointer access.

Since the left PEBS records have been processed when stopping the event, check and skip to process the last PEBS record if cpuc->events[*] is NULL.

Reported-by kernel test robot <[email protected]>

Closes https://lore.kernel.org/oe-lkp/[email protected] open here

Fixes 9734e25fbf5a ("perf: Fix the throttle logic for a group")

Signed-off-by Dapeng Mi <[email protected]>

Tested-by kernel test robot <[email protected]>

---
 arch/x86/events/intel/ds.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/x86/events/intel/ds.c +10 -0

index c0b7ac1c7594..dcf29c099ad2 100644
@@ -2663,6 +2663,16 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
 			continue;
 
 		event = cpuc->events[bit];
+		/*
+		 * perf_event_overflow() called by below __intel_pmu_pebs_last_event()
+		 * could trigger interrupt throttle and clear all event pointers of the
+		 * group in cpuc->events[] to NULL. So need to re-check if cpuc->events[*]
+		 * is NULL, if so it indicates the event has been throttled (stopped) and
+		 * the corresponding last PEBS records have been processed in stopping
+		 * event, don't need to process it again.
+		 */
+		if (!event)
+			continue;
 

Quoted 8 lines

arch/x86/events/intel/ds.c static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d · line 2678

Quoted 2 lines

arch/x86/events/intel/ds.c +0 -0

 		__intel_pmu_pebs_last_event(event, iregs, regs, data, last[bit],
 					    counts[bit], setup_pebs_adaptive_sample_data);

So if this is due to __intel_pmu_pebs_last_event() calling into perf_event_overflow(); then isn't intel_pmu_drain_pebs_nhm() similarly affected?

And worse, the _nhm() version would loose all events for that counter, not just the last.

hmm, Yes. After double check, I suppose I made a mistake for the answer to Andi. It indeed has data loss since the "ds->pebs_index" is reset at the head of _nhm()/_icl() these drain_pebs helper instead of the end of the drain_pebs helper.  :(

Quoted 6 lines

I'm really thinking this isn't the right thing to do.

How about we audit the entirety of arch/x86/events/ for cpuc->events[] usage and see if we can get away with changing x86_pmu_stop() to simply not clearing that field.

Checking current code, I suppose it's fine that we don't clear cpuc->events[] in x86_pmu_stop() since we already have another variable "cpuc->active_mask" which is used to indicate if the corresponding cpuc->events[*] is active. But in current code, the cpuc->active_mask is not always checked.

So if we select not to clear cpuc->events[] in x86_pmu_stop(), then it's a must to check cpuc->active_mask before really accessing cpuc->events[] represented event. Maybe we can add an inline function got check this.

bool inline x86_pmu_cntr_event_active(int idx)
{
    struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);

    return cpuc->events[idx] && test_bit(idx, cpuc->active_mask); }

Quoted 4 lines

Or perhaps move the setting and clearing into x86_pmu_{add,del}() rather than x86_pmu_{start,stop}(). After all, the latter don't affect the counter placement, they just stop/start the event.

IIUC, we could not move the setting into x86_pmu_add() from x86_pmu_stop() since the counter index is not finalized at x86_pmu_add() is called. The counter index could change for each adding a new event.

Quoted 1 line

Macro GLOBAL_CTRL_EN_PERF_METRICS is defined to 48 instead of BIT_ULL(48), it's inconsistent with other similar macros. This leads to this macro is quite easily used wrongly since users thinks it's a bit-mask just like other similar macros.

Thus change GLOBAL_CTRL_EN_PERF_METRICS to BIT_ULL(48) and eliminate this potential misuse.

Signed-off-by Dapeng Mi <[email protected]>

Tested-by Yi Lai <[email protected]>

---
 arch/x86/events/intel/core.c      | 8 ++++----
 arch/x86/include/asm/perf_event.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

arch/x86/events/intel/core.c +4 -4

index 15da60cf69f2..f88a99d8d125 100644
@@ -5319,9 +5319,9 @@ static void intel_pmu_check_hybrid_pmus(struct x86_hybrid_pmu *pmu)
                                                0, x86_pmu_num_counters(&pmu->pmu), 0, 0);
 
        if (pmu->intel_cap.perf_metrics)
-               pmu->intel_ctrl |= 1ULL << GLOBAL_CTRL_EN_PERF_METRICS;
+               pmu->intel_ctrl |= GLOBAL_CTRL_EN_PERF_METRICS;
        else
-               pmu->intel_ctrl &= ~(1ULL << GLOBAL_CTRL_EN_PERF_METRICS);
+               pmu->intel_ctrl &= ~GLOBAL_CTRL_EN_PERF_METRICS;
 
        intel_pmu_check_event_constraints(pmu->event_constraints,
                                          pmu->cntr_mask64,

@@ -5456,7 +5456,7 @@ static void intel_pmu_cpu_starting(int cpu)
                rdmsrq(MSR_IA32_PERF_CAPABILITIES, perf_cap.capabilities);
                if (!perf_cap.perf_metrics) {
                        x86_pmu.intel_cap.perf_metrics = 0;
-                       x86_pmu.intel_ctrl &= ~(1ULL << GLOBAL_CTRL_EN_PERF_METRICS);
+                       x86_pmu.intel_ctrl &= ~GLOBAL_CTRL_EN_PERF_METRICS;
                }
        }
 

@@ -7790,7 +7790,7 @@ __init int intel_pmu_init(void)
        }
 
        if (!is_hybrid() && x86_pmu.intel_cap.perf_metrics)
-               x86_pmu.intel_ctrl |= 1ULL << GLOBAL_CTRL_EN_PERF_METRICS;
+               x86_pmu.intel_ctrl |= GLOBAL_CTRL_EN_PERF_METRICS;
 
        if (x86_pmu.intel_cap.pebs_timing_info)
                x86_pmu.flags |= PMU_FL_RETIRE_LATENCY;

arch/x86/include/asm/perf_event.h +1 -2

index 70d1d94aca7e..f8247ac276c4 100644
@@ -430,7 +430,7 @@ static inline bool is_topdown_idx(int idx)
 #define GLOBAL_STATUS_TRACE_TOPAPMI            BIT_ULL(GLOBAL_STATUS_TRACE_TOPAPMI_BIT)
 #define GLOBAL_STATUS_PERF_METRICS_OVF_BIT     48
 
-#define GLOBAL_CTRL_EN_PERF_METRICS            48
+#define GLOBAL_CTRL_EN_PERF_METRICS            BIT_ULL(48)
 /*
  * We model guest LBR event tracing as another fixed-mode PMC like BTS.
  *
--

2.34.1

Changes:
v2 -> v3:
* Rebase to latest tip perf/core tree.
* Rewrite commit message to explain why NULL access happens and refine code (Patch 3/7)
* Refine commit message of patch 6/7
* Dump counters bitmap instead of absolute counter in boot message

    (patch 7/7)

v1 -> v2:
* Rebase to 6.17-rc1.
* No code changes.

Tests:
* Run perf stats/record commands on Intel Sapphire Rapids platform, no issue is found.

History:
v2: https://lore.kernel.org/all/[email protected]/
v1:
* Patch 1/6: https://lore.kernel.org/all/[email protected]/
* Patch 2/6: https://lore.kernel.org/all/[email protected]/
* Patch 3/6: https://lore.kernel.org/all/[email protected]/
* Patches 4-6/6: https://lore.kernel.org/all/[email protected]/

Dapeng Mi (7):
perf/x86/intel: Use early_initcall() to hook bts_init() perf/x86/intel: Fix IA32_PMC_x_CFG_B MSRs access error perf/x86: Check if cpuc->events[*] pointer exists before accessing it perf/x86: Add PERF_CAP_PEBS_TIMING_INFO flag perf/x86/intel: Change macro GLOBAL_CTRL_EN_PERF_METRICS to

    BIT_ULL(48)

perf/x86/intel: Add ICL_FIXED_0_ADAPTIVE bit into
INTEL_FIXED_BITS_MASK
perf/x86: Print PMU counters bitmap in x86_pmu_show_pmu_cap()

 arch/x86/events/core.c                 | 16 +++++++++-------
 arch/x86/events/intel/bts.c            |  2 +-
 arch/x86/events/intel/core.c           | 21 +++++++++------------
 arch/x86/events/intel/ds.c             | 10 ++++++++++
 arch/x86/include/asm/msr-index.h       | 14 ++++++++------
 arch/x86/include/asm/perf_event.h      |  8 ++++++--
 arch/x86/kvm/pmu.h                     |  2 +-
 tools/arch/x86/include/asm/msr-index.h | 14 ++++++++------
 8 files changed, 52 insertions(+), 35 deletions(-)

base-commit: 448f97fba9013ffa13f5dd82febd18836b189499

Quoted 11 lines

Dapeng Mi (7):
perf/x86/intel: Use early_initcall() to hook bts_init() perf/x86/intel: Fix IA32_PMC_x_CFG_B MSRs access error perf/x86: Check if cpuc->events[*] pointer exists before accessing it perf/x86: Add PERF_CAP_PEBS_TIMING_INFO flag perf/x86/intel: Change macro GLOBAL_CTRL_EN_PERF_METRICS to

    BIT_ULL(48)

perf/x86/intel: Add ICL_FIXED_0_ADAPTIVE bit into
INTEL_FIXED_BITS_MASK
perf/x86: Print PMU counters bitmap in x86_pmu_show_pmu_cap()

The series looks good to me.

Reviewed-by Kan Liang <[email protected]>

Quoted 11 lines · Kan

Quoted 11 lines

Dapeng Mi (7):
perf/x86/intel: Use early_initcall() to hook bts_init() perf/x86/intel: Fix IA32_PMC_x_CFG_B MSRs access error perf/x86: Check if cpuc->events[*] pointer exists before accessing it perf/x86: Add PERF_CAP_PEBS_TIMING_INFO flag perf/x86/intel: Change macro GLOBAL_CTRL_EN_PERF_METRICS to

    BIT_ULL(48)

perf/x86/intel: Add ICL_FIXED_0_ADAPTIVE bit into
INTEL_FIXED_BITS_MASK
perf/x86: Print PMU counters bitmap in x86_pmu_show_pmu_cap()

Quoted 3 lines

The series looks good to me.

Reviewed-by Kan Liang <[email protected]>

I've picked up all but patch 3 -- I really don't think that does the right thing.