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

[Jiri Olsa]

On Thu, Sep 18, 2014 at 09:07:38PM +0400, Alexander Yarygin wrote:
> From: David Ahern <dsahern@xxxxxxxxx>
> 
> When processing events the session code has an ordered samples queue which is
> used to time-sort events coming in across multiple mmaps. At a later point in
> time samples on the queue are flushed up to some timestamp at which point the
> event is actually processed.
> 
> When analyzing events live (ie., record/analysis path in the same command)
> there is a race that leads to corrupted events and parse errors which cause
> perf to terminate. The problem is that when the event is placed in the ordered
> samples queue it is only a reference to the event which is really sitting in
> the mmap buffer. Even though the event is queued for later processing the mmap
> tail pointer is updated which indicates to the kernel that the event has been
> processed. The race is flushing the event from the queue before it gets
> overwritten by some other event. For commands trying to process events live
> (versus just writing to a file) and processing a high rate of events this leads
> to parse failures and perf terminates.
> 
> Examples hitting this problem are 'perf kvm stat live', especially with nested
> VMs which generate 100,000+ traces per second, and a command processing
> scheduling events with a high rate of context switching -- e.g., running
> 'perf bench sched pipe'.
> 
> This patch offers live commands an option to copy the event when it is placed in
> the ordered samples queue.
> 
> Signed-off-by: David Ahern <dsahern@xxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> Cc: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: Mike Galbraith <efault@xxxxxx>
> Cc: Namhyung Kim <namhyung.kim@xxxxxxx>
> Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> Cc: Stephane Eranian <eranian@xxxxxxxxxx>
> Signed-off-by: Alexander Yarygin <yarygin@xxxxxxxxxxxxxxxxxx>
> ---
>  tools/perf/util/ordered-events.c | 12 +++++++++---
>  tools/perf/util/ordered-events.h |  2 +-
>  tools/perf/util/session.c        | 12 +++++++++---
>  tools/perf/util/session.h        |  1 +
>  4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
> index 706ce1a..5b51de5 100644
> --- a/tools/perf/util/ordered-events.c
> +++ b/tools/perf/util/ordered-events.c
> @@ -140,11 +140,15 @@ static int __ordered_events__flush(struct perf_session *s,
>  			break;
>  
>  		ret = perf_evlist__parse_sample(s->evlist, iter->event, &sample);
> -		if (ret)
> +		if (ret) {
>  			pr_err("Can't parse sample, err = %d\n", ret);
> -		else {
> +			if (s->copy_on_queue)
> +				free(iter->event);
> +		} else {
>  			ret = perf_session__deliver_event(s, iter->event, &sample, tool,
>  							  iter->file_offset);
> +			if (s->copy_on_queue)
> +				free(iter->event);
>  			if (ret)
>  				return ret;
>  		}
> @@ -233,13 +237,15 @@ void ordered_events__init(struct ordered_events *oe)
>  	oe->cur_alloc_size = 0;
>  }
>  
> -void ordered_events__free(struct ordered_events *oe)
> +void ordered_events__free(struct perf_session *s, struct ordered_events *oe)
>  {
>  	while (!list_empty(&oe->to_free)) {
>  		struct ordered_event *event;
>  
>  		event = list_entry(oe->to_free.next, struct ordered_event, list);
>  		list_del(&event->list);
> +		if (s->copy_on_queue)
> +			free(event->event);
>  		free(event);
>  	}
>  }
> diff --git a/tools/perf/util/ordered-events.h b/tools/perf/util/ordered-events.h
> index 3b2f205..a156b0e 100644
> --- a/tools/perf/util/ordered-events.h
> +++ b/tools/perf/util/ordered-events.h
> @@ -41,7 +41,7 @@ void ordered_events__delete(struct ordered_events *oe, struct ordered_event *eve
>  int ordered_events__flush(struct perf_session *s, struct perf_tool *tool,
>  			  enum oe_flush how);
>  void ordered_events__init(struct ordered_events *oe);
> -void ordered_events__free(struct ordered_events *oe);
> +void ordered_events__free(struct perf_session *s, struct ordered_events *oe);
>  
>  static inline
>  void ordered_events__set_alloc_size(struct ordered_events *oe, u64 size)
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 6d2d50d..40d3aec 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -542,7 +542,13 @@ int perf_session_queue_event(struct perf_session *s, union perf_event *event,
>  		return -ENOMEM;
>  
>  	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);

we have memdup, and you need to check for allocation failure

hum.. how about allocation limits?

Now have report.queue-size to keep track and limit the ordered_events
memory size. I think we should add this allocation size under this
limit as well.

> +	} else
> +		new->event = event;
> +

also the copy_on_queue flag and the logic above feels more like
it belongs to 'ordered_events' object to me

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/