Re: [PATCH v8 4/8] perf record: Introduce rec->overwrite_evlist for overwritable events

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Mon, Jun 20, 2016 at 10:47:21AM +0000, Wang Nan escreveu:
> Create an auxiliary evlist for overwritable events.
> 
> Before mmap, build this evlist and set 'overwrite' and 'backward'
> attribute. Since perf_evlist__mmap_ex() only maps events when
> evsel->overwrite matches evlist's corresponding attributes, with
> these two evlists an event goes to either rec->evlist or
> rec->overwrite_evlist.

Please try to make your patches more granular, for instance, in this
case you moved the call to perf_evlist__mmap_ex and all that block to a
function called record__mmap_evlist(), I had to check if you had
introduced any change in that block of code.

Instead you could've made a prep change, one that introduced
record__mmap_evlist() with that block, making clear in the changeset
that no change was introduced, I would check that anyway, but being in a
separate patch, would move this out of the way for the meat in this
patch, which is to introduce the rec->overwrite_evlist when backward
events were asked for.

Doing it this way will speed up processing of your patches and even more
importantly, will make our lives easier in the future when looking for
bugs :-\

I am breaking this down as described, please take a look at the result.

Thanks,

- Arnaldo
 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: He Kuang <hekuang@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Zefan Li <lizefan@huawei.com>
