lkml.org 
[lkml]   [2010]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 5/6] perf: Fix race in callchains
On Fri, Jul 02, 2010 at 08:07:35PM +0200, Peter Zijlstra wrote:
> On Thu, 2010-07-01 at 17:36 +0200, Frederic Weisbecker wrote:
> > Now that software events don't have interrupt disabled anymore in
> > the event path, callchains can nest on any context. So seperating
> > nmi and others contexts in two buffers has become racy.
> >
> > Fix this by providing one buffer per nesting level. Given the size
> > of the callchain entries (2040 bytes * 4), we now need to allocate
> > them dynamically.
>
> OK so I guess you want to allocate them because 8k per cpu is too much
> to always have about?



Right. I know that really adds complexity and I hesitated much before
doing so. But I think that's quite necessary.



> > +static int get_callchain_buffers(void)
> > +{
> > + int i;
> > + int err = 0;
> > + struct perf_callchain_entry_cpus *buf;
> > +
> > + mutex_lock(&callchain_mutex);
> > +
> > + if (WARN_ON_ONCE(++nr_callchain_events < 1)) {
> > + err = -EINVAL;
> > + goto exit;
> > + }
> > +
> > + if (nr_callchain_events > 1)
> > + goto exit;
> > +
> > + for (i = 0; i < 4; i++) {
> > + buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> > + /* free_event() will clean the rest */
> > + if (!buf) {
> > + err = -ENOMEM;
> > + goto exit;
> > + }
> > + buf->entries = alloc_percpu(struct perf_callchain_entry);
> > + if (!buf->entries) {
> > + kfree(buf);
> > + err = -ENOMEM;
> > + goto exit;
> > + }
> > + rcu_assign_pointer(callchain_entries[i], buf);
> > + }
> > +
> > +exit:
> > + mutex_unlock(&callchain_mutex);
> > +
> > + return err;
> > +}
>
> > +static void put_callchain_buffers(void)
> > +{
> > + int i;
> > + struct perf_callchain_entry_cpus *entry;
> > +
> > + mutex_lock(&callchain_mutex);
> > +
> > + if (WARN_ON_ONCE(--nr_callchain_events < 0))
> > + goto exit;
> > +
> > + if (nr_callchain_events > 0)
> > + goto exit;
> > +
> > + for (i = 0; i < 4; i++) {
> > + entry = callchain_entries[i];
> > + if (entry) {
> > + callchain_entries[i] = NULL;
> > + call_rcu(&entry->rcu_head, release_callchain_buffers);
> > + }
> > + }
> > +
> > +exit:
> > + mutex_unlock(&callchain_mutex);
> > +}
>
> If you make nr_callchain_events an atomic_t, then you can do the
> refcounting outside the mutex. See the existing user of
> atomic_dec_and_mutex_lock().
>
> I would also split it in get/put and alloc/free functions for clarity.



Ok I will.




> I'm not at all sure why you're using RCU though.
>
> > @@ -1895,6 +2072,8 @@ static void free_event(struct perf_event *event)
> > atomic_dec(&nr_comm_events);
> > if (event->attr.task)
> > atomic_dec(&nr_task_events);
> > + if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
> > + put_callchain_buffers();
> > }
> >
> > if (event->buffer) {
>
> If this was the last even, there's no callchain user left, so nobody can
> be here:
>
> > @@ -3480,14 +3610,20 @@ static void perf_event_output(struct perf_event *event, int nmi,
> > struct perf_output_handle handle;
> > struct perf_event_header header;
> >
> > + /* protect the callchain buffers */
> > + rcu_read_lock();
> > +
> > perf_prepare_sample(&header, data, event, regs);
> >
> > if (perf_output_begin(&handle, event, header.size, nmi, 1))
> > - return;
> > + goto exit;
> >
> > perf_output_sample(&handle, &header, data, event);
> >
> > perf_output_end(&handle);
> > +
> > +exit:
> > + rcu_read_unlock();
> > }
>
> Rendering that RCU stuff superfluous.


May be I'm omitting something that would make it non-rcu-safe.

But consider a perf event running on CPU 1. And you close the fd on
CPU 0. CPU 1 has started to use a callchain buffer but receives an IPI
to retire the event from the cpu. But still it has yet to finish his
callchain processing.

If right after that CPU 0 releases the callchain buffers, CPU 1 may
crash in the middle.

So you need to wait for the grace period to end.



\
 
 \ /
  Last update: 2010-07-03 22:31    [W:0.053 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site