Messages in this thread Patch in this message | | | Date | Sat, 14 Sep 2013 11:25:40 -0600 | From | David Ahern <> | Subject | Re: [PATCH] perf session: Add option to copy events when queueing |
| |
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).
> > 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; 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!). > > 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?
David
| |