linux-kernel - Re: [PATCH 1/3] perf header: Add die information in CPU topology

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] [day] [month] [year] [list]

Message-ID: <20190528192942.GJ10611@krava>
Date:   Tue, 28 May 2019 21:29:42 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     "Liang, Kan" <kan.liang@...ux.intel.com>
Cc:     acme@...nel.org, jolsa@...nel.org, mingo@...hat.com,
        linux-kernel@...r.kernel.org, peterz@...radead.org,
        ak@...ux.intel.com
Subject: Re: [PATCH 1/3] perf header: Add die information in CPU topology

On Tue, May 28, 2019 at 03:06:16PM -0400, Liang, Kan wrote:
> 
> 
> On 5/28/2019 5:00 AM, Jiri Olsa wrote:
> > On Thu, May 23, 2019 at 01:41:19PM -0700, kan.liang@...ux.intel.com wrote:
> > 
> > SNIP
> > 
> > > diff --git a/tools/perf/util/cputopo.c b/tools/perf/util/cputopo.c
> > > index ece0710..f6e7db7 100644
> > > --- a/tools/perf/util/cputopo.c
> > > +++ b/tools/perf/util/cputopo.c
> > > @@ -1,5 +1,6 @@
> > >   // SPDX-License-Identifier: GPL-2.0
> > >   #include <sys/param.h>
> > > +#include <sys/utsname.h>
> > >   #include <inttypes.h>
> > >   #include <api/fs/fs.h>
> > > @@ -8,9 +9,10 @@
> > >   #include "util.h"
> > >   #include "env.h"
> > > -
> > >   #define CORE_SIB_FMT \
> > >   	"%s/devices/system/cpu/cpu%d/topology/core_siblings_list"
> > > +#define DIE_SIB_FMT \
> > > +	"%s/devices/system/cpu/cpu%d/topology/die_cpus_list"
> > >   #define THRD_SIB_FMT \
> > >   	"%s/devices/system/cpu/cpu%d/topology/thread_siblings_list"
> > >   #define NODE_ONLINE_FMT \
> > > @@ -20,7 +22,26 @@
> > >   #define NODE_CPULIST_FMT \
> > >   	"%s/devices/system/node/node%d/cpulist"
> > > -static int build_cpu_topology(struct cpu_topology *tp, int cpu)
> > > +bool check_x86_die_exists(void)
> > > +{
> > > +	char filename[MAXPATHLEN];
> > > +	struct utsname uts;
> > > +
> > > +	if (uname(&uts) < 0)
> > > +		return false;
> > > +
> > > +	if (strncmp(uts.machine, "x86_64", 6))
> > > +		return false;
> > > +
> > > +	scnprintf(filename, MAXPATHLEN, DIE_SIB_FMT,
> > > +		  sysfs__mountpoint(), 0);
> > > +	if (access(filename, F_OK) == -1)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > 
> > we could rename this to:
> > 
> > static bool has_die(void)
> > {
> > 	static bool has_die;
> > 
> > 	if (initialized)
> > 		return has_die;
> > 
> > 	has_die = ...
> > 	initialized = true;
> > }
> > 
> > and got rid of all those 'has_die' arguments below
> 
> Yes, we can rename the function to has_die(). It looks like all the
> 'has_die' arguments can be replaced either.
> 
> But why we want a "initialized" here? to cache the value? It looks like we
> only need to call has_die() once.

right, if it's called from one place then it's ok

thanks,
jirka