| Message ID | 20211101060419.4682-9-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | extend task comm from 16 to 24 | expand |
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
On Wed, Nov 17, 2021 at 10:31 PM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > 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? > Hi Arnaldo, Thanks for the review. This perf tests code won't be changed in the latest version as we don't want to extend comm size any more, see also https://lore.kernel.org/lkml/20211108083840.4627-1-laoar.shao@gmail.com/ The hard-coded 16 in tools/perf/tests/evsel-tp-sched.c is kept as-is. > > > +#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
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, +}; + +#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))
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