Messages in this thread | | | From | Alexander Shishkin <> | Subject | Re: [PATCH v4 02/22] perf: Add AUX area to ring buffer for raw data streams | Date | Mon, 08 Sep 2014 15:55:11 +0300 |
| |
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
| |