Re: [PATCH v12 1/8] perf tests: Skip backward ring buffer on old kernel

[pi3orama]

发自我的 iPhone

> 在 2016年7月1日，下午9:40，Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> 写道：
> 
> Em Fri, Jul 01, 2016 at 02:23:44AM +0000, Wang Nan escreveu:
>> From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>> 
>> 'perf test backward' FAILED on old kernel:
>> 
>> [root@jouet ~]# perf test -v backward
>> 45: Test backward reading from ring buffer                   :
>> --- start ---
>> <SNIP>
>> mmap size 1052672B
>> Unexpected counter: sample_count=0, comm_count=0
>> ---- end ----
>> Test backward reading from ring buffer: FAILED!
>> [root@jouet ~]#
>> 
>> Reason: when kernel doesn't support 'attr.backward', perf_evsel__open() will
>> trim this bit and try again, unless evsel->overwrite is true.
>> 
>> Set evsel->overwrite in this test.
> 
> This fixes it, but then, if we get attr.backward set and the kernel
> doesn't support it, what whould be the point of trying without it? I.e.
> its not like we can do what is being asked for by fallbacking to some
> other method.
> 
> I.e. perhaps evsel->overwrite is completely unnecessary and places
> checking for it should instead check for evsel->attr.backward, no?
> 

If we remove evsel->overwrite, what should
we set in parser? Make it directly setting
attr?

Thank you.

> Something like the patch below, with it I also get:
> 
> [root@jouet perf]# perf test backward
> 45: Test backward reading from ring buffer                   : Skip
> [root@jouet perf]# perf test -v backward 2>&1 | grep -v regist
> 45: Test backward reading from ring buffer                   :
> --- start ---
> test child forked, pid 29997
> perf_evlist__open: Invalid argument
> test child finished with -2
> ---- end ----
> Test backward reading from ring buffer: Skip
> [root@jouet perf]# 
> 
> - Arnaldo
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 113507716044..060c3b3d14db 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1003,7 +1003,7 @@ static bool
> perf_evlist__should_poll(struct perf_evlist *evlist __maybe_unused,
>             struct perf_evsel *evsel)
> {
> -    if (evsel->overwrite)
> +    if (evsel->attr.write_backward)
>        return false;
>    return true;
> }
> @@ -1018,7 +1018,7 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>    evlist__for_each_entry(evlist, evsel) {
>        int fd;
> 
> -        if (evsel->overwrite != (evlist->overwrite && evlist->backward))
> +        if (evsel->attr.write_backward != (evlist->overwrite && evlist->backward))
>            continue;
> 
>        if (evsel->system_wide && thread)
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 0fea724e735c..3abe5195fe80 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1359,6 +1359,9 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>    int pid = -1, err;
>    enum { NO_CHANGE, SET_TO_MAX, INCREASED_MAX } set_rlimit = NO_CHANGE;
> 
> +    if (perf_missing_features.write_backward && evsel->attr.write_backward)
> +        return -EINVAL;
> +
>    if (evsel->system_wide)
>        nthreads = 1;
>    else
> @@ -1389,11 +1392,6 @@ fallback_missing_features:
>    if (perf_missing_features.lbr_flags)
>        evsel->attr.branch_sample_type &= ~(PERF_SAMPLE_BRANCH_NO_FLAGS |
>                     PERF_SAMPLE_BRANCH_NO_CYCLES);
> -    if (perf_missing_features.write_backward) {
> -        if (evsel->overwrite)
> -            return -EINVAL;
> -        evsel->attr.write_backward = false;
> -    }
> retry_sample_id:
>    if (perf_missing_features.sample_id_all)
>        evsel->attr.sample_id_all = 0;
> @@ -1495,7 +1493,7 @@ try_fallback:
>     */
>    if (!perf_missing_features.write_backward && evsel->attr.write_backward) {
>        perf_missing_features.write_backward = true;
> -        goto fallback_missing_features;
> +        goto out_close;
>    } else if (!perf_missing_features.clockid_wrong && evsel->attr.use_clockid) {
>        perf_missing_features.clockid_wrong = true;
>        goto fallback_missing_features;
> @@ -2404,7 +2402,7 @@ int perf_evsel__open_strerror(struct perf_evsel *evsel, struct target *target,
>    "We found oprofile daemon running, please stop it and try again.");
>        break;
>    case EINVAL:
> -        if (evsel->overwrite && perf_missing_features.write_backward)
> +        if (evsel->attr.write_backward && perf_missing_features.write_backward)
>            return scnprintf(msg, size, "Reading from overwrite event is not supported by this kernel.");
>        if (perf_missing_features.clockid)
>            return scnprintf(msg, size, "clockid feature not supported.");
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 86fed7a2932b..a31ee2d5d2d3 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -114,7 +114,6 @@ struct perf_evsel {
>    bool            tracking;
>    bool            per_pkg;
>    bool            precise_max;
> -    bool            overwrite;
>    /* parse modifier helper */
>    int            exclude_GH;
>    int            nr_members;