RE: [PATCH 6/7] perf tools: Remove 'overwrite' concept from code level

From: Liang, Kan
Date: Mon Nov 13 2017 - 10:07:30 EST

Next message: Ingo Molnar: "Re: [PATCH RFC v3 4/6] Documentation: Add three sysctls for smart idle poll"
Previous message: Paolo Bonzini: "Re: [PATCH] KVM: X86: Avoid to handle first-time write when updating the pv stuffs each time"
In reply to: Jiri Olsa: "Re: [PATCH 6/7] perf tools: Remove 'overwrite' concept from code level"
Next in thread: Wang Nan: "[PATCH 2/7] perf tests: Set evlist of test__backward_ring_buffer() to !overwrite"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> Since all 'overwrite' usage are cleaned and no one really use a readonly main
> ringbuffer, remove 'overwrite' from function arguments and evlist. The
> concept
> of 'overwrite' and 'write_backward' are cleanner than before:
>
> 1. In code level, there's no 'overwrite' concept. Each evlist has two
> ringbuffer groups. One is read-write/forward, another is
> readonly/backward.
> Don't support read-write/backward and readonly/forward.
>
> 2. In user interface, we keep '--overwrite' and translate it into
> write_backward
> in each event.
>
> Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> ---
> tools/perf/arch/x86/tests/perf-time-to-tsc.c | 2 +-
> tools/perf/builtin-kvm.c | 2 +-
> tools/perf/builtin-record.c | 4 ++--
> tools/perf/builtin-top.c | 2 +-
> tools/perf/builtin-trace.c | 2 +-
> tools/perf/tests/backward-ring-buffer.c | 2 +-
> tools/perf/tests/bpf.c | 2 +-
> tools/perf/tests/code-reading.c | 2 +-
> tools/perf/tests/keep-tracking.c | 2 +-
> tools/perf/tests/mmap-basic.c | 2 +-
> tools/perf/tests/openat-syscall-tp-fields.c | 2 +-
> tools/perf/tests/perf-record.c | 2 +-
> tools/perf/tests/sw-clock.c | 2 +-
> tools/perf/tests/switch-tracking.c | 2 +-
> tools/perf/tests/task-exit.c | 2 +-
> tools/perf/util/evlist.c | 14 ++++++--------
> tools/perf/util/evlist.h | 6 ++----
> tools/perf/util/mmap.c | 6 +++---
> tools/perf/util/mmap.h | 2 +-
> tools/perf/util/python.c | 10 +++++-----
> 20 files changed, 33 insertions(+), 37 deletions(-)
>
> diff --git a/tools/perf/arch/x86/tests/perf-time-to-tsc.c
> b/tools/perf/arch/x86/tests/perf-time-to-tsc.c
> index 5dd7efb..c7ea843 100644
> --- a/tools/perf/arch/x86/tests/perf-time-to-tsc.c
> +++ b/tools/perf/arch/x86/tests/perf-time-to-tsc.c
> @@ -83,7 +83,7 @@ int test__perf_time_to_tsc(struct test *test
> __maybe_unused, int subtest __maybe
>
> CHECK__(perf_evlist__open(evlist));
>
> - CHECK__(perf_evlist__mmap(evlist, UINT_MAX, false));
> + CHECK__(perf_evlist__mmap(evlist, UINT_MAX));
>
> pc = evlist->mmap[0].base;
> ret = perf_read_tsc_conversion(pc, &tc);
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 0af4c09..e3e2a80 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -1043,7 +1043,7 @@ static int kvm_live_open_events(struct
> perf_kvm_stat *kvm)
> goto out;
> }
>
> - if (perf_evlist__mmap(evlist, kvm->opts.mmap_pages, false) < 0) {
> + if (perf_evlist__mmap(evlist, kvm->opts.mmap_pages) < 0) {
> ui__error("Failed to mmap the events: %s\n",
> str_error_r(errno, sbuf, sizeof(sbuf)));
> perf_evlist__close(evlist);
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index f4d9fc5..b3ef33f 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -300,7 +300,7 @@ static int record__mmap_evlist(struct record *rec,
> struct record_opts *opts = &rec->opts;
> char msg[512];
>
> - if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, false,
> + if (perf_evlist__mmap_ex(evlist, opts->mmap_pages,
> opts->auxtrace_mmap_pages,
> opts->auxtrace_snapshot_mode) < 0) {
> if (errno == EPERM) {
> @@ -481,7 +481,7 @@ static int record__mmap_read_evlist(struct record
> *rec, struct perf_evlist *evli
> struct auxtrace_mmap *mm = &maps[i].auxtrace_mmap;
>
> if (maps[i].base) {
> - if (perf_mmap__push(&maps[i], evlist->overwrite,
> backward, rec, record__pushfn) != 0) {
> + if (perf_mmap__push(&maps[i], backward, rec,
> record__pushfn) != 0) {
> rc = -1;
> goto out;
> }
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 477a869..83d2ae2 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -902,7 +902,7 @@ static int perf_top__start_counters(struct perf_top
> *top)
> }
> }
>
> - if (perf_evlist__mmap(evlist, opts->mmap_pages, false) < 0) {
> + if (perf_evlist__mmap(evlist, opts->mmap_pages) < 0) {
> ui__error("Failed to mmap with %d (%s)\n",
> errno, str_error_r(errno, msg, sizeof(msg)));
> goto out_err;
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index c373f9a..c3f2f98 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -2404,7 +2404,7 @@ static int trace__run(struct trace *trace, int argc,
> const char **argv)
> if (err < 0)
> goto out_error_apply_filters;
>
> - err = perf_evlist__mmap(evlist, trace->opts.mmap_pages, false);
> + err = perf_evlist__mmap(evlist, trace->opts.mmap_pages);
> if (err < 0)
> goto out_error_mmap;
>
> diff --git a/tools/perf/tests/backward-ring-buffer.c
> b/tools/perf/tests/backward-ring-buffer.c
> index 992c917..bdebcf9 100644
> --- a/tools/perf/tests/backward-ring-buffer.c
> +++ b/tools/perf/tests/backward-ring-buffer.c
> @@ -58,7 +58,7 @@ static int do_test(struct perf_evlist *evlist, int
> mmap_pages,
> int err;
> char sbuf[STRERR_BUFSIZE];
>
> - err = perf_evlist__mmap(evlist, mmap_pages, false);
> + err = perf_evlist__mmap(evlist, mmap_pages);
> if (err < 0) {
> pr_debug("perf_evlist__mmap: %s\n",
> str_error_r(errno, sbuf, sizeof(sbuf)));
> diff --git a/tools/perf/tests/bpf.c b/tools/perf/tests/bpf.c
> index 34c22cd..c433dd3 100644
> --- a/tools/perf/tests/bpf.c
> +++ b/tools/perf/tests/bpf.c
> @@ -167,7 +167,7 @@ static int do_test(struct bpf_object *obj, int
> (*func)(void),
> goto out_delete_evlist;
> }
>
> - err = perf_evlist__mmap(evlist, opts.mmap_pages, false);
> + err = perf_evlist__mmap(evlist, opts.mmap_pages);
> if (err < 0) {
> pr_debug("perf_evlist__mmap: %s\n",
> str_error_r(errno, sbuf, sizeof(sbuf)));
> diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> index 466a462..b6c813e 100644
> --- a/tools/perf/tests/code-reading.c
> +++ b/tools/perf/tests/code-reading.c
> @@ -638,7 +638,7 @@ static int do_test_code_reading(bool try_kcore)
> break;
> }
>
> - ret = perf_evlist__mmap(evlist, UINT_MAX, false);
> + ret = perf_evlist__mmap(evlist, UINT_MAX);
> if (ret < 0) {
> pr_debug("perf_evlist__mmap failed\n");
> goto out_put;
> diff --git a/tools/perf/tests/keep-tracking.c b/tools/perf/tests/keep-tracking.c
> index 7394286..b282916 100644
> --- a/tools/perf/tests/keep-tracking.c
> +++ b/tools/perf/tests/keep-tracking.c
> @@ -94,7 +94,7 @@ int test__keep_tracking(struct test *test
> __maybe_unused, int subtest __maybe_un
> goto out_err;
> }
>
> - CHECK__(perf_evlist__mmap(evlist, UINT_MAX, false));
> + CHECK__(perf_evlist__mmap(evlist, UINT_MAX));
>
> /*
> * First, test that a 'comm' event can be found when the event is
> diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
> index 667d696..f43d862 100644
> --- a/tools/perf/tests/mmap-basic.c
> +++ b/tools/perf/tests/mmap-basic.c
> @@ -93,7 +93,7 @@ int test__basic_mmap(struct test *test
> __maybe_unused, int subtest __maybe_unuse
> expected_nr_events[i] = 1 + rand() % 127;
> }
>
> - if (perf_evlist__mmap(evlist, 128, false) < 0) {
> + if (perf_evlist__mmap(evlist, 128) < 0) {
> pr_debug("failed to mmap events: %d (%s)\n", errno,
> str_error_r(errno, sbuf, sizeof(sbuf)));
> goto out_delete_evlist;
> diff --git a/tools/perf/tests/openat-syscall-tp-fields.c
> b/tools/perf/tests/openat-syscall-tp-fields.c
> index b6ee1c4..f68d818 100644
> --- a/tools/perf/tests/openat-syscall-tp-fields.c
> +++ b/tools/perf/tests/openat-syscall-tp-fields.c
> @@ -63,7 +63,7 @@ int test__syscall_openat_tp_fields(struct test *test
> __maybe_unused, int subtest
> goto out_delete_evlist;
> }
>
> - err = perf_evlist__mmap(evlist, UINT_MAX, false);
> + err = perf_evlist__mmap(evlist, UINT_MAX);
> if (err < 0) {
> pr_debug("perf_evlist__mmap: %s\n",
> str_error_r(errno, sbuf, sizeof(sbuf)));
> diff --git a/tools/perf/tests/perf-record.c b/tools/perf/tests/perf-record.c
> index 19b6500..f9fbd8b 100644
> --- a/tools/perf/tests/perf-record.c
> +++ b/tools/perf/tests/perf-record.c
> @@ -140,7 +140,7 @@ int test__PERF_RECORD(struct test *test
> __maybe_unused, int subtest __maybe_unus
> * fds in the same CPU to be injected in the same mmap ring buffer
> * (using ioctl(PERF_EVENT_IOC_SET_OUTPUT)).
> */
> - err = perf_evlist__mmap(evlist, opts.mmap_pages, false);
> + err = perf_evlist__mmap(evlist, opts.mmap_pages);
> if (err < 0) {
> pr_debug("perf_evlist__mmap: %s\n",
> str_error_r(errno, sbuf, sizeof(sbuf)));
> diff --git a/tools/perf/tests/sw-clock.c b/tools/perf/tests/sw-clock.c
> index c468e6c..fd01b24 100644
> --- a/tools/perf/tests/sw-clock.c
> +++ b/tools/perf/tests/sw-clock.c
> @@ -77,7 +77,7 @@ static int __test__sw_clock_freq(enum perf_sw_ids
> clock_id)
> goto out_delete_evlist;
> }
>
> - err = perf_evlist__mmap(evlist, 128, false);
> + err = perf_evlist__mmap(evlist, 128);
> if (err < 0) {
> pr_debug("failed to mmap event: %d (%s)\n", errno,
> str_error_r(errno, sbuf, sizeof(sbuf)));
> diff --git a/tools/perf/tests/switch-tracking.c b/tools/perf/tests/switch-
> tracking.c
> index 2acd785..9fee2a0 100644
> --- a/tools/perf/tests/switch-tracking.c
> +++ b/tools/perf/tests/switch-tracking.c
> @@ -448,7 +448,7 @@ int test__switch_tracking(struct test *test
> __maybe_unused, int subtest __maybe_
> goto out;
> }
>
> - err = perf_evlist__mmap(evlist, UINT_MAX, false);
> + err = perf_evlist__mmap(evlist, UINT_MAX);
> if (err) {
> pr_debug("perf_evlist__mmap failed!\n");
> goto out_err;
> diff --git a/tools/perf/tests/task-exit.c b/tools/perf/tests/task-exit.c
> index 4fb6609..4ba5a27 100644
> --- a/tools/perf/tests/task-exit.c
> +++ b/tools/perf/tests/task-exit.c
> @@ -96,7 +96,7 @@ int test__task_exit(struct test *test __maybe_unused,
> int subtest __maybe_unused
> goto out_delete_evlist;
> }
>
> - if (perf_evlist__mmap(evlist, 128, false) < 0) {
> + if (perf_evlist__mmap(evlist, 128) < 0) {
> pr_debug("failed to mmap events: %d (%s)\n", errno,
> str_error_r(errno, sbuf, sizeof(sbuf)));
> goto out_delete_evlist;
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 4c5daba..4948d3d 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -711,7 +711,7 @@ union perf_event
> *perf_evlist__mmap_read_forward(struct perf_evlist *evlist, int
> * No need for read-write ring buffer: kernel stop outputting when
> * it hit md->prev (perf_mmap__consume()).
> */
> - return perf_mmap__read_forward(md, evlist->overwrite);
> + return perf_mmap__read_forward(md, false);

If we remove the 'overwrite' in forward, I think 'check_messup' is useless.
It's better to remove it as well.

> }
>
> union perf_event *perf_evlist__mmap_read_backward(struct perf_evlist
> *evlist, int idx)
> @@ -738,7 +738,7 @@ void perf_evlist__mmap_read_catchup(struct
> perf_evlist *evlist, int idx)
>
> void perf_evlist__mmap_consume(struct perf_evlist *evlist, int idx)
> {
> - perf_mmap__consume(&evlist->mmap[idx], evlist->overwrite);
> + perf_mmap__consume(&evlist->mmap[idx], false);

I think we still need to remove 'overwrite' in the implementation of
perf_mmap__consume.

There are too many generic functions are changed in this patch.
I think it's better to split into several patches.
It's better to have one generic function changed in one patch.

Thanks,
Kan

> }
>
> static void perf_evlist__munmap_nofree(struct perf_evlist *evlist)
> @@ -1058,14 +1058,14 @@ int perf_evlist__parse_mmap_pages(const
> struct option *opt, const char *str,
> * Return: %0 on success, negative error code otherwise.
> */
> int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
> - bool overwrite, unsigned int auxtrace_pages,
> + unsigned int auxtrace_pages,
> bool auxtrace_overwrite)
> {
> struct perf_evsel *evsel;
> const struct cpu_map *cpus = evlist->cpus;
> const struct thread_map *threads = evlist->threads;
> struct mmap_params mp = {
> - .prot = PROT_READ | (overwrite ? 0 : PROT_WRITE),
> + .prot = PROT_READ | PROT_WRITE,
> };
>
> if (!evlist->mmap)
> @@ -1076,7 +1076,6 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist,
> unsigned int pages,
> if (evlist->pollfd.entries == NULL && perf_evlist__alloc_pollfd(evlist) <
> 0)
> return -ENOMEM;
>
> - evlist->overwrite = overwrite;
> evlist->mmap_len = perf_evlist__mmap_size(pages);
> pr_debug("mmap size %zuB\n", evlist->mmap_len);
> mp.mask = evlist->mmap_len - page_size - 1;
> @@ -1097,10 +1096,9 @@ int perf_evlist__mmap_ex(struct perf_evlist
> *evlist, unsigned int pages,
> return perf_evlist__mmap_per_cpu(evlist, &mp);
> }
>
> -int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages,
> - bool overwrite)
> +int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages)
> {
> - return perf_evlist__mmap_ex(evlist, pages, overwrite, 0, false);
> + return perf_evlist__mmap_ex(evlist, pages, 0, false);
> }
>
> int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 8c433e9..1dd3291 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -30,7 +30,6 @@ struct perf_evlist {
> int nr_entries;
> int nr_groups;
> int nr_mmaps;
> - bool overwrite;
> bool enabled;
> bool has_user_cpus;
> size_t mmap_len;
> @@ -168,10 +167,9 @@ int perf_evlist__parse_mmap_pages(const struct
> option *opt,
> unsigned long perf_event_mlock_kb_in_pages(void);
>
> int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
> - bool overwrite, unsigned int auxtrace_pages,
> + unsigned int auxtrace_pages,
> bool auxtrace_overwrite);
> -int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages,
> - bool overwrite);
> +int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages);
> void perf_evlist__munmap(struct perf_evlist *evlist);
>
> size_t perf_evlist__mmap_size(unsigned long pages);
> diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> index 9fe5f9c..703ed41 100644
> --- a/tools/perf/util/mmap.c
> +++ b/tools/perf/util/mmap.c
> @@ -299,7 +299,7 @@ static int rb_find_range(void *data, int mask, u64
> head, u64 old,
> return backward_rb_find_range(data, mask, head, start, end);
> }
>
> -int perf_mmap__push(struct perf_mmap *md, bool overwrite, bool
> backward,
> +int perf_mmap__push(struct perf_mmap *md, bool backward,
> void *to, int push(void *to, void *buf, size_t size))
> {
> u64 head = perf_mmap__read_head(md);
> @@ -321,7 +321,7 @@ int perf_mmap__push(struct perf_mmap *md, bool
> overwrite, bool backward,
> WARN_ONCE(1, "failed to keep up with mmap data. (warn
> only once)\n");
>
> md->prev = head;
> - perf_mmap__consume(md, overwrite || backward);
> + perf_mmap__consume(md, backward);
> return 0;
> }
>
> @@ -346,7 +346,7 @@ int perf_mmap__push(struct perf_mmap *md, bool
> overwrite, bool backward,
> }
>
> md->prev = head;
> - perf_mmap__consume(md, overwrite || backward);
> + perf_mmap__consume(md, backward);
> out:
> return rc;
> }
> diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h
> index efd78b8..2c3d291 100644
> --- a/tools/perf/util/mmap.h
> +++ b/tools/perf/util/mmap.h
> @@ -89,7 +89,7 @@ static inline void perf_mmap__write_tail(struct
> perf_mmap *md, u64 tail)
> union perf_event *perf_mmap__read_forward(struct perf_mmap *map,
> bool check_messup);
> union perf_event *perf_mmap__read_backward(struct perf_mmap *map);
>
> -int perf_mmap__push(struct perf_mmap *md, bool overwrite, bool
> backward,
> +int perf_mmap__push(struct perf_mmap *md, bool backward,
> void *to, int push(void *to, void *buf, size_t size));
>
> size_t perf_mmap__mmap_len(struct perf_mmap *map);
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index c129e99..ece33b4 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -856,14 +856,14 @@ static PyObject *pyrf_evlist__mmap(struct
> pyrf_evlist *pevlist,
> PyObject *args, PyObject *kwargs)
> {
> struct perf_evlist *evlist = &pevlist->evlist;
> - static char *kwlist[] = { "pages", "overwrite", NULL };
> - int pages = 128, overwrite = false;
> + static char *kwlist[] = { "pages", NULL };
> + int pages = 128;
>
> - if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|ii", kwlist,
> - &pages, &overwrite))
> + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|i", kwlist,
> + &pages))
> return NULL;
>
> - if (perf_evlist__mmap(evlist, pages, overwrite) < 0) {
> + if (perf_evlist__mmap(evlist, pages) < 0) {
> PyErr_SetFromErrno(PyExc_OSError);
> return NULL;
> }
> --
> 2.10.1

Next message: Ingo Molnar: "Re: [PATCH RFC v3 4/6] Documentation: Add three sysctls for smart idle poll"
Previous message: Paolo Bonzini: "Re: [PATCH] KVM: X86: Avoid to handle first-time write when updating the pv stuffs each time"
In reply to: Jiri Olsa: "Re: [PATCH 6/7] perf tools: Remove 'overwrite' concept from code level"
Next in thread: Wang Nan: "[PATCH 2/7] perf tests: Set evlist of test__backward_ring_buffer() to !overwrite"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]