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

[Jiri Olsa <jolsa@redhat.com>]

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@linux.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