Re: [PATCH 3/3] perf tools: Add numa_topology object

[Jiri Olsa <jolsa@redhat.com>]

On Mon, Feb 18, 2019 at 11:08:55PM +0900, Namhyung Kim wrote:
> Hi Jirka,
> 
> On Mon, Feb 18, 2019 at 8:39 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding numa_topology object to return the list of numa
> > nodes together with their cpus. It will replace the numa
> > code in header.c and will be used from perf record code
> > in following patches.
> >
> > Adding following interface functions to load numa details:
> >
> >   struct numa_topology *numa_topology__new(void);
> >   void numa_topology__delete(struct numa_topology *tp);
> >
> > And replacing current (copied) local interface, with no
> > functional changes.
> >
> > Link: http://lkml.kernel.org/n/tip-rn15st6hjj7txg2i88v7yff4@git.kernel.org
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/cputopo.c | 120 ++++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/cputopo.h |  16 +++++
> >  tools/perf/util/header.c  | 119 +++++++++----------------------------
> >  3 files changed, 162 insertions(+), 93 deletions(-)
> >
> > diff --git a/tools/perf/util/cputopo.c b/tools/perf/util/cputopo.c
> > index 84470ed4e707..daac3cf90b12 100644
> > --- a/tools/perf/util/cputopo.c
> > +++ b/tools/perf/util/cputopo.c
> > @@ -1,9 +1,11 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <sys/param.h>
> > +#include <inttypes.h>
> >
> >  #include "cputopo.h"
> >  #include "cpumap.h"
> >  #include "util.h"
> > +#include "env.h"
> >
> >
> >  #define CORE_SIB_FMT \
> > @@ -142,3 +144,121 @@ struct cpu_topology *cpu_topology__new(void)
> >         }
> >         return tp;
> >  }
> > +
> > +static int load_numa_node(struct numa_topology_node *node, int nr)
> > +{
> > +       char str[MAXPATHLEN];
> > +       char field[32];
> > +       char *buf = NULL, *p;
> > +       size_t len = 0;
> > +       int ret = -1;
> > +       FILE *fp;
> > +       u64 mem;
> > +
> > +       node->node = (u32) nr;
> > +
> > +       sprintf(str, "/sys/devices/system/node/node%d/meminfo", nr);
> 
> Why not using similar macro like NODE_MEMINFO_FMT?
> Also it'd be better using snprintf() or scnprintf() instead.

right, that code needs to be changed to take /sys from sysfs__mountpoint

but I wanted just to move the code now, with only necessary changes,
like the issues you found below ;-)

> 
> 
> > +       fp = fopen(str, "r");
> > +       if (!fp)
> > +               return -1;
> > +
> > +       while (getline(&buf, &len, fp) > 0) {
> > +               /* skip over invalid lines */
> > +               if (!strchr(buf, ':'))
> > +                       continue;
> > +               if (sscanf(buf, "%*s %*d %31s %"PRIu64, field, &mem) != 2)
> > +                       goto err;
> > +               if (!strcmp(field, "MemTotal:"))
> > +                       node->mem_total = mem;
> > +               if (!strcmp(field, "MemFree:"))
> > +                       node->mem_free = mem;
> > +               if (node->mem_total && node->mem_free)
> > +                       break;
> > +       }
> > +
> > +       fclose(fp);
> > +       fp  = NULL;
> > +       buf = NULL;
> 
> No need to make them NULL.  Also it'd leak buf..

oops, I'll put free in here

> 
> 
> > +
> > +       ret = -1;
> 
> It's not changed yet.

right, will change

thanks,
jirka