| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CALOAHbC1Cn7RA_X5TrKQb9nmKMxuinfh+Z9j51yMoaSBPx3DuA@mail.gmail.com>
Date: Thu, 18 Nov 2021 22:18:42 +0800
From: Yafang Shao <laoar.shao@...il.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Kees Cook <keescook@...omium.org>,
Steven Rostedt <rostedt@...dmis.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Petr Mladek <pmladek@...e.com>,
Peter Zijlstra <peterz@...radead.org>,
Al Viro <viro@...iv.linux.org.uk>,
Valentin Schneider <valentin.schneider@....com>,
Qiang Zhang <qiang.zhang@...driver.com>,
robdclark <robdclark@...omium.org>,
christian <christian@...uner.io>,
Dietmar Eggemann <dietmar.eggemann@....com>,
Ingo Molnar <mingo@...hat.com>,
Juri Lelli <juri.lelli@...hat.com>,
Vincent Guittot <vincent.guittot@...aro.org>,
David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
john fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>,
Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>,
mike.marciniszyn@...nelisnetworks.com, dledford@...hat.com,
jgg@...pe.ca, linux-rdma@...r.kernel.org,
netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
"linux-perf-use." <linux-perf-users@...r.kernel.org>,
linux-fsdevel@...r.kernel.org, Linux MM <linux-mm@...ck.org>,
LKML <linux-kernel@...r.kernel.org>,
kernel test robot <oliver.sang@...el.com>,
kbuild test robot <lkp@...el.com>,
Alexei Starovoitov <alexei.starovoitov@...il.com>,
Andrii Nakryiko <andrii.nakryiko@...il.com>
Subject: Re: [PATCH v7 08/11] tools/perf/test: make perf test adopt to task
comm size change
On Wed, Nov 17, 2021 at 10:31 PM Arnaldo Carvalho de Melo
<arnaldo.melo@...il.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@...el.com>
> > Suggested-by: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
> > Signed-off-by: Yafang Shao <laoar.shao@...il.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
> > Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>
> > Cc: Andrii Nakryiko <andrii.nakryiko@...il.com>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Steven Rostedt <rostedt@...dmis.org>
> > Cc: Al Viro <viro@...iv.linux.org.uk>
> > Cc: Kees Cook <keescook@...omium.org>
> > Cc: Petr Mladek <pmladek@...e.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
--
Thanks
Yafang
Powered by blists - more mailing lists