Re: [PATCH] perf session: Add option to copy events when queueing

[Frederic Weisbecker <fweisbec@gmail.com>]

On Sat, Sep 14, 2013 at 11:25:40AM -0600, David Ahern wrote:
> On 9/14/13 10:16 AM, Frederic Weisbecker wrote:
> >>@@ -676,7 +682,12 @@ int perf_session_queue_event(struct perf_session *s, union perf_event *event,
> >>
> >>  	new->timestamp = timestamp;
> >>  	new->file_offset = file_offset;
> >>-	new->event = event;
> >>+
> >>+	if (s->copy_on_queue) {
> >>+		new->event = malloc(event->header.size);
> >>+		memcpy(new->event, event, event->header.size);
> >>+	} else
> >>+		new->event = event;
> 
> ---8<---
> 
> >So do you think it should stay optional? This looks like a global problem, I mean
> >the event can be unmapped anytime for any builtin tool mapping it, right?
> 
> Yes. I could make it the default behavior; just overhead in doing
> that (malloc/copy for each event).

Are there any tool that don't suffer from this bug somehow? If not then it must
be applied unconditionally.

> 
> >
> >Also we already allocate the sample list node (struct sample_queue) from os->sample
> >buffer. ie: we have our own allocator there.
> >
> >Probably we should reuse that and include the copied event space in "struct sample_queue"?
> 
> 
> Right, that's where I put the malloc and copy - I kept the relevant
> change above. I take it you are thinking of something different but
> I am not following you. You definitely do NOT want to change struct
> sample_queue to include an event - like this:
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 51f5edf..866944a 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -491,7 +491,7 @@ static perf_event__swap_op perf_event__swap_ops[] = {
>  struct sample_queue {
>     u64         timestamp;
>     u64         file_offset;
> -   union perf_event    *event;
> +   union perf_event    event;

Right that's roughly what I thought.

>     struct list_head    list;
>  };
> 
> size of event is determined by mmap_event (mmap2_event in latest
> code) which is > 4096 because of the filename argument. Including
> the event directly in sample_queue would balloon memory usage
> (learned this the hard way!).

Ah then perhaps we can allocate with the dynamic size of the event?

> 
> >
> >Also looking at it now, it seems we have a bug on the existing code:
> >
> >
> >         if (!list_empty(sc)) {
> >                 new = list_entry(sc->next, struct sample_queue, list);
> >                 list_del(&new->list);
> >         } else if (os->sample_buffer) {
> >                 new = os->sample_buffer + os->sample_buffer_idx;
> >                 if (++os->sample_buffer_idx == MAX_SAMPLE_BUFFER)
> >                         os->sample_buffer = NULL;
> >         } else {
> >                os->sample_buffer = malloc(MAX_SAMPLE_BUFFER * sizeof(*new));
> >                if (!os->sample_buffer)
> >                         return -ENOMEM;
> >                list_add(&os->sample_buffer->list, &os->to_free);
> >                os->sample_buffer_idx = 2;
> >                new = os->sample_buffer + 1;
> >         }
> >
> >If we actually run out of buffer rooms, we should realloc right after and not
> >wait for the next entry, otherwise we loose an event:
> >
> >         if (!list_empty(sc)) {
> >                 new = list_entry(sc->next, struct sample_queue, list);
> >                 list_del(&new->list);
> >         } else {
> >                 if (os->sample_buffer) {
> >                         new = os->sample_buffer + os->sample_buffer_idx;
> >                         if (++os->sample_buffer_idx == MAX_SAMPLE_BUFFER)
> >                                 os->sample_buffer = NULL;
> >                 }
> >
> >                 if (!os->sample_buffer) {
> >	                os->sample_buffer = malloc(MAX_SAMPLE_BUFFER * sizeof(*new));
> >                         if (!os->sample_buffer)
> >                                 return -ENOMEM;
> >                         list_add(&os->sample_buffer->list, &os->to_free);
> >                         os->sample_buffer_idx = 2;
> >                         new = os->sample_buffer + 1;
> >         }
> >
> >
> >Although the mirrored os->sample_buffer condition check is a bit ugly and should move to
> >a function. But the idea is there.
> 
> Ok. That should be a separate patch. Are you going to submit that one?

Yeah, unless you beat me at it :)