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);
}
