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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
