lkml.org 
[lkml]   [2021]   [Nov]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v7 08/11] tools/perf/test: make perf test adopt to task comm size change
    Em Mon, Nov 01, 2021 at 06:04:16AM +0000, Yafang Shao escreveu:
    > kernel test robot reported a perf-test failure after I extended task comm
    > size from 16 to 24. The failure as follows,
    >
    > 2021-10-13 18:00:46 sudo /usr/src/perf_selftests-x86_64-rhel-8.3-317419b91ef4eff4e2f046088201e4dc4065caa0/tools/perf/perf test 15
    > 15: Parse sched tracepoints fields : FAILED!
    >
    > The reason is perf-test requires a fixed-size task comm. If we extend
    > task comm size to 24, it will not equil with the required size 16 in perf
    > test.
    >
    > After some analyzation, I found perf itself can adopt to the size
    > change, for example, below is the output of perf-sched after I extend
    > comm size to 24 -
    >
    > task 614 ( kthreadd: 84), nr_events: 1
    > task 615 ( systemd: 843), nr_events: 1
    > task 616 ( networkd-dispat: 1026), nr_events: 1
    > task 617 ( systemd: 846), nr_events: 1
    >
    > $ cat /proc/843/comm
    > networkd-dispatcher
    >
    > The task comm can be displayed correctly as expected.
    >
    > Replace old hard-coded 16 with the new one can fix the warning, but we'd
    > better make the test accept both old and new sizes, then it can be
    > backward compatibility.
    >
    > After this patch, the perf-test succeeds no matter task comm is 16 or
    > 24 -
    >
    > 15: Parse sched tracepoints fields : Ok
    >
    > This patch is a preparation for the followup patch.
    >
    > Reported-by: kernel test robot <oliver.sang@intel.com>
    > Suggested-by: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
    > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
    > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
    > Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
    > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
    > Cc: Peter Zijlstra <peterz@infradead.org>
    > Cc: Steven Rostedt <rostedt@goodmis.org>
    > Cc: Al Viro <viro@zeniv.linux.org.uk>
    > Cc: Kees Cook <keescook@chromium.org>
    > Cc: Petr Mladek <pmladek@suse.com>
    > ---
    > tools/include/linux/sched.h | 11 +++++++++++
    > tools/perf/tests/evsel-tp-sched.c | 26 ++++++++++++++++++++------
    > 2 files changed, 31 insertions(+), 6 deletions(-)
    > create mode 100644 tools/include/linux/sched.h
    >
    > diff --git a/tools/include/linux/sched.h b/tools/include/linux/sched.h
    > new file mode 100644
    > index 000000000000..0d575afd7f43
    > --- /dev/null
    > +++ b/tools/include/linux/sched.h
    > @@ -0,0 +1,11 @@
    > +/* SPDX-License-Identifier: GPL-2.0 */
    > +#ifndef _TOOLS_LINUX_SCHED_H
    > +#define _TOOLS_LINUX_SCHED_H
    > +
    > +/* Keep both length for backward compatibility */
    > +enum {
    > + TASK_COMM_LEN_16 = 16,
    > + TASK_COMM_LEN = 24,
    > +};
    > +
    I don't think this is a good idea, to have it in tools/include/linux/,
    we have /usr/include/linux/sched.h, this may end up confusing the build
    at some point as your proposal is for a trimmed down header while what
    is in /usr/include/linux/sched.h doesn't have just this.

    But since we're using enums for this, we can't check for it with:

    #ifdef TASK_COMM_LEN_16
    #define TASK_COMM_LEN_16 16
    #endif

    ditto for TASK_COMM_LEN and be future proof, so I'd say just use
    hardcoded values in tools/perf/tests/evsel-tp-sched.c?

    - Arnaldo

    > +#endif /* _TOOLS_LINUX_SCHED_H */
    > diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
    > index f9e34bd26cf3..029f2a8c8e51 100644
    > --- a/tools/perf/tests/evsel-tp-sched.c
    > +++ b/tools/perf/tests/evsel-tp-sched.c
    > @@ -1,11 +1,13 @@
    > // SPDX-License-Identifier: GPL-2.0
    > #include <linux/err.h>
    > +#include <linux/sched.h>
    > #include <traceevent/event-parse.h>
    > #include "evsel.h"
    > #include "tests.h"
    > #include "debug.h"
    >
    > -static int evsel__test_field(struct evsel *evsel, const char *name, int size, bool should_be_signed)
    > +static int evsel__test_field_alt(struct evsel *evsel, const char *name,
    > + int size, int alternate_size, bool should_be_signed)
    > {
    > struct tep_format_field *field = evsel__field(evsel, name);
    > int is_signed;
    > @@ -23,15 +25,24 @@ static int evsel__test_field(struct evsel *evsel, const char *name, int size, bo
    > ret = -1;
    > }
    >
    > - if (field->size != size) {
    > - pr_debug("%s: \"%s\" size (%d) should be %d!\n",
    > + if (field->size != size && field->size != alternate_size) {
    > + pr_debug("%s: \"%s\" size (%d) should be %d",
    > evsel->name, name, field->size, size);
    > + if (alternate_size > 0)
    > + pr_debug(" or %d", alternate_size);
    > + pr_debug("!\n");
    > ret = -1;
    > }
    >
    > return ret;
    > }
    >
    > +static int evsel__test_field(struct evsel *evsel, const char *name,
    > + int size, bool should_be_signed)
    > +{
    > + return evsel__test_field_alt(evsel, name, size, -1, should_be_signed);
    > +}
    > +
    > int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtest __maybe_unused)
    > {
    > struct evsel *evsel = evsel__newtp("sched", "sched_switch");
    > @@ -42,7 +53,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
    > return -1;
    > }
    >
    > - if (evsel__test_field(evsel, "prev_comm", 16, false))
    > + if (evsel__test_field_alt(evsel, "prev_comm", TASK_COMM_LEN_16,
    > + TASK_COMM_LEN, false))
    > ret = -1;
    >
    > if (evsel__test_field(evsel, "prev_pid", 4, true))
    > @@ -54,7 +66,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
    > if (evsel__test_field(evsel, "prev_state", sizeof(long), true))
    > ret = -1;
    >
    > - if (evsel__test_field(evsel, "next_comm", 16, false))
    > + if (evsel__test_field_alt(evsel, "next_comm", TASK_COMM_LEN_16,
    > + TASK_COMM_LEN, false))
    > ret = -1;
    >
    > if (evsel__test_field(evsel, "next_pid", 4, true))
    > @@ -72,7 +85,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
    > return -1;
    > }
    >
    > - if (evsel__test_field(evsel, "comm", 16, false))
    > + if (evsel__test_field_alt(evsel, "comm", TASK_COMM_LEN_16,
    > + TASK_COMM_LEN, false))
    > ret = -1;
    >
    > if (evsel__test_field(evsel, "pid", 4, true))
    > --
    > 2.17.1

    --

    - Arnaldo

    \
     
     \ /
      Last update: 2021-11-17 15:32    [W:4.228 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site