Messages in this thread | | | Date | Sat, 3 Jul 2010 22:28:17 +0200 | From | Frederic Weisbecker <> | Subject | Re: [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.
| |