Re: [PATCH] perf: Prevent recursion in ring buffer

[Jiri Olsa]

On Thu, Sep 13, 2018 at 09:07:40AM +0200, Peter Zijlstra wrote:
> On Wed, Sep 12, 2018 at 09:33:17PM +0200, Jiri Olsa wrote:
> > Some of the scheduling tracepoints allow the perf_tp_event
> > code to write to ring buffer under different cpu than the
> > code is running on.
> 
> ARGH.. that is indeed borken.
> 
> > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> > index 4a9937076331..0c976ac414c5 100644
> > --- a/kernel/events/ring_buffer.c
> > +++ b/kernel/events/ring_buffer.c
> > @@ -101,6 +101,7 @@ static void perf_output_put_handle(struct perf_output_handle *handle)
> >  
> >  out:
> >  	preempt_enable();
> > +	atomic_set(&rb->recursion, 0);
> >  }
> >  
> >  static __always_inline bool
> > @@ -145,6 +146,12 @@ __perf_output_begin(struct perf_output_handle *handle,
> >  		goto out;
> >  	}
> >  
> > +	if (atomic_cmpxchg(&rb->recursion, 0, 1) != 0) {
> > +		if (rb->nr_pages)
> > +			local_inc(&rb->lost);
> > +		goto out;
> > +	}
> > +
> >  	handle->rb    = rb;
> >  	handle->event = event;
> >  
> > @@ -286,6 +293,7 @@ ring_buffer_init(struct ring_buffer *rb, long watermark, int flags)
> >  		rb->overwrite = 1;
> >  
> >  	atomic_set(&rb->refcount, 1);
> > +	atomic_set(&rb->recursion, 0);
> >  
> >  	INIT_LIST_HEAD(&rb->event_list);
> >  	spin_lock_init(&rb->event_lock);
> 
> That's not a recursion count, that's a test-and-set spinlock, and you
> got the memory ordering wrong for that.
> 
> Also, we tried very hard to avoid atomic ops in the ring-buffer and you
> just wrecked that. Worse, you wrecked previously working interrupt
> nesting output.

ah.. the interrupt will also bail out now.. right :-\

> 
> Let me have a look at this.
>