[PATCH 0/2] perf/core: Fix broken throttling bugs

[PATCH 0/2] perf/core: Fix broken throttling bugs

[Qing Wong]

From: Qing Wang <wangqing7171@gmail.com>

There is a bug that broken throttling, this patchset intends
to improve the accuracy of throttling.

This patchset does two changes:
  - Revert a specific patch to avoid broken throttling when
    max_samples_per_tick>1.
  - Add throttling judgement when max_samples_per_tick=1.

base-commit: acc4d5ff0b61

Qing Wang (2):
  Revert "perf/core: Fix hardlockup failure caused by perf throttle"
  perf/core: Fix broken throttling when max_samples_per_tick=1

 kernel/events/core.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH 1/2] Revert "perf/core: Fix hardlockup failure caused by perf throttle"

[Qing Wong]

From: Qing Wang <wangqing7171@gmail.com>

This reverts commit 15def34e2635ab7e0e96f1bc32e1b69609f14942.

The hardlockup failure does not exist because:
1. The hardlockup's watchdog event is a pinned event, which exclusively
occupies a dedicated PMC (Performance Monitoring Counter) and is unaffected
by PMC scheduling.
2. The hardware event throttling mechanism only disables the specific PMC
where throttling occurs, without impacting other PMCs. Consequently, The
hardlockup event's dedicated PMC remains entirely unaffected.

Signed-off-by: Qing Wang <wangqing7171@gmail.com>
---
 kernel/events/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0bb21659e252..29cdb240e104 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10049,8 +10049,8 @@ __perf_event_account_interrupt(struct perf_event *event, int throttle)
 		hwc->interrupts = 1;
 	} else {
 		hwc->interrupts++;
-		if (unlikely(throttle &&
-			     hwc->interrupts > max_samples_per_tick)) {
+		if (unlikely(throttle
+			     && hwc->interrupts >= max_samples_per_tick)) {
 			__this_cpu_inc(perf_throttled_count);
 			tick_dep_set_cpu(smp_processor_id(), TICK_DEP_BIT_PERF_EVENTS);
 			hwc->interrupts = MAX_INTERRUPTS;
-- 
2.43.0

[PATCH 2/2] perf/core: Fix broken throttling when max_samples_per_tick=1

[Qing Wong]

From: Qing Wang <wangqing7171@gmail.com>

According to the throttling mechanism, the pmu interrupts number can not
exceed the max_samples_per_tick in one tick. But this mechanism is
ineffective when max_samples_per_tick=1, because the throttling check is
skipped during the first interrupt and only performed when the second
interrupt arrives.

Perhaps this bug may cause little influence in one tick, but if in a
larger time scale, the problem can not be underestimated.

When max_samples_per_tick = 1:
Allowed-interrupts-per-second max-samples-per-second  default-HZ  ARCH
200                           100                     100         X86
500                           250                     250         ARM64
...
Obviously, the pmu interrupt number far exceed the user's expect.

Fixes: e050e3f0a71b ("perf: Fix broken interrupt rate throttling")
Signed-off-by: Qing Wang <wangqing7171@gmail.com>
---
 kernel/events/core.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 29cdb240e104..4ac2ac988ddc 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10047,16 +10047,15 @@ __perf_event_account_interrupt(struct perf_event *event, int throttle)
 	if (seq != hwc->interrupts_seq) {
 		hwc->interrupts_seq = seq;
 		hwc->interrupts = 1;
-	} else {
+	} else
 		hwc->interrupts++;
-		if (unlikely(throttle
-			     && hwc->interrupts >= max_samples_per_tick)) {
-			__this_cpu_inc(perf_throttled_count);
-			tick_dep_set_cpu(smp_processor_id(), TICK_DEP_BIT_PERF_EVENTS);
-			hwc->interrupts = MAX_INTERRUPTS;
-			perf_log_throttle(event, 0);
-			ret = 1;
-		}
+
+	if (unlikely(throttle && hwc->interrupts >= max_samples_per_tick)) {
+		__this_cpu_inc(perf_throttled_count);
+		tick_dep_set_cpu(smp_processor_id(), TICK_DEP_BIT_PERF_EVENTS);
+		hwc->interrupts = MAX_INTERRUPTS;
+		perf_log_throttle(event, 0);
+		ret = 1;
 	}
 
 	if (event->attr.freq) {
-- 
2.43.0

Re: [PATCH 0/2] perf/core: Fix broken throttling bugs

[Qing Wang]

Hello,

PING.

Thanks,
Qing.

On 4/5/2025 10:16 PM, Qing Wong wrote:
> From: Qing Wang <wangqing7171@gmail.com>
>
> There is a bug that broken throttling, this patchset intends
> to improve the accuracy of throttling.
>
> This patchset does two changes:
>    - Revert a specific patch to avoid broken throttling when
>      max_samples_per_tick>1.
>    - Add throttling judgement when max_samples_per_tick=1.
>
> base-commit: acc4d5ff0b61
>
> Qing Wang (2):
>    Revert "perf/core: Fix hardlockup failure caused by perf throttle"
>    perf/core: Fix broken throttling when max_samples_per_tick=1
>
>   kernel/events/core.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
>

Re: [PATCH 1/2] Revert "perf/core: Fix hardlockup failure caused by perf throttle"

[Peter Zijlstra]

On Sat, Apr 05, 2025 at 10:16:34PM +0800, Qing Wong wrote:
> From: Qing Wang <wangqing7171@gmail.com>
> 
> This reverts commit 15def34e2635ab7e0e96f1bc32e1b69609f14942.
> 
> The hardlockup failure does not exist because:
> 1. The hardlockup's watchdog event is a pinned event, which exclusively
> occupies a dedicated PMC (Performance Monitoring Counter) and is unaffected
> by PMC scheduling.
> 2. The hardware event throttling mechanism only disables the specific PMC
> where throttling occurs, without impacting other PMCs. Consequently, The
> hardlockup event's dedicated PMC remains entirely unaffected.
> 
> Signed-off-by: Qing Wang <wangqing7171@gmail.com>
> ---
>  kernel/events/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 0bb21659e252..29cdb240e104 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10049,8 +10049,8 @@ __perf_event_account_interrupt(struct perf_event *event, int throttle)
>  		hwc->interrupts = 1;
>  	} else {
>  		hwc->interrupts++;
> -		if (unlikely(throttle &&
> -			     hwc->interrupts > max_samples_per_tick)) {
> +		if (unlikely(throttle
> +			     && hwc->interrupts >= max_samples_per_tick)) {

Well, it restores bad coding style. The referred commit also states that
max_samples_per_tick can be 1, at which point we'll always throttle the
thing, since we've just increased.

That is, the part of the old commit that argued about e050e3f0a71bf
flipped the compare and the increment is still true. So even though it
might not be related to the hardlockup problem, I still don't think the
patch was wrong.

Re: [PATCH 2/2] perf/core: Fix broken throttling when max_samples_per_tick=1

[Peter Zijlstra]

On Sat, Apr 05, 2025 at 10:16:35PM +0800, Qing Wong wrote:
> From: Qing Wang <wangqing7171@gmail.com>
> 
> According to the throttling mechanism, the pmu interrupts number can not
> exceed the max_samples_per_tick in one tick. But this mechanism is
> ineffective when max_samples_per_tick=1, because the throttling check is
> skipped during the first interrupt and only performed when the second
> interrupt arrives.
> 
> Perhaps this bug may cause little influence in one tick, but if in a
> larger time scale, the problem can not be underestimated.
> 
> When max_samples_per_tick = 1:
> Allowed-interrupts-per-second max-samples-per-second  default-HZ  ARCH
> 200                           100                     100         X86
> 500                           250                     250         ARM64
> ...
> Obviously, the pmu interrupt number far exceed the user's expect.
> 
> Fixes: e050e3f0a71b ("perf: Fix broken interrupt rate throttling")
> Signed-off-by: Qing Wang <wangqing7171@gmail.com>
> ---
>  kernel/events/core.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 29cdb240e104..4ac2ac988ddc 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10047,16 +10047,15 @@ __perf_event_account_interrupt(struct perf_event *event, int throttle)
>  	if (seq != hwc->interrupts_seq) {
>  		hwc->interrupts_seq = seq;
>  		hwc->interrupts = 1;
> -	} else {
> +	} else
>  		hwc->interrupts++;
> -		if (unlikely(throttle
> -			     && hwc->interrupts >= max_samples_per_tick)) {
> -			__this_cpu_inc(perf_throttled_count);
> -			tick_dep_set_cpu(smp_processor_id(), TICK_DEP_BIT_PERF_EVENTS);
> -			hwc->interrupts = MAX_INTERRUPTS;
> -			perf_log_throttle(event, 0);
> -			ret = 1;
> -		}
> +
> +	if (unlikely(throttle && hwc->interrupts >= max_samples_per_tick)) {
> +		__this_cpu_inc(perf_throttled_count);
> +		tick_dep_set_cpu(smp_processor_id(), TICK_DEP_BIT_PERF_EVENTS);
> +		hwc->interrupts = MAX_INTERRUPTS;
> +		perf_log_throttle(event, 0);
> +		ret = 1;
>  	}

Fair enough I suppose. I'll make this apply without that revert -- it
seems pointless to have that in between.

Re: [PATCH 2/2] perf/core: Fix broken throttling when max_samples_per_tick=1

[Qing Wang]

Thank you very much for your review. Do you need me to reorganize the 
patch and send it out? Because if only the second patch is accepted, its 
context won't match the current mainline code.

On 4/18/2025 5:03 PM, Peter Zijlstra wrote:
> Fair enough I suppose. I'll make this apply without that revert -- it
> seems pointless to have that in between.

Re: [PATCH 2/2] perf/core: Fix broken throttling when max_samples_per_tick=1

[Peter Zijlstra]

On Fri, Apr 18, 2025 at 09:08:30PM +0800, Qing Wang wrote:
> Thank you very much for your review. Do you need me to reorganize the patch
> and send it out? Because if only the second patch is accepted, its context
> won't match the current mainline code.

I've stomped on it a bit and pushed out to queue/perf/core.

If all looks well, and the robots don't have a fit because I failed to
compile test the thing, it should eventually make its way into tip.

Re: [PATCH 2/2] perf/core: Fix broken throttling when max_samples_per_tick=1

[Qing Wang]

Thank you again. This has kick-started my journey of contributing to the 
Linux Kernel community, and it's really awesome.

On 4/18/2025 9:10 PM, Peter Zijlstra wrote:
> I've stomped on it a bit and pushed out to queue/perf/core.
>
> If all looks well, and the robots don't have a fit because I failed to
> compile test the thing, it should eventually make its way into tip.