Re: [PATCH v4 02/22] perf: Add AUX area to ring buffer for raw data streams

From: Peter Zijlstra <peterz@infradead.org>
To: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>,
	linux-kernel@vger.kernel.org, Robert Richter <rric@kernel.org>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Mike Galbraith <efault@gmx.de>, Paul Mackerras <paulus@samba.org>,
	Stephane Eranian <eranian@google.com>,
	Andi Kleen <ak@linux.intel.com>,
	kan.liang@intel.com
Subject: Re: [PATCH v4 02/22] perf: Add AUX area to ring buffer for raw data streams
Date: Mon, 8 Sep 2014 13:34:22 +0200	[thread overview]
Message-ID: <20140908113422.GD6758@twins.programming.kicks-ass.net> (raw)
In-Reply-To: <87lhpuuztj.fsf@ashishki-desk.ger.corp.intel.com>

[-- Attachment #1: Type: text/plain, Size: 2738 bytes --]

On Mon, Sep 08, 2014 at 02:16:56PM +0300, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > On Wed, Aug 20, 2014 at 03:35:59PM +0300, Alexander Shishkin wrote:
> >> From: Peter Zijlstra <peterz@infradead.org>
> >> 
> >> This patch introduces "AUX space" in the perf mmap buffer, intended for
> >> exporting high bandwidth data streams to userspace, such as instruction
> >> flow traces.
> >> 
> >> AUX space is a ring buffer, defined by aux_{offset,size} fields in the
> >> user_page structure, and read/write pointers aux_{head,tail}, which abide
> >> by the same rules as data_* counterparts of the main perf buffer.
> >> 
> >> In order to allocate/mmap AUX, userspace needs to set up aux_offset to
> >> such an offset that will be greater than data_offset+data_size and
> >> aux_size to be the desired buffer size. Both need to be page aligned.
> >> Then, same aux_offset and aux_size should be passed to mmap() call and
> >> if everything adds up, you should have an AUX buffer as a result.
> >> 
> >> Pages that are mapped into this buffer also come out of user's mlock
> >> rlimit plus perf_event_mlock_kb allowance.
> >> 
> >> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >> ---
> >
> >> +void rb_free_aux(struct ring_buffer *rb, struct perf_event *event)
> >> +{
> >> +	struct perf_event *iter;
> >> +	int pg;
> >> +
> >> +	if (rb->aux_priv) {
> >> +		/* disable all potential writers before freeing */
> >> +		rcu_read_lock();
> >> +		list_for_each_entry_rcu(iter, &rb->event_list, rb_entry)
> >> +			perf_event_disable(iter);
> >> +		rcu_read_unlock();
> >
> > Hmm, I cannot remember this from the last time; and its not explained
> > why this was added.
> >
> > This would change semantics between munmap of a buffer with and without
> > AUX bits in. 
> 
> Indeed, but this seems fair: if an event is generating AUX data while we
> unmap the AUX buffer, there is no reason to keep it on, because chances
> are, AUX data is the only thing this event is generating. The
> alternative would be to have a separate callback for this, but then
> again, that would race with other pmu callbacks, since it would be
> called from munmap() path. I should add an elaborate comment about this.
> 
> Does this sound reasonable?

Well, the same is true for regular buffers, if you munmap() them while
in use the events are pointless.

Still we keep them enabled, we simply skip generating output.

I would much like not to create deviating behaviour if you don't
absolutely have to. In both cases its an explicit user request, so you
don't need to go hold hands. He did it, he gets to keep whatever pieces
it generates.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]