mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: perf AUX: race causes poll() hang
       [not found] <CAE7+JAnhZSuMMG9+0pZ2G6ZBR-OAC1hHptkr=T=x5LK9n19vOQ@mail.gmail.com>
@ 2026-05-18  9:41 ` Peter Zijlstra
  2026-05-19  5:58   ` Adrian Hunter
  2026-06-14 22:40   ` Konstantin Mikhaylov
  0 siblings, 2 replies; 4+ messages in thread
From: Peter Zijlstra @ 2026-05-18  9:41 UTC (permalink / raw)
  To: Константин
	Михайлов
  Cc: adrian.hunter, Mark Rutland, linux-kernel

On Fri, May 15, 2026 at 01:35:48AM +0300, Константин Михайлов wrote:
> Hello Peter and Adrian,
> 
> I'd like to report a potential race condition in perf AUX buffer handling.
> 
> AUX tracing is designed to allow the tracee continue running when the AUX
> buffer fills. The PMU driver must disable tracing when AUX buffer is full.
> Typically, it schedules IRQ work to disable the event later. Meanwhile, a
> typical tracer's workflow looks like: poll() on perf FDs, consume the data,
> re-enable the event via PERF_EVENT_IOC_ENABLE ioctl(), then poll() again.
> 
> Given this, the following race is possible:
> -------------------
> | CPU #0 | CPU #1 |
> | tracee | tracer |
> -------------------
> |   **   |        |   tracee fills the AUX buffer completely with some data
> -------------------
> |   **   |        |   PMU driver updates aux_head accordingly and schedules IRQ works
> |   **   |        |   to disable the event and wake up the tracer (setting rb->poll in
> |   **   |        |   perf_output_wakeup() along the way)
> -------------------
> |        |   **   |   tracer consumes all the data from AUX buffer,
> |        |   **   |   thus clears rb->poll in perf_poll()
> -------------------
> |        |   **   |   tracer re-enables the tracing (the event is still active,
> |        |   **   |   so ioctl(...) returns immediately)
> -------------------
> |        |   **   |   tracer starts poll()'ing the AUX buffer again
> -------------------
> |   **   |        |   IRQ work handler finally disables the event and
> |   **   |        |   wakes up tracer
> -------------------
> |        |   **   |   tracer obtains zero rb->poll and continues polling
> -------------------
> As a result, tracee runs without PMU tracing, and tracer's poll() will
> never be woken up unless it has some timeout.
> 
> I reproduced this on an x86 machine with intel_pt and kernel v6.17.
> Reproducing this race on the vanilla kernel is timing-sensitive, so I added
> 30 ms delay in the error path in intel_pt_interrupt() when
> pt_buffer_reset_markers() returns an error - this delay widens the window
> between aux_head update and actual event disable in IRQ work handler. I'm
> not sure that pt_buffer_reset_markers()'s error means that buffer
> overflowed, but this error branch is taken sometimes and all needed IRQ
> works are scheduled during a call to perf_aux_output_end(). I also added 3
> ms delay in perf in __auxtrace_mmap__read() before itr->read_finish(),
> ensuring the ioctl() falls into that window. With these changes, some perf
> runs collected smaller traces than usual. I added traceprints to intel_pt
> driver and enabled tracing for sys_poll and sys_ioctl, which confirmed the
> exact sequence described above. The problem was sometimes mitigated by
> tracee migration to another cpu (as perf creates an event for every cpu,
> the event is re-enabled by kernel when it is set on a new cpu). Otherwise,
> tracee stayed on the same cpu and tracer hung on poll() until tracee exited.
> 
> Could you please confirm if this analysis is correct? Should we move
> setting of rb->poll *after* the event is disabled in IRQ work handler?

I *think* (its been a minute since I looked at this code), that you're
right.

Does something like the below cure things?

---
 kernel/events/core.c | 54 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7935d5663944..490407618f36 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2677,6 +2677,9 @@ static void __perf_event_disable(struct perf_event *event,
 				 struct perf_event_context *ctx,
 				 void *info)
 {
+	if (event->pending_disable)
+		event->pending_disable = 0;
+
 	if (event->state < PERF_EVENT_STATE_INACTIVE)
 		return;
 
@@ -3278,32 +3281,37 @@ static void _perf_event_enable(struct perf_event *event)
 {
 	struct perf_event_context *ctx = event->ctx;
 
-	raw_spin_lock_irq(&ctx->lock);
-	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
-	    event->state <  PERF_EVENT_STATE_ERROR) {
-out:
-		raw_spin_unlock_irq(&ctx->lock);
-		return;
-	}
+	scoped_guard (raw_spinlock_irq, &ctx->lock) {
+		if (event->state <  PERF_EVENT_STATE_ERROR)
+			return;
 
-	/*
-	 * If the event is in error state, clear that first.
-	 *
-	 * That way, if we see the event in error state below, we know that it
-	 * has gone back into error state, as distinct from the task having
-	 * been scheduled away before the cross-call arrived.
-	 */
-	if (event->state == PERF_EVENT_STATE_ERROR) {
 		/*
-		 * Detached SIBLING events cannot leave ERROR state.
+		 * If the event is in error state, clear that first.
+		 *
+		 * That way, if we see the event in error state below, we know that it
+		 * has gone back into error state, as distinct from the task having
+		 * been scheduled away before the cross-call arrived.
 		 */
-		if (event->event_caps & PERF_EV_CAP_SIBLING &&
-		    event->group_leader == event)
-			goto out;
+		if (event->state == PERF_EVENT_STATE_ERROR) {
+			/*
+			 * Detached SIBLING events cannot leave ERROR state.
+			 */
+			if (event->event_caps & PERF_EV_CAP_SIBLING &&
+			    event->group_leader == event)
+				return;
 
-		event->state = PERF_EVENT_STATE_OFF;
+			event->state = PERF_EVENT_STATE_OFF;
+		}
+
+		if (event->pending_disable)
+			event->pending_disable = 0;
+
+		/*
+		 * Already running, nothing to do.
+		 */
+		if (event->state >= PERF_EVENT_STATE_INACTIVE)
+			return;
 	}
-	raw_spin_unlock_irq(&ctx->lock);
 
 	event_function_call(event, __perf_event_enable, NULL);
 }
@@ -7612,10 +7620,8 @@ static void __perf_pending_disable(struct perf_event *event)
 	 * Yay, we hit home and are in the context of the event.
 	 */
 	if (cpu == smp_processor_id()) {
-		if (event->pending_disable) {
-			event->pending_disable = 0;
+		if (event->pending_disable)
 			perf_event_disable_local(event);
-		}
 		return;
 	}
 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: perf AUX: race causes poll() hang
  2026-05-18  9:41 ` perf AUX: race causes poll() hang Peter Zijlstra
