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.
> 
> Let me have a look at this.

I was first thinking to just leave it on the current cpu,
but not sure current users would be ok with that ;-)

jirka


---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index abaed4f8bb7f..9b534a2ecf17 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8308,6 +8308,8 @@ void perf_tp_event(u16 event_type, u64 count, void *record, int entry_size,
 				continue;
 			if (event->attr.config != entry->type)
 				continue;
+			if (event->cpu != smp_processor_id())
+				continue;
 			if (perf_tp_event_match(event, &data, regs))
 				perf_swevent_event(event, count, &data, regs);
 		}