> Cc: pi3orama@163.com
> ---
>  tools/perf/builtin-record.c | 132 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 109 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index d4cf1b0..dbbb3c0 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -50,6 +50,7 @@ struct record {
>  	struct perf_data_file	file;
>  	struct auxtrace_record	*itr;
>  	struct perf_evlist	*evlist;
> +	struct perf_evlist	*overwrite_evlist;
>  	struct perf_session	*session;
>  	const char		*progname;
>  	int			realtime_prio;
> @@ -341,6 +342,84 @@ int auxtrace_record__snapshot_start(struct auxtrace_record *itr __maybe_unused)
>  
>  #endif
>  
> +static int record__create_overwrite_evlist(struct record *rec)
> +{
> +	struct perf_evlist *evlist = rec->evlist;
> +	struct perf_evsel *pos;
> +
> +	evlist__for_each(evlist, pos) {
> +		if (!pos->overwrite)
> +			continue;
> +
> +		if (!rec->overwrite_evlist) {
> +			rec->overwrite_evlist = perf_evlist__new_aux(evlist);
> +			if (rec->overwrite_evlist) {
> +				rec->overwrite_evlist->backward = true;
> +				rec->overwrite_evlist->overwrite = true;
> +				return 0;
> +			} else
> +				return -ENOMEM;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int record__mmap_evlist(struct record *rec,
> +			       struct perf_evlist *evlist,
> +			       bool overwrite)
> +{
> +	struct record_opts *opts = &rec->opts;
> +	char msg[512];
> +
> +	/*
> +	 * Don't use evlist->overwrite because it is logically an
> +	 * internal attribute and is set by perf_evlist__mmap_ex().
> +	 * Avoid circular dependency.
> +	 */
> +	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, overwrite,
> +				 opts->auxtrace_mmap_pages,
> +				 opts->auxtrace_snapshot_mode) < 0) {
> +		if (errno == EPERM) {
> +			pr_err("Permission error mapping pages.\n"
> +			       "Consider increasing "
> +			       "/proc/sys/kernel/perf_event_mlock_kb,\n"
> +			       "or try again with a smaller value of -m/--mmap_pages.\n"
> +			       "(current value: %u,%u)\n",
> +			       opts->mmap_pages, opts->auxtrace_mmap_pages);
> +			return -errno;
> +		} else {
> +			pr_err("failed to mmap with %d (%s)\n", errno,
> +				strerror_r(errno, msg, sizeof(msg)));
> +			if (errno)
> +				return -errno;
> +			else
> +				return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int record__mmap(struct record *rec)
> +{
> +	int err;
> +
> +	err = record__create_overwrite_evlist(rec);
> +	if (err)
> +		return err;
> +
> +	err = record__mmap_evlist(rec, rec->evlist, false);
> +	if (err)
> +		return err;
> +
> +	if (!rec->overwrite_evlist)
> +		return 0;
> +
> +	err = record__mmap_evlist(rec, rec->overwrite_evlist, true);
> +	if (err)
> +		return err;
> +	return 0;
> +}
> +
>  static int record__open(struct record *rec)
>  {
>  	char msg[512];
> @@ -353,6 +432,13 @@ static int record__open(struct record *rec)
>  	perf_evlist__config(evlist, opts, &callchain_param);
>  
>  	evlist__for_each(evlist, pos) {
> +		if (pos->overwrite) {
> +			if (!pos->attr.write_backward) {
> +				ui__warning("Unable to read from overwrite ring buffer\n\n");
> +				rc = -ENOSYS;
> +				goto out;
> +			}
> +		}
>  try_again:
>  		if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) {
>  			if (perf_evsel__fallback(pos, errno, msg, sizeof(msg))) {
> @@ -377,28 +463,9 @@ try_again:
>  		goto out;
>  	}
>  
> -	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, false,
> -				 opts->auxtrace_mmap_pages,
> -				 opts->auxtrace_snapshot_mode) < 0) {
> -		if (errno == EPERM) {
> -			pr_err("Permission error mapping pages.\n"
> -			       "Consider increasing "
> -			       "/proc/sys/kernel/perf_event_mlock_kb,\n"
> -			       "or try again with a smaller value of -m/--mmap_pages.\n"
> -			       "(current value: %u,%u)\n",
> -			       opts->mmap_pages, opts->auxtrace_mmap_pages);
> -			rc = -errno;
> -		} else {
> -			pr_err("failed to mmap with %d (%s)\n", errno,
> -				strerror_r(errno, msg, sizeof(msg)));
> -			if (errno)
> -				rc = -errno;
> -			else
> -				rc = -EINVAL;
> -		}
> +	rc = record__mmap(rec);
> +	if (rc)
>  		goto out;
> -	}
> -
>  	session->evlist = evlist;
>  	perf_session__set_id_hdr_size(session);
>  out:
> @@ -655,10 +722,26 @@ perf_event__synth_time_conv(const struct perf_event_mmap_page *pc __maybe_unused
>  	return 0;
>  }
>  
> +static const struct perf_event_mmap_page *
> +perf_evlist__pick_pc(struct perf_evlist *evlist)
> +{
> +	if (evlist && evlist->mmap && evlist->mmap[0].base)
> +		return evlist->mmap[0].base;
> +	return NULL;
> +}
> +
>  static const struct perf_event_mmap_page *record__pick_pc(struct record *rec)
>  {
> -	if (rec->evlist && rec->evlist->mmap && rec->evlist->mmap[0].base)
> -		return rec->evlist->mmap[0].base;
> +	const struct perf_event_mmap_page *pc;
> +
> +	/* Change it to a loop if a new aux evlist is added */
> +	pc = perf_evlist__pick_pc(rec->evlist);
> +	if (pc)
> +		return pc;
> +	pc = perf_evlist__pick_pc(rec->overwrite_evlist);
> +	if (pc)
> +		return pc;
> +
>  	return NULL;
>  }
>  
> @@ -1269,6 +1352,7 @@ static struct record record = {
>  		.mmap2		= perf_event__process_mmap2,
>  		.ordered_events	= true,
>  	},
> +	.overwrite_evlist = NULL,
>  };
>  
>  const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
> @@ -1565,6 +1649,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
>  	err = __cmd_record(&record, argc, argv);
>  out_symbol_exit:
>  	perf_evlist__delete(rec->evlist);
> +	if (rec->overwrite_evlist)
> +		perf_evlist__delete(rec->overwrite_evlist);
>  	symbol__exit();
>  	auxtrace_record__free(rec->itr);
>  	return err;
> -- 
> 1.8.3.4