@ 2026-05-19  5:58   ` Adrian Hunter
  2026-05-19  9:11     ` Peter Zijlstra
  2026-06-14 22:40   ` Konstantin Mikhaylov
  1 sibling, 1 reply; 4+ messages in thread
From: Adrian Hunter @ 2026-05-19  5:58 UTC (permalink / raw)
  To: Peter Zijlstra,
	Константин
	Михайлов
  Cc: Mark Rutland, linux-kernel

On 18/05/2026 12:41, Peter Zijlstra wrote:
> On Fri, May 15, 2026 at 01:35:48AM +0300, Константин Михайлов wrote:
>> Hello Peter and Adrian,
>>
>> I'd like to report a potential race condition in perf AUX buffer handling.
>>
>> AUX tracing is designed to allow the tracee continue running when the AUX
>> buffer fills. The PMU driver must disable tracing when AUX buffer is full.
>> Typically, it schedules IRQ work to disable the event later. Meanwhile, a
>> typical tracer's workflow looks like: poll() on perf FDs, consume the data,
>> re-enable the event via PERF_EVENT_IOC_ENABLE ioctl(), then poll() again.
>>
>> Given this, the following race is possible:
>> -------------------
>> | CPU #0 | CPU #1 |
>> | tracee | tracer |
>> -------------------
>> |   **   |        |   tracee fills the AUX buffer completely with some data
>> -------------------
>> |   **   |        |   PMU driver updates aux_head accordingly and schedules IRQ works
>> |   **   |        |   to disable the event and wake up the tracer (setting rb->poll in
>> |   **   |        |   perf_output_wakeup() along the way)
>> -------------------
>> |        |   **   |   tracer consumes all the data from AUX buffer,
>> |        |   **   |   thus clears rb->poll in perf_poll()
>> -------------------
>> |        |   **   |   tracer re-enables the tracing (the event is still active,
>> |        |   **   |   so ioctl(...) returns immediately)
>> -------------------
>> |        |   **   |   tracer starts poll()'ing the AUX buffer again
>> -------------------
>> |   **   |        |   IRQ work handler finally disables the event and
>> |   **   |        |   wakes up tracer
>> -------------------
>> |        |   **   |   tracer obtains zero rb->poll and continues polling
>> -------------------
>> As a result, tracee runs without PMU tracing, and tracer's poll() will
>> never be woken up unless it has some timeout.
>>
>> I reproduced this on an x86 machine with intel_pt and kernel v6.17.
>> Reproducing this race on the vanilla kernel is timing-sensitive, so I added
>> 30 ms delay in the error path in intel_pt_interrupt() when
>> pt_buffer_reset_markers() returns an error - this delay widens the window
>> between aux_head update and actual event disable in IRQ work handler. I'm
>> not sure that pt_buffer_reset_markers()'s error means that buffer
>> overflowed, but this error branch is taken sometimes and all needed IRQ
>> works are scheduled during a call to perf_aux_output_end(). I also added 3
>> ms delay in perf in __auxtrace_mmap__read() before itr->read_finish(),
>> ensuring the ioctl() falls into that window. With these changes, some perf
>> runs collected smaller traces than usual. I added traceprints to intel_pt
>> driver and enabled tracing for sys_poll and sys_ioctl, which confirmed the
>> exact sequence described above. The problem was sometimes mitigated by
>> tracee migration to another cpu (as perf creates an event for every cpu,
>> the event is re-enabled by kernel when it is set on a new cpu). Otherwise,
>> tracee stayed on the same cpu and tracer hung on poll() until tracee exited.
>>
>> Could you please confirm if this analysis is correct? Should we move
>> setting of rb->poll *after* the event is disabled in IRQ work handler?
> 
> I *think* (its been a minute since I looked at this code), that you're
> right.
> 
> Does something like the below cure things?
> 
> ---
>  kernel/events/core.c | 54 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 7935d5663944..490407618f36 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2677,6 +2677,9 @@ static void __perf_event_disable(struct perf_event *event,
>  				 struct perf_event_context *ctx,
>  				 void *info)
>  {
> +	if (event->pending_disable)
> +		event->pending_disable = 0;
> +
>  	if (event->state < PERF_EVENT_STATE_INACTIVE)
>  		return;
>  
> @@ -3278,32 +3281,37 @@ static void _perf_event_enable(struct perf_event *event)
>  {
>  	struct perf_event_context *ctx = event->ctx;
>  
> -	raw_spin_lock_irq(&ctx->lock);
> -	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
> -	    event->state <  PERF_EVENT_STATE_ERROR) {
> -out:
> -		raw_spin_unlock_irq(&ctx->lock);
> -		return;
> -	}
> +	scoped_guard (raw_spinlock_irq, &ctx->lock) {
> +		if (event->state <  PERF_EVENT_STATE_ERROR)
> +			return;
>  
> -	/*
> -	 * If the event is in error state, clear that first.
> -	 *
> -	 * That way, if we see the event in error state below, we know that it
> -	 * has gone back into error state, as distinct from the task having
> -	 * been scheduled away before the cross-call arrived.
> -	 */
> -	if (event->state == PERF_EVENT_STATE_ERROR) {
>  		/*
> -		 * Detached SIBLING events cannot leave ERROR state.
> +		 * If the event is in error state, clear that first.
> +		 *
> +		 * That way, if we see the event in error state below, we know that it
> +		 * has gone back into error state, as distinct from the task having
> +		 * been scheduled away before the cross-call arrived.
>  		 */
> -		if (event->event_caps & PERF_EV_CAP_SIBLING &&
> -		    event->group_leader == event)
> -			goto out;
> +		if (event->state == PERF_EVENT_STATE_ERROR) {
> +			/*
> +			 * Detached SIBLING events cannot leave ERROR state.
> +			 */
> +			if (event->event_caps & PERF_EV_CAP_SIBLING &&
> +			    event->group_leader == event)
> +				return;
>  
> -		event->state = PERF_EVENT_STATE_OFF;
> +			event->state = PERF_EVENT_STATE_OFF;
> +		}
> +
> +		if (event->pending_disable)
> +			event->pending_disable = 0;
> +
> +		/*
> +		 * Already running, nothing to do.
> +		 */
> +		if (event->state >= PERF_EVENT_STATE_INACTIVE)
> +			return;
>  	}
> -	raw_spin_unlock_irq(&ctx->lock);
>  
>  	event_function_call(event, __perf_event_enable, NULL);
>  }
> @@ -7612,10 +7620,8 @@ static void __perf_pending_disable(struct perf_event *event)
>  	 * Yay, we hit home and are in the context of the event.
>  	 */
>  	if (cpu == smp_processor_id()) {
> -		if (event->pending_disable) {
> -			event->pending_disable = 0;
> +		if (event->pending_disable)

At this point, can _perf_event_enable() on another CPU decide
to do nothing ("Already running, nothing to do"), but then the
disable below contradicts that?

>  			perf_event_disable_local(event);
> -		}
>  		return;
>  	}
>  


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: perf AUX: race causes poll() hang
  2026-05-19  5:58   ` Adrian Hunter
