[PATCH] perf/aux: Properly launch pending disable flow

[PATCH] perf/aux: Properly launch pending disable flow

[Leo Yan]

If an AUX event overruns, the event core layer intends to disable the
event by setting the 'pending_disable' flag. Unfortunately, the event
is not actually disabled afterwards.

Since commit ca6c21327c6a ("perf: Fix missing SIGTRAPs"), the
'pending_disable' flag was changed to a boolean toggles. However, the
AUX event code was not updated accordingly. The flag ends up holding a
CPU number. If this number is zero, the flag is taken as false and the
IRQ work is never triggered.

Later, with commit 2b84def990d3 ("perf: Split __perf_pending_irq() out
of perf_pending_irq()"), a new IRQ work 'pending_disable_irq' was
introduced to handle event disabling. The AUX event path was not updated
to kick off the work queue.

To fix this issue, when an AUX ring buffer overrun is detected, call
perf_event_disable_inatomic() to initiate the pending disable flow.

Fixes: ca6c21327c6a ("perf: Fix missing SIGTRAPs")
Fixes: 2b84def990d3 ("perf: Split __perf_pending_irq() out of perf_pending_irq()")
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 kernel/events/ring_buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index d2aef87c7e9f..aa9a759e824f 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -441,7 +441,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
 		 * store that will be enabled on successful return
 		 */
 		if (!handle->size) { /* A, matches D */
-			event->pending_disable = smp_processor_id();
+			perf_event_disable_inatomic(handle->event);
 			perf_output_wakeup(handle);
 			WRITE_ONCE(rb->aux_nest, 0);
 			goto err_put;
@@ -526,7 +526,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 
 	if (wakeup) {
 		if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
-			handle->event->pending_disable = smp_processor_id();
+			perf_event_disable_inatomic(handle->event);
 		perf_output_wakeup(handle);
 	}
 
-- 
2.34.1

Re: [PATCH] perf/aux: Properly launch pending disable flow

[Leo Yan]

On Thu, May 22, 2025 at 04:05:10PM +0100, Leo Yan wrote:
> If an AUX event overruns, the event core layer intends to disable the
> event by setting the 'pending_disable' flag. Unfortunately, the event
> is not actually disabled afterwards.
> 
> Since commit ca6c21327c6a ("perf: Fix missing SIGTRAPs"), the
> 'pending_disable' flag was changed to a boolean toggles. However, the
> AUX event code was not updated accordingly. The flag ends up holding a
> CPU number. If this number is zero, the flag is taken as false and the
> IRQ work is never triggered.
> 
> Later, with commit 2b84def990d3 ("perf: Split __perf_pending_irq() out
> of perf_pending_irq()"), a new IRQ work 'pending_disable_irq' was
> introduced to handle event disabling. The AUX event path was not updated
> to kick off the work queue.
> 
> To fix this issue, when an AUX ring buffer overrun is detected, call
> perf_event_disable_inatomic() to initiate the pending disable flow.
> 
> Fixes: ca6c21327c6a ("perf: Fix missing SIGTRAPs")
> Fixes: 2b84def990d3 ("perf: Split __perf_pending_irq() out of perf_pending_irq()")
> Signed-off-by: Leo Yan <leo.yan@arm.com>

Gentle ping.  This would be an important fix for AUX trace.

Thanks,
Leo

> ---
>  kernel/events/ring_buffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index d2aef87c7e9f..aa9a759e824f 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -441,7 +441,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
>  		 * store that will be enabled on successful return
>  		 */
>  		if (!handle->size) { /* A, matches D */
> -			event->pending_disable = smp_processor_id();
> +			perf_event_disable_inatomic(handle->event);
>  			perf_output_wakeup(handle);
>  			WRITE_ONCE(rb->aux_nest, 0);
>  			goto err_put;
> @@ -526,7 +526,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
>  
>  	if (wakeup) {
>  		if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
> -			handle->event->pending_disable = smp_processor_id();
> +			perf_event_disable_inatomic(handle->event);
>  		perf_output_wakeup(handle);
>  	}
>  
> -- 
> 2.34.1
> 

Re: [PATCH] perf/aux: Properly launch pending disable flow

[Leo Yan]

Hi Peter, Ingo,

On Mon, Jun 09, 2025 at 01:57:59PM +0100, Leo Yan wrote:
> On Thu, May 22, 2025 at 04:05:10PM +0100, Leo Yan wrote:
> > If an AUX event overruns, the event core layer intends to disable the
> > event by setting the 'pending_disable' flag. Unfortunately, the event
> > is not actually disabled afterwards.

Do you mind to check if this is a valid fix?  Thanks!

Leo.

> > Since commit ca6c21327c6a ("perf: Fix missing SIGTRAPs"), the
> > 'pending_disable' flag was changed to a boolean toggles. However, the
> > AUX event code was not updated accordingly. The flag ends up holding a
> > CPU number. If this number is zero, the flag is taken as false and the
> > IRQ work is never triggered.
> > 
> > Later, with commit 2b84def990d3 ("perf: Split __perf_pending_irq() out
> > of perf_pending_irq()"), a new IRQ work 'pending_disable_irq' was
> > introduced to handle event disabling. The AUX event path was not updated
> > to kick off the work queue.
> > 
> > To fix this issue, when an AUX ring buffer overrun is detected, call
> > perf_event_disable_inatomic() to initiate the pending disable flow.
> > 
> > Fixes: ca6c21327c6a ("perf: Fix missing SIGTRAPs")
> > Fixes: 2b84def990d3 ("perf: Split __perf_pending_irq() out of perf_pending_irq()")
> > Signed-off-by: Leo Yan <leo.yan@arm.com>
> 
> Gentle ping.  This would be an important fix for AUX trace.
> 
> Thanks,
> Leo
> 
> > ---
> >  kernel/events/ring_buffer.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> > index d2aef87c7e9f..aa9a759e824f 100644
> > --- a/kernel/events/ring_buffer.c
> > +++ b/kernel/events/ring_buffer.c
> > @@ -441,7 +441,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
> >  		 * store that will be enabled on successful return
> >  		 */
> >  		if (!handle->size) { /* A, matches D */
> > -			event->pending_disable = smp_processor_id();
> > +			perf_event_disable_inatomic(handle->event);
> >  			perf_output_wakeup(handle);
> >  			WRITE_ONCE(rb->aux_nest, 0);
> >  			goto err_put;
> > @@ -526,7 +526,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
> >  
> >  	if (wakeup) {
> >  		if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
> > -			handle->event->pending_disable = smp_processor_id();
> > +			perf_event_disable_inatomic(handle->event);
> >  		perf_output_wakeup(handle);
> >  	}
> >  
> > -- 
> > 2.34.1
> > 
> 

Re: [PATCH] perf/aux: Properly launch pending disable flow

[Yeoreum Yun]

Hi Leo,

[...]
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -441,7 +441,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
>  		 * store that will be enabled on successful return
>  		 */
>  		if (!handle->size) { /* A, matches D */
> -			event->pending_disable = smp_processor_id();
> +			perf_event_disable_inatomic(handle->event);
>  			perf_output_wakeup(handle);
>  			WRITE_ONCE(rb->aux_nest, 0);
>  			goto err_put;
> @@ -526,7 +526,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
>
>  	if (wakeup) {
>  		if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
> -			handle->event->pending_disable = smp_processor_id();
> +			perf_event_disable_inatomic(handle->event);
>  		perf_output_wakeup(handle);
>  	}

LGTM.

Reviewed-by: Yeoreum Yun <yeoreum.yun@arm.com>

--
Sincerely,
Yeoreum Yun

Re: [PATCH] perf/aux: Properly launch pending disable flow

[James Clark]

On 22/05/2025 4:05 pm, Leo Yan wrote:
> If an AUX event overruns, the event core layer intends to disable the
> event by setting the 'pending_disable' flag. Unfortunately, the event
> is not actually disabled afterwards.
> 
> Since commit ca6c21327c6a ("perf: Fix missing SIGTRAPs"), the
> 'pending_disable' flag was changed to a boolean toggles. However, the
> AUX event code was not updated accordingly. The flag ends up holding a
> CPU number. If this number is zero, the flag is taken as false and the
> IRQ work is never triggered.
> 
> Later, with commit 2b84def990d3 ("perf: Split __perf_pending_irq() out
> of perf_pending_irq()"), a new IRQ work 'pending_disable_irq' was
> introduced to handle event disabling. The AUX event path was not updated
> to kick off the work queue.
> 
> To fix this issue, when an AUX ring buffer overrun is detected, call
> perf_event_disable_inatomic() to initiate the pending disable flow.
> 
> Fixes: ca6c21327c6a ("perf: Fix missing SIGTRAPs")
> Fixes: 2b84def990d3 ("perf: Split __perf_pending_irq() out of perf_pending_irq()")
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   kernel/events/ring_buffer.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index d2aef87c7e9f..aa9a759e824f 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -441,7 +441,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
>   		 * store that will be enabled on successful return
>   		 */
>   		if (!handle->size) { /* A, matches D */
> -			event->pending_disable = smp_processor_id();
> +			perf_event_disable_inatomic(handle->event);
>   			perf_output_wakeup(handle);
>   			WRITE_ONCE(rb->aux_nest, 0);
>   			goto err_put;
> @@ -526,7 +526,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
>   
>   	if (wakeup) {
>   		if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
> -			handle->event->pending_disable = smp_processor_id();
> +			perf_event_disable_inatomic(handle->event);
>   		perf_output_wakeup(handle);
>   	}
>   

The types are now a bit misleading and pending_wakeup and 
pending_disable could be bool types. The other pending_*s do use their 
types properly though.

__perf_pending_disable() also still contains a big comment that 
describes use of CPU ID and -1 values.

Other than that it makes sense.

Reviewed-by: James Clark <james.clark@linaro.org>

Re: [PATCH] perf/aux: Properly launch pending disable flow

[Leo Yan]

On Tue, Jun 24, 2025 at 02:11:38PM +0100, James Clark wrote:

[...]

> > @@ -441,7 +441,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
> >   		 * store that will be enabled on successful return
> >   		 */
> >   		if (!handle->size) { /* A, matches D */
> > -			event->pending_disable = smp_processor_id();
> > +			perf_event_disable_inatomic(handle->event);
> >   			perf_output_wakeup(handle);
> >   			WRITE_ONCE(rb->aux_nest, 0);
> >   			goto err_put;
> > @@ -526,7 +526,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
> >   	if (wakeup) {
> >   		if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
> > -			handle->event->pending_disable = smp_processor_id();
> > +			perf_event_disable_inatomic(handle->event);
> >   		perf_output_wakeup(handle);
> >   	}
> 
> The types are now a bit misleading and pending_wakeup and pending_disable
> could be bool types. The other pending_*s do use their types properly
> though.
> 
> __perf_pending_disable() also still contains a big comment that describes
> use of CPU ID and -1 values.

I might use an extra patch to address type and comment issue.

> Other than that it makes sense.
> 
> Reviewed-by: James Clark <james.clark@linaro.org>

Thanks for review!

Re: [PATCH] perf/aux: Properly launch pending disable flow

[Leo Yan]

On Tue, Jun 24, 2025 at 02:32:17PM +0100, Leo Yan wrote:
> On Tue, Jun 24, 2025 at 02:11:38PM +0100, James Clark wrote:
> 
> [...]
> 
> > > @@ -441,7 +441,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
> > >   		 * store that will be enabled on successful return
> > >   		 */
> > >   		if (!handle->size) { /* A, matches D */
> > > -			event->pending_disable = smp_processor_id();
> > > +			perf_event_disable_inatomic(handle->event);
> > >   			perf_output_wakeup(handle);
> > >   			WRITE_ONCE(rb->aux_nest, 0);
> > >   			goto err_put;
> > > @@ -526,7 +526,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
> > >   	if (wakeup) {
> > >   		if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
> > > -			handle->event->pending_disable = smp_processor_id();
> > > +			perf_event_disable_inatomic(handle->event);
> > >   		perf_output_wakeup(handle);
> > >   	}
> > 
> > The types are now a bit misleading and pending_wakeup and pending_disable
> > could be bool types. The other pending_*s do use their types properly
> > though.

Changing the type from unsigned int to bool is a refactoring and not
part of the actual fix. Therefore, I left it out in patch v2.

Thanks,
Leo