On Wed, May 07, 2014 at 08:06:21PM +0200, Peter Zijlstra wrote:
> On Wed, May 07, 2014 at 02:35:26PM +0200, Peter Zijlstra wrote:
> > static void ring_buffer_attach(struct perf_event *event,
> > struct ring_buffer *rb)
> > {
> > + struct ring_buffer *old_rb = NULL;
> > unsigned long flags;
> >
> > + if (event->rb) {
> > + /*
> > + * Should be impossible, we set this when removing
> > + * event->rb_entry and wait/clear when adding event->rb_entry.
> > + */
> > + WARN_ON_ONCE(event->rcu_pending);
> >
> > + old_rb = event->rb;
> > + event->rcu_batches = get_state_synchronize_rcu();
> > + event->rcu_pending = 1;
> >
> > + spin_lock_irqsave(&rb->event_lock, flags);
> > + list_del_rcu(&event->rb_entry);
> > + spin_unlock_irqrestore(&rb->event_lock, flags);
>
> This all works a whole lot better if you make that old_rb->event_lock.
>
> > + }
> >
> > + if (event->rcu_pending && rb) {
> > + cond_synchronize_rcu(event->rcu_batches);
There is not a whole lot of code between the get_state_synchronize_rcu()
and the cond_synchronize_rcu(), so I would expect this to do a
synchronize_rcu() almost all the time. Or am I missing something here?
Thanx, Paul
> > + event->rcu_pending = 0;
> > + }
> >
> > + if (rb) {
> > + spin_lock_irqsave(&rb->event_lock, flags);
> > + list_add_rcu(&event->rb_entry, &rb->event_list);
> > + spin_unlock_irqrestore(&rb->event_lock, flags);
> > + }
> > +
> > + rcu_assign_pointer(event->rb, rb);
> > +
> > + if (old_rb) {
> > + ring_buffer_put(old_rb);
> > + /*
> > + * Since we detached before setting the new rb, so that we
> > + * could attach the new rb, we could have missed a wakeup.
> > + * Provide it now.
> > + */
> > + wake_up_all(&event->waitq);
> > + }
> > }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
