Re: [PATCH v8 3/8] perf tests: Add testcase for auxiliary evlist

[Arnaldo Carvalho de Melo]

Em Mon, Jun 20, 2016 at 10:47:20AM +0000, Wang Nan escreveu:
> Improve test backward-ring-buffer, trace both enter and exit event of
> prctl() syscall, utilize auxiliary evlist to mmap enter and exit event
> into separated mmaps.
> 
> Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Cc: Zefan Li <lizefan@xxxxxxxxxx>
> Cc: He Kuang <hekuang@xxxxxxxxxx>
> ---
>  tools/perf/tests/backward-ring-buffer.c | 87 ++++++++++++++++++++++++++-------
>  1 file changed, 68 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c
> index d9ba991..76e34c0 100644
> --- a/tools/perf/tests/backward-ring-buffer.c
> +++ b/tools/perf/tests/backward-ring-buffer.c
> @@ -31,16 +31,19 @@ static int count_samples(struct perf_evlist *evlist, int *sample_count,
>  	for (i = 0; i < evlist->nr_mmaps; i++) {
>  		union perf_event *event;
>  
> -		perf_evlist__mmap_read_catchup(evlist, i);
> -		while ((event = perf_evlist__mmap_read_backward(evlist, i)) != NULL) {
> +		if (evlist->backward)
> +			perf_evlist__mmap_read_catchup(evlist, i);

So, shouldn't this be done at perf_evlist__mmap_read_catchup()? I.e. you
will use this only when you know that one of the evlists count_samples()
will deal with can be a backwards one...

I.e. do with perf_evlist__mmap_read_catchup the same thing you did in
perf_evlist__mmap_read, checking there this evlist->backward. 

This can be done on top, so I'll continue tentatively merging this.

> +		while ((event = perf_evlist__mmap_read(evlist, i)) != NULL) {
>  			const u32 type = event->header.type;
>  
>  			switch (type) {
>  			case PERF_RECORD_SAMPLE:
> -				(*sample_count)++;
> +				if (sample_count)
> +					(*sample_count)++;
>  				break;
>  			case PERF_RECORD_COMM:
> -				(*comm_count)++;
> +				if (comm_count)
> +					(*comm_count)++;

You could've avoided all this by passing some dummy integer pointer for
the enter_sample_count case. Making the patch smaller helps reviewing
:-)

>  				break;
>  			default:
>  				pr_err("Unexpected record of type %d\n", type);
> @@ -51,34 +54,53 @@ static int count_samples(struct perf_evlist *evlist, int *sample_count,
>  	return TEST_OK;
>  }
>  
> -static int do_test(struct perf_evlist *evlist, int mmap_pages,
> -		   int *sample_count, int *comm_count)
> +static int do_test(struct perf_evlist *evlist,
> +		   struct perf_evlist *aux_evlist,
> +		   int mmap_pages,
> +		   int *enter_sample_count,
> +		   int *exit_sample_count,
> +		   int *comm_count)
>  {
>  	int err;
>  	char sbuf[STRERR_BUFSIZE];
>  
> -	err = perf_evlist__mmap(evlist, mmap_pages, true);
> +	err = perf_evlist__mmap(evlist, mmap_pages, false);
>  	if (err < 0) {
>  		pr_debug("perf_evlist__mmap: %s\n",
>  			 strerror_r(errno, sbuf, sizeof(sbuf)));
>  		return TEST_FAIL;
>  	}
>  
> +	err = perf_evlist__mmap(aux_evlist, mmap_pages, true);
> +	if (err < 0) {
> +		pr_debug("perf_evlist__mmap for aux_evlist: %s\n",
> +			 strerror_r(errno, sbuf, sizeof(sbuf)));
> +		return TEST_FAIL;
> +	}
> +
>  	perf_evlist__enable(evlist);
>  	testcase();
>  	perf_evlist__disable(evlist);
>  
> -	err = count_samples(evlist, sample_count, comm_count);
> +	err = count_samples(aux_evlist, exit_sample_count, comm_count);
> +	if (err)
> +		goto errout;
> +	err = count_samples(evlist, enter_sample_count, NULL);
> +	if (err)
> +		goto errout;
> +errout:
>  	perf_evlist__munmap(evlist);
> +	perf_evlist__munmap(aux_evlist);
>  	return err;
>  }
>  
>  
>  int test__backward_ring_buffer(int subtest __maybe_unused)
>  {
> -	int ret = TEST_SKIP, err, sample_count = 0, comm_count = 0;
> +	int ret = TEST_SKIP, err;
> +	int enter_sample_count = 0, exit_sample_count = 0, comm_count = 0;
>  	char pid[16], sbuf[STRERR_BUFSIZE];
> -	struct perf_evlist *evlist;
> +	struct perf_evlist *evlist, *aux_evlist = NULL;
>  	struct perf_evsel *evsel __maybe_unused;
>  	struct parse_events_error parse_error;
>  	struct record_opts opts = {
> @@ -115,11 +137,22 @@ int test__backward_ring_buffer(int subtest __maybe_unused)
>  		goto out_delete_evlist;
>  	}
>  
> -	perf_evlist__config(evlist, &opts, NULL);
> +	/*
> +	 * Set backward bit, ring buffer should be writing from end. Record
> +	 * it in aux evlist
> +	 */
> +	perf_evlist__last(evlist)->overwrite = true;
> +	perf_evlist__last(evlist)->attr.write_backward = 1;
>  
> -	/* Set backward bit, ring buffer should be writing from end */
> -	evlist__for_each(evlist, evsel)
> -		evsel->attr.write_backward = 1;
> +	err = parse_events(evlist, "syscalls:sys_exit_prctl", &parse_error);
> +	if (err) {
> +		pr_debug("Failed to parse tracepoint event, try use root\n");
> +		ret = TEST_SKIP;
> +		goto out_delete_evlist;
> +	}
> +	/* Don't set backward bit for exit event. Record it in main evlist */
> +
> +	perf_evlist__config(evlist, &opts, NULL);
>  
>  	err = perf_evlist__open(evlist);
>  	if (err < 0) {
> @@ -128,24 +161,40 @@ int test__backward_ring_buffer(int subtest __maybe_unused)
>  		goto out_delete_evlist;
>  	}
>  
> +	aux_evlist = perf_evlist__new_aux(evlist);
> +	if (!aux_evlist) {
> +		pr_debug("perf_evlist__new_aux failed\n");
> +		goto out_delete_evlist;
> +	}
> +	aux_evlist->backward = true;
> +
>  	ret = TEST_FAIL;
> -	err = do_test(evlist, opts.mmap_pages, &sample_count,
> +	err = do_test(evlist, aux_evlist, opts.mmap_pages,
> +		      &enter_sample_count, &exit_sample_count,
>  		      &comm_count);
>  	if (err != TEST_OK)
>  		goto out_delete_evlist;
>  
> -	if ((sample_count != NR_ITERS) || (comm_count != NR_ITERS)) {
> -		pr_err("Unexpected counter: sample_count=%d, comm_count=%d\n",
> -		       sample_count, comm_count);
> +	if (enter_sample_count != exit_sample_count) {
> +		pr_err("Unexpected counter: enter_sample_count=%d, exit_sample_count=%d\n",
> +		       enter_sample_count, exit_sample_count);
> +		goto out_delete_evlist;
> +	}
> +
> +	if ((exit_sample_count != NR_ITERS) || (comm_count != NR_ITERS)) {
> +		pr_err("Unexpected counter: exit_sample_count=%d, comm_count=%d\n",
> +		       exit_sample_count, comm_count);
>  		goto out_delete_evlist;
>  	}
>  
> -	err = do_test(evlist, 1, &sample_count, &comm_count);
> +	err = do_test(evlist, aux_evlist, 1, NULL, NULL, NULL);
>  	if (err != TEST_OK)
>  		goto out_delete_evlist;
>  
>  	ret = TEST_OK;
>  out_delete_evlist:
> +	if (aux_evlist)
> +		perf_evlist__delete(aux_evlist);
>  	perf_evlist__delete(evlist);
>  	return ret;
>  }
> -- 
> 1.8.3.4