@ 2026-05-19  9:11     ` Peter Zijlstra
  0 siblings, 0 replies; 4+ messages in thread
From: Peter Zijlstra @ 2026-05-19  9:11 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Константин
	Михайлов,
	Mark Rutland, linux-kernel

On Tue, May 19, 2026 at 08:58:52AM +0300, Adrian Hunter wrote:

> > @@ -7612,10 +7620,8 @@ static void __perf_pending_disable(struct perf_event *event)
> >  	 * Yay, we hit home and are in the context of the event.
> >  	 */
> >  	if (cpu == smp_processor_id()) {
> > -		if (event->pending_disable) {
> > -			event->pending_disable = 0;
> > +		if (event->pending_disable)
> 
> At this point, can _perf_event_enable() on another CPU decide
> to do nothing ("Already running, nothing to do"), but then the
> disable below contradicts that?

Should be serialized on ctx->lock IIRC

> >  			perf_event_disable_local(event);
> > -		}
> >  		return;
> >  	}
> >  
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: perf AUX: race causes poll() hang
  2026-05-18  9:41 ` perf AUX: race causes poll() hang Peter Zijlstra
  2026-05-19  5:58   ` Adrian Hunter
@ 2026-06-14 22:40   ` Konstantin Mikhaylov
  1 sibling, 0 replies; 4+ messages in thread
