linux-kernel - Re: [PATCH 01/12] perf test: Add -w/--workload option

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 9 Nov 2022 11:00:15 -0800
From:   Namhyung Kim <namhyung@...nel.org>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-perf-users@...r.kernel.org, Leo Yan <leo.yan@...aro.org>,
        German Gomez <german.gomez@....com>,
        Zhengjun Xing <zhengjun.xing@...ux.intel.com>,
        James Clark <james.clark@....com>
Subject: Re: [PATCH 01/12] perf test: Add -w/--workload option

Hi Arnaldo,

On Wed, Nov 9, 2022 at 10:39 AM Arnaldo Carvalho de Melo
<acme@...nel.org> wrote:
>
> Em Wed, Nov 09, 2022 at 09:46:24AM -0800, Namhyung Kim escreveu:
> > --- /dev/null
> > +++ b/tools/perf/tests/workloads/noploop.c
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#include <stdlib.h>
> > +#include <signal.h>
> > +#include <unistd.h>
> > +#include <linux/compiler.h>
> > +#include "../tests.h"
> > +
> > +static volatile int done;
> > +
> > +static void sighandler(int sig __maybe_unused)
> > +{
> > +     done = 1;
> > +}
>
> You forgot to do what was done in:

Oops, right.  Will fix in v2.

Thanks,
Namhyung


>
> 92ea0720ba9cf7f0 perf trace: Use sig_atomic_t to avoid undefined behaviour in a signal handler
> 691768968f2a13eb perf top: Use sig_atomic_t to avoid undefined behaviour in a signal handler
> 01513fdc18f395db perf stat: Use sig_atomic_t to avoid undefined behaviour in a signal handler
> 057929f9d083e80c perf session: Change type to avoid undefined behaviour in a signal handler
> 853596fb71f7c2f7 perf ftrace: Use sig_atomic_t to avoid UB
> 7f3374299f9762ba perf daemon: Use sig_atomic_t to avoid UB
> 8ed28c2b56b78442 perf record: Use sig_atomic_t for signal handlers
> f3c9bd4e16a503cb perf build: Update to C standard to gnu11
>
> To speed up the process here is one of those csets:
>
> ⬢[acme@...lbox perf]$ git show 01513fdc18f395db
> commit 01513fdc18f395dbcc924bc5e9962b12f86f947a
> Author: Ian Rogers <irogers@...gle.com>
> Date:   Mon Oct 24 11:19:11 2022 -0700
>
>     perf stat: Use sig_atomic_t to avoid undefined behaviour in a signal handler
>
>     Use sig_atomic_t for variables written/accessed in signal
>     handlers. This is undefined behavior as per:
>
>       https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
>
>     Signed-off-by: Ian Rogers <irogers@...gle.com>
>     Cc: Adrian Hunter <adrian.hunter@...el.com>
>     Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
>     Cc: Alexey Bayduraev <alexey.v.bayduraev@...ux.intel.com>
>     Cc: German Gomez <german.gomez@....com>
>     Cc: Ingo Molnar <mingo@...hat.com>
>     Cc: Jiri Olsa <jolsa@...nel.org>
>     Cc: Leo Yan <leo.yan@...aro.org>
>     Cc: Mark Rutland <mark.rutland@....com>
>     Cc: Namhyung Kim <namhyung@...nel.org>
>     Cc: Peter Zijlstra <peterz@...radead.org>
>     Cc: Stephane Eranian <eranian@...gle.com>
>     Link: https://lore.kernel.org/r/20221024181913.630986-7-irogers@google.com
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index e52601a54b26d669..d5e1670bca204450 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -173,7 +173,7 @@ static struct target target = {
>
>  #define METRIC_ONLY_LEN 20
>
> -static volatile pid_t          child_pid                       = -1;
> +static volatile sig_atomic_t   child_pid                       = -1;
>  static int                     detailed_run                    =  0;
>  static bool                    transaction_run;
>  static bool                    topdown_run                     = false;
> @@ -208,7 +208,7 @@ struct perf_stat {
>  static struct perf_stat                perf_stat;
>  #define STAT_RECORD            perf_stat.record
>
> -static volatile int done = 0;
> +static volatile sig_atomic_t done = 0;
>
>  static struct perf_stat_config stat_config = {
>         .aggr_mode              = AGGR_GLOBAL,
> @@ -580,7 +580,7 @@ static void disable_counters(void)
>         }
>  }
>
> -static volatile int workload_exec_errno;
> +static volatile sig_atomic_t workload_exec_errno;
>
>  /*
>   * evlist__prepare_workload will send a SIGUSR1
> @@ -1039,7 +1039,7 @@ static void print_counters(struct timespec *ts, int argc, const char **argv)
>         evlist__print_counters(evsel_list, &stat_config, &target, ts, argc, argv);
>  }
>
> -static volatile int signr = -1;
> +static volatile sig_atomic_t signr = -1;
>
>  static void skip_signal(int signo)
>  {
> ⬢[acme@...lbox perf]$
>
> > +
> > +static int noploop(int argc, const char **argv)
> > +{
> > +     int sec = 1;
> > +
> > +     if (argc > 0)
> > +             sec = atoi(argv[0]);
> > +
> > +     signal(SIGINT, sighandler);
> > +     signal(SIGALRM, sighandler);
> > +     alarm(sec);
> > +
> > +     while (!done)
> > +             continue;
> > +
> > +     return 0;
> > +}
> > +
> > +DEFINE_WORKLOAD(noploop);
> > --
> > 2.38.1.431.g37b22c650d-goog
>
> --
>
> - Arnaldo