Messages in this thread | | | Subject | Re: [PATCH 1/4] perf core: Introduce new ioctl options to pause and resume ring buffer | From | "Wangnan (F)" <> | Date | Wed, 30 Mar 2016 09:57:02 +0800 |
| |
On 2016/3/29 20:54, Peter Zijlstra wrote: > On Mon, Mar 28, 2016 at 06:41:29AM +0000, Wang Nan wrote: >> Add new ioctl() to pause/resume ring-buffer output. >> >> In some situations we want to read from ring buffer only when we >> ensure nothing can write to the ring buffer during reading. Without >> this patch we have to turn off all events attached to this ring buffer >> to achieve this. >> >> This patch is for supporting overwrite ring buffer. Following >> commits will introduce new methods support reading from overwrite ring >> buffer. Before reading, caller must ensure the ring buffer is frozen, or >> the reading is unreliable. >> >> Signed-off-by: Wang Nan <wangnan0@huawei.com> > I made the below changes.
Can I add your SOB when I resend it?
> --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -4346,7 +4346,7 @@ static long _perf_ioctl(struct perf_even > > rcu_read_lock(); > rb = rcu_dereference(event->rb); > - if (!event->rb) { > + if (!event->rb || !event->nr_pages) { > rcu_read_unlock(); > return -EINVAL; > } > --- a/kernel/events/internal.h > +++ b/kernel/events/internal.h > @@ -66,9 +66,7 @@ static inline void rb_free_rcu(struct rc > rb_free(rb); > } > > -static inline void > -rb_toggle_paused(struct ring_buffer *rb, > - bool pause) > +static inline void rb_toggle_paused(struct ring_buffer *rb, bool pause) > { > if (!pause && rb->nr_pages) > rb->paused = 0; > --- a/kernel/events/ring_buffer.c > +++ b/kernel/events/ring_buffer.c > @@ -248,7 +248,12 @@ ring_buffer_init(struct ring_buffer *rb, > spin_lock_init(&rb->event_lock); > init_irq_work(&rb->irq_work, rb_irq_work); > > - rb->paused = rb->nr_pages ? 0 : 1; > + /* > + * perf_output_begin() only checks rb->paused, therefore > + * rb->paused must be true if we have no pages for output. > + */ > + if (!rb->nr_pages) > + rb->paused = 1; > }
I still think we need to explicitly set rb->paused to 0 when rb->nr_pages is non-zero to avoid further improvement re-init an old 'struct ring_buffer':
rb->paused = 0; if (unlikely(!rb->nr_pages)) rb->paused = 1;
Thank you.
> > static void ring_buffer_put_async(struct ring_buffer *rb)
| |