Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753824AbcC3B5c (ORCPT ); Tue, 29 Mar 2016 21:57:32 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:36449 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751701AbcC3B5a (ORCPT ); Tue, 29 Mar 2016 21:57:30 -0400 Subject: Re: [PATCH 1/4] perf core: Introduce new ioctl options to pause and resume ring buffer To: Peter Zijlstra References: <1459147292-239310-1-git-send-email-wangnan0@huawei.com> <1459147292-239310-2-git-send-email-wangnan0@huawei.com> <20160329125423.GJ3408@twins.programming.kicks-ass.net> CC: Alexei Starovoitov , Arnaldo Carvalho de Melo , , Brendan Gregg , He Kuang , Jiri Olsa , Masami Hiramatsu , Namhyung Kim , , Zefan Li From: "Wangnan (F)" Message-ID: <56FB326E.9030909@huawei.com> Date: Wed, 30 Mar 2016 09:57:02 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <20160329125423.GJ3408@twins.programming.kicks-ass.net> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.66.109] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090204.56FB327C.029E,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 5db26b45a7e34ae5cfa8aa03938e343e Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2237 Lines: 73 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 > 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)