linux-kernel - Re: [PATCH] perf bench: add benchmark for evlist open/close operations

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:   Tue, 10 Aug 2021 12:31:55 +0200
From:   Riccardo Mancini <rickyman7@...il.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     Ian Rogers <irogers@...gle.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Jiri Olsa <jolsa@...hat.com>, linux-kernel@...r.kernel.org,
        linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] perf bench: add benchmark for evlist open/close
 operations

Hi Arnaldo,

On Mon, 2021-08-09 at 17:28 -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Aug 09, 2021 at 05:23:31PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Aug 09, 2021 at 10:11:02PM +0200, Riccardo Mancini escreveu:
> > > +static struct evlist *bench__create_evlist(char *evstr)
> > > +{
> > > +       struct evlist *evlist;
> > > +       struct parse_events_error err;
> > > +       int ret;
> 
> > > +       evlist = evlist__new();
> > > +       if (!evlist) {
> > > +               pr_err("Not enough memory to create evlist\n");
> > > +               return NULL;
> > > +       }
> 
> > > +       bzero(&err, sizeof(err));
> 
> > man bzero
> > 
> >        The bzero() function is deprecated (marked as LEGACY in POSIX.1-2001);
> > use memset(3) in new programs.  POSIX.1-2008 removes the specification of
> > bzero().  The bzero() function first appeared in 4.3BSD.

Oops, I didn't know, but I saw it is being used in some parts in perf, maybe we
should get rid of them:
$ rg -c bzero
builtin-lock.c:1
arch/powerpc/util/kvm-stat.c:1
builtin-stat.c:1
builtin-trace.c:2
bench/evlist-open-close.c:1
bench/numa.c:5
tests/parse-events.c:1
tests/backward-ring-buffer.c:1
tests/bpf.c:2
util/metricgroup.c:1
util/parse-events.c:1

> 
> > I'm replacing it with a memset().
> 
> This one is also equivalent:
> 
> tools/perf/tests/pmu-events.c:  struct parse_events_error error = { .idx = 0, };
> 
> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
> 
> That text is a bit roundabout, as it says that the members that are not
> explicitely initialized will be initialized as variables with static
> storage duration, i.e. zeroed.

Would it be the same doing the shorter {0}. It would be a general solution for
these init-to-zero cases.

Unrelated to this small issue, I noticed I forgot to check the return of
bench__create_evlist. Would you like me to send a v2 fixing both issues or are
you able to apply this other small change yourself?

diff --git a/tools/perf/bench/evlist-open-close.c b/tools/perf/bench/evlist-open-close.c
index 40bce06f5ca7bef3..f0b9c330f34f2984 100644
--- a/tools/perf/bench/evlist-open-close.c
+++ b/tools/perf/bench/evlist-open-close.c
@@ -168,7 +168,11 @@ static int bench_evlist_open_close__run(char *evstr)
 
        for (i = 0; i < iterations; i++) {
                pr_debug("Started iteration %d\n", i);
+
                evlist = bench__create_evlist(evstr);
+               if (!evlist)
+                       return -ENOMEM;
+
                gettimeofday(&start, NULL);
                err = bench__do_evlist_open_close(evlist);
                if (err) {

Thanks,
Riccardo

> 
> - Arnaldo