From: Konstantin Mikhaylov @ 2026-06-14 22:40 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: adrian.hunter, mark.rutland, linux-kernel

On 5/18/26 12:41 PM, Peter Zijlstra wrote:
> On Fri, May 15, 2026 at 01:35:48AM +0300, Константин Михайлов wrote:
>> Hello Peter and Adrian,
>>
>> I'd like to report a potential race condition in perf AUX buffer handling.
>>
>> AUX tracing is designed to allow the tracee continue running when the AUX
>> buffer fills. The PMU driver must disable tracing when AUX buffer is full.
>> Typically, it schedules IRQ work to disable the event later. Meanwhile, a
>> typical tracer's workflow looks like: poll() on perf FDs, consume the data,
>> re-enable the event via PERF_EVENT_IOC_ENABLE ioctl(), then poll() again.
>>
>> Given this, the following race is possible:
>> -------------------
>> | CPU #0 | CPU #1 |
>> | tracee | tracer |
>> -------------------
>> |   **   |        |   tracee fills the AUX buffer completely with some data
>> -------------------
>> |   **   |        |   PMU driver updates aux_head accordingly and schedules IRQ works
>> |   **   |        |   to disable the event and wake up the tracer (setting rb->poll in
>> |   **   |        |   perf_output_wakeup() along the way)
>> -------------------
>> |        |   **   |   tracer consumes all the data from AUX buffer,
>> |        |   **   |   thus clears rb->poll in perf_poll()
>> -------------------
>> |        |   **   |   tracer re-enables the tracing (the event is still active,
>> |        |   **   |   so ioctl(...) returns immediately)
>> -------------------
>> |        |   **   |   tracer starts poll()'ing the AUX buffer again
>> -------------------
>> |   **   |        |   IRQ work handler finally disables the event and
>> |   **   |        |   wakes up tracer
>> -------------------
>> |        |   **   |   tracer obtains zero rb->poll and continues polling
>> -------------------
>> As a result, tracee runs without PMU tracing, and tracer's poll() will
>> never be woken up unless it has some timeout.
>>
>> I reproduced this on an x86 machine with intel_pt and kernel v6.17.
>> Reproducing this race on the vanilla kernel is timing-sensitive, so I added
>> 30 ms delay in the error path in intel_pt_interrupt() when
>> pt_buffer_reset_markers() returns an error - this delay widens the window
>> between aux_head update and actual event disable in IRQ work handler. I'm
>> not sure that pt_buffer_reset_markers()'s error means that buffer
>> overflowed, but this error branch is taken sometimes and all needed IRQ
>> works are scheduled during a call to perf_aux_output_end(). I also added 3
>> ms delay in perf in __auxtrace_mmap__read() before itr->read_finish(),
>> ensuring the ioctl() falls into that window. With these changes, some perf
>> runs collected smaller traces than usual. I added traceprints to intel_pt
>> driver and enabled tracing for sys_poll and sys_ioctl, which confirmed the
>> exact sequence described above. The problem was sometimes mitigated by
>> tracee migration to another cpu (as perf creates an event for every cpu,
>> the event is re-enabled by kernel when it is set on a new cpu). Otherwise,
>> tracee stayed on the same cpu and tracer hung on poll() until tracee exited.
>>
>> Could you please confirm if this analysis is correct? Should we move
>> setting of rb->poll *after* the event is disabled in IRQ work handler?
> 
> I *think* (its been a minute since I looked at this code), that you're
> right.
> 
> Does something like the below cure things?
> 
> ---
>   kernel/events/core.c | 54 +++++++++++++++++++++++++++++-----------------------
>   1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 7935d5663944..490407618f36 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2677,6 +2677,9 @@ static void __perf_event_disable(struct perf_event *event,
>   				 struct perf_event_context *ctx,
>   				 void *info)
>   {
> +	if (event->pending_disable)
> +		event->pending_disable = 0;
> +
>   	if (event->state < PERF_EVENT_STATE_INACTIVE)
>   		return;
>   
> @@ -3278,32 +3281,37 @@ static void _perf_event_enable(struct perf_event *event)
>   {
>   	struct perf_event_context *ctx = event->ctx;
>   
> -	raw_spin_lock_irq(&ctx->lock);
> -	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
> -	    event->state <  PERF_EVENT_STATE_ERROR) {
> -out:
> -		raw_spin_unlock_irq(&ctx->lock);
> -		return;
> -	}
> +	scoped_guard (raw_spinlock_irq, &ctx->lock) {
> +		if (event->state <  PERF_EVENT_STATE_ERROR)
> +			return;
>   
> -	/*
> -	 * If the event is in error state, clear that first.
> -	 *
> -	 * That way, if we see the event in error state below, we know that it
> -	 * has gone back into error state, as distinct from the task having
> -	 * been scheduled away before the cross-call arrived.
> -	 */
> -	if (event->state == PERF_EVENT_STATE_ERROR) {
>   		/*
> -		 * Detached SIBLING events cannot leave ERROR state.
> +		 * If the event is in error state, clear that first.
> +		 *
> +		 * That way, if we see the event in error state below, we know that it
> +		 * has gone back into error state, as distinct from the task having
> +		 * been scheduled away before the cross-call arrived.
>   		 */
> -		if (event->event_caps & PERF_EV_CAP_SIBLING &&
> -		    event->group_leader == event)
> -			goto out;
> +		if (event->state == PERF_EVENT_STATE_ERROR) {
> +			/*
> +			 * Detached SIBLING events cannot leave ERROR state.
> +			 */
> +			if (event->event_caps & PERF_EV_CAP_SIBLING &&
> +			    event->group_leader == event)
> +				return;
>   
> -		event->state = PERF_EVENT_STATE_OFF;
> +			event->state = PERF_EVENT_STATE_OFF;
> +		}
> +
> +		if (event->pending_disable)
> +			event->pending_disable = 0;
> +
> +		/*
> +		 * Already running, nothing to do.
> +		 */
> +		if (event->state >= PERF_EVENT_STATE_INACTIVE)
> +			return;
>   	}
> -	raw_spin_unlock_irq(&ctx->lock);
>   
>   	event_function_call(event, __perf_event_enable, NULL);
>   }
> @@ -7612,10 +7620,8 @@ static void __perf_pending_disable(struct perf_event *event)
>   	 * Yay, we hit home and are in the context of the event.
>   	 */
>   	if (cpu == smp_processor_id()) {
> -		if (event->pending_disable) {
> -			event->pending_disable = 0;
> +		if (event->pending_disable)
>   			perf_event_disable_local(event);
> -		}
>   		return;
>   	}
>   

Sorry, I'm re-sending this response because my previous email (sent 3+ 
weeks ago) was not properly formatted as plain text and was not accepted 
by lkml.org. To ensure this reaches the thread, I'm re-sending it 
correctly formatted now.

I tested this patch with the same delay on the error path in intel_pt 
driver. With this patch, the hardware tracing sometimes remains stopped 
until the next reschedule. The tracer does not re-enable the event when 
event->state >= PERF_EVENT_STATE_INACTIVE, since this condition is true 
while the driver is disabling hardware tracing. As a result, hardware 
tracing can stay disabled until the next reschedule even when the tracer 
has drained all data from the AUX buffer.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-06-14 22:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CAE7+JAnhZSuMMG9+0pZ2G6ZBR-OAC1hHptkr=T=x5LK9n19vOQ@mail.gmail.com>
2026-05-18  9:41 ` perf AUX: race causes poll() hang Peter Zijlstra
2026-05-19  5:58   ` Adrian Hunter
2026-05-19  9:11     ` Peter Zijlstra
2026-06-14 22:40   ` Konstantin Mikhaylov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome