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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87a96auv9s.fsf@ashishki-desk.ger.corp.intel.com>
Date:	Mon, 08 Sep 2014 15:55:11 +0300
From:	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
	Robert Richter <rric@...nel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Mike Galbraith <efault@....de>,
	Paul Mackerras <paulus@...ba.org>,
	Stephane Eranian <eranian@...gle.com>,
	Andi Kleen <ak@...ux.intel.com>, kan.liang@...el.com
Subject: Re: [PATCH v4 02/22] perf: Add AUX area to ring buffer for raw data streams

Peter Zijlstra <peterz@...radead.org> writes:

> On Mon, Sep 08, 2014 at 02:16:56PM +0300, Alexander Shishkin wrote:
>> Peter Zijlstra <peterz@...radead.org> writes:
>> 
>> > On Wed, Aug 20, 2014 at 03:35:59PM +0300, Alexander Shishkin wrote:
>> >> From: Peter Zijlstra <peterz@...radead.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@...ux.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/