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

[Alexander Shishkin <alexander.shishkin@linux.intel.com>]

Peter Zijlstra <peterz@infradead.org> writes:

> 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.

Fair enough. Then I'd like to disable the ACTIVE ones before freeing AUX
stuff and then re-enabling them since perf_event_{en,dis}able() already
provide the convenient cross-cpu calls, which would also avoid
concurrency between pmu::{add,del} callbacks and this unmap path. Makes
sense?

Regards,
--
Alex