[PATCH] perf/x86/amd/core: always clear status for idx

[PATCH] perf/x86/amd/core: always clear status for idx

[Breno Leitao]

The variable 'status' (which contains the unhandled overflow bits) is
not being properly masked in some cases, displaying the following
warning:

  WARNING: CPU: 156 PID: 475601 at arch/x86/events/amd/core.c:972 amd_pmu_v2_handle_irq+0x216/0x270

This seems to be happening because the loop is being continued before
the status bit being unset, in case x86_perf_event_set_period()
returns 0. This is also causing an inconsistency because the "handled"
counter is incremented, but the status bit is not cleaned.

Move the bit cleaning together above, together when the "handled"
counter is incremented.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/x86/events/amd/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index 8c45b198b62f..bccea57dee81 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -923,6 +923,7 @@ static int amd_pmu_v2_handle_irq(struct pt_regs *regs)
 
 		/* Event overflow */
 		handled++;
+		status &= ~mask;
 		perf_sample_data_init(&data, 0, hwc->last_period);
 
 		if (!x86_perf_event_set_period(event))
@@ -933,8 +934,6 @@ static int amd_pmu_v2_handle_irq(struct pt_regs *regs)
 
 		if (perf_event_overflow(event, &data, regs))
 			x86_pmu_stop(event, 0);
-
-		status &= ~mask;
 	}
 
 	/*
-- 
2.34.1

Re: [PATCH] perf/x86/amd/core: always clear status for idx

[Sandipan Das]

On 3/21/2023 5:03 PM, Breno Leitao wrote:
> The variable 'status' (which contains the unhandled overflow bits) is
> not being properly masked in some cases, displaying the following
> warning:
> 
>   WARNING: CPU: 156 PID: 475601 at arch/x86/events/amd/core.c:972 amd_pmu_v2_handle_irq+0x216/0x270
> 
> This seems to be happening because the loop is being continued before
> the status bit being unset, in case x86_perf_event_set_period()
> returns 0. This is also causing an inconsistency because the "handled"
> counter is incremented, but the status bit is not cleaned.
> 
> Move the bit cleaning together above, together when the "handled"
> counter is incremented.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/x86/events/amd/core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 8c45b198b62f..bccea57dee81 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -923,6 +923,7 @@ static int amd_pmu_v2_handle_irq(struct pt_regs *regs)
>  
>  		/* Event overflow */
>  		handled++;
> +		status &= ~mask;
>  		perf_sample_data_init(&data, 0, hwc->last_period);
>  
>  		if (!x86_perf_event_set_period(event))
> @@ -933,8 +934,6 @@ static int amd_pmu_v2_handle_irq(struct pt_regs *regs)
>  
>  		if (perf_event_overflow(event, &data, regs))
>  			x86_pmu_stop(event, 0);
> -
> -		status &= ~mask;
>  	}
>  
>  	/*

Thanks for fixing this.

Reviewed-by: Sandipan Das <sandipan.das@amd.com>

Re: [PATCH] perf/x86/amd/core: always clear status for idx

[Peter Zijlstra]

On Tue, Mar 21, 2023 at 04:33:38AM -0700, Breno Leitao wrote:
> The variable 'status' (which contains the unhandled overflow bits) is
> not being properly masked in some cases, displaying the following
> warning:
> 
>   WARNING: CPU: 156 PID: 475601 at arch/x86/events/amd/core.c:972 amd_pmu_v2_handle_irq+0x216/0x270
> 
> This seems to be happening because the loop is being continued before
> the status bit being unset, in case x86_perf_event_set_period()
> returns 0. This is also causing an inconsistency because the "handled"
> counter is incremented, but the status bit is not cleaned.
> 
> Move the bit cleaning together above, together when the "handled"
> counter is incremented.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Can I get a Fixes: tag so I can consider it for perf/urgent ?

Re: [PATCH] perf/x86/amd/core: always clear status for idx

[Sandipan Das]

On 3/21/2023 6:12 PM, Peter Zijlstra wrote:
> On Tue, Mar 21, 2023 at 04:33:38AM -0700, Breno Leitao wrote:
>> The variable 'status' (which contains the unhandled overflow bits) is
>> not being properly masked in some cases, displaying the following
>> warning:
>>
>>   WARNING: CPU: 156 PID: 475601 at arch/x86/events/amd/core.c:972 amd_pmu_v2_handle_irq+0x216/0x270
>>
>> This seems to be happening because the loop is being continued before
>> the status bit being unset, in case x86_perf_event_set_period()
>> returns 0. This is also causing an inconsistency because the "handled"
>> counter is incremented, but the status bit is not cleaned.
>>
>> Move the bit cleaning together above, together when the "handled"
>> counter is incremented.
>>
>> Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> Can I get a Fixes: tag so I can consider it for perf/urgent ?

That would be

Fixes: 7685665c390d ("perf/x86/amd/core: Add PerfMonV2 overflow handling")

Re: [PATCH] perf/x86/amd/core: always clear status for idx

[Peter Zijlstra]

On Tue, Mar 21, 2023 at 06:26:11PM +0530, Sandipan Das wrote:
> On 3/21/2023 6:12 PM, Peter Zijlstra wrote:
> > On Tue, Mar 21, 2023 at 04:33:38AM -0700, Breno Leitao wrote:
> >> The variable 'status' (which contains the unhandled overflow bits) is
> >> not being properly masked in some cases, displaying the following
> >> warning:
> >>
> >>   WARNING: CPU: 156 PID: 475601 at arch/x86/events/amd/core.c:972 amd_pmu_v2_handle_irq+0x216/0x270
> >>
> >> This seems to be happening because the loop is being continued before
> >> the status bit being unset, in case x86_perf_event_set_period()
> >> returns 0. This is also causing an inconsistency because the "handled"
> >> counter is incremented, but the status bit is not cleaned.
> >>
> >> Move the bit cleaning together above, together when the "handled"
> >> counter is incremented.
> >>
> >> Signed-off-by: Breno Leitao <leitao@debian.org>
> > 
> > Can I get a Fixes: tag so I can consider it for perf/urgent ?
> 
> That would be
> 
> Fixes: 7685665c390d ("perf/x86/amd/core: Add PerfMonV2 overflow handling")
> 

Cheers!