Re: [PATCH 04/13] perf env: Introduce read_cpu_topology_map() method

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Sep 11, 2015 at 11:40:28AM -0300, Arnaldo Carvalho de Melo escreveu:
> So this is a problem before and after my patches, i.e. If I go on and
> do, with what we have in acme/perf/core, i.e. none of the changes I'm
> playing with in perf/env:
> 
>   $ git remote update acme
>   Fetching acme
>   $ git checkout -b tmp acme/perf/core
>   Branch tmp set up to track remote branch perf/core from acme.
>   Switched to a new branch 'tmp'
>   $ git log --oneline | head -5
>   7e150fb33a91 perf tests: Introduce iterator function for tests
>   1765d9b26f84 perf test: Add entry for hists socket filter
>   207bb55e9193 perf hists browser: Zoom in/out for processor socket
>   9a2843a5f421 perf report: Introduce --socket-filter option
>   99851c76436a perf tools: Introduce new sort type "socket" for the processor socket
>   $ rm -rf /tmp/build/perf ; mkdir -p /tmp/build/perf ; make DEBUG=1 -C tools/perf O=/tmp/build/perf install-bin
>   # echo 0 > /sys/devices/system/cpu/cpu2/online
>   $ cat /sys/devices/system/cpu/cpu2/topology/core_id
>   cat: /sys/devices/system/cpu/cpu2/topology/core_id: No such file or directory
>   $ ls -la /sys/devices/system/cpu/cpu2/topology/
>   ls: cannot access /sys/devices/system/cpu/cpu2/topology/: No such file or directory
>   $ perf record usleep 1
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.012 MB perf.data (7 samples) ]
>   [acme@felicio linux]$ perf report --header-only -I 
>   # ========
>   # captured on: Fri Sep 11 11:34:18 2015
>   # hostname : felicio.ghostprotocols.net
>   # os release : 4.2.0
>   # perf version : 4.2.g7e150f
>   #  arch : x86_64
>   # nrcpus online : 4
>   # nrcpus avail : 3
>   # cpudesc : Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz
>   <SNIP>
>   # node0 meminfo  : total = 8085412 kB, free = 5317596 kB
>   # node0 cpu list : 0-1,3
>   <SNIP>
>   $ 

Stephane, Namhyung, the bug report below is about the online/avail
above, they are swapped, full explanation below.
 
> We can see multiple bugs here, right? online/avail is swapped, and when
> online != avail we simply do not record the cpu topology info at all!
 
> If we get that CPU back online:
> 
>   # echo 1 > /sys/devices/system/cpu/cpu2/online
> 
> Then all works:
> 
>   $ perf record usleep 1
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.012 MB perf.data (7 samples) ]
>   $ perf report --header-only -I
>   # ========
>   # captured on: Fri Sep 11 11:37:31 2015
>   # hostname : felicio.ghostprotocols.net
>   # os release : 4.2.0
>   # perf version : 4.2.g7e150f
>   # arch : x86_64
>   # nrcpus online : 4
>   # nrcpus avail : 4
>   # cpudesc : Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz
>   <SNIP>
>   # sibling cores   : 0-3
>   # sibling threads : 0
>   # sibling threads : 1
>   # sibling threads : 2
>   # sibling threads : 3
>   # CPU 0: Core ID 0, Socket ID 0
>   # CPU 1: Core ID 1, Socket ID 0
>   # CPU 2: Core ID 2, Socket ID 0
>   # CPU 3: Core ID 3, Socket ID 0
>   # node0 meminfo  : total = 8085412 kB, free = 5316992 kB
>   # node0 cpu list : 0-3
> 
> So, again, this was not introduced by this patchkit, but it is good that you did
> these offline tests, so we can fix it!

Stephane, this was introduced in:

  commit fbe96f29ce4b33e0a22219cc7f5996d9157717e3
  Author: Stephane Eranian <eranian@google.com>
  Date:   Fri Sep 30 15:40:40 2011 +0200

    perf tools: Make perf.data more self-descriptive (v8)

------------------

When you write this part:

      - HEADER_NRCPUS: number of online/avail cpus

You do:

static int write_nrcpus(int fd, struct perf_header *h __used,
                       struct perf_evlist *evlist __used)
{
       long nr;
       u32 nrc, nra;
       int ret;

       nr = sysconf(_SC_NPROCESSORS_CONF);
       if (nr < 0)
               return -1;

       nrc = (u32)(nr & UINT_MAX);

       nr = sysconf(_SC_NPROCESSORS_ONLN);
       if (nr < 0)
               return -1;

       nra = (u32)(nr & UINT_MAX);

       ret = do_write(fd, &nrc, sizeof(nrc));
       if (ret < 0)
               return ret;

       return do_write(fd, &nra, sizeof(nra));
}

I.e. write what you called 'nrc' using what is in SC_NRPROCESSORS_CONF, that in
the documentation for glibc reads:

---------------------

http://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html

  sysconf (_SC_NPROCESSORS_CONF)

which returns the number of processors the operating system configured. But it
might be possible for the operating system to disable individual processors and
so the call 

---------------------

Which menas "NR_AVAILABLE", right?

But then you call a variable 'nra' which sounds like you think that what is in
_SC_NPROCESSORS_ONLN is the "available" number of CPUs, which is confused a bit
more by the glibc docs when refering to _SC_NPROCESSORS_ONLN:

---------------------
  sysconf (_SC_NPROCESSORS_ONLN)

returns the number of processors which are currently online (i.e., available). 
---------------------

Then, when printing the number of CPUs encoded in the perf.data file by the
above write_nrcpus() routine, you did:

static void print_nrcpus(struct perf_header *ph, int fd, FILE *fp)
{
       ssize_t ret;
       u32 nr;

       ret = read(fd, &nr, sizeof(nr));
       if (ret != (ssize_t)sizeof(nr))
               nr = -1; /* interpreted as error */

       if (ph->needs_swap)
               nr = bswap_32(nr);

       fprintf(fp, "# nrcpus online : %u\n", nr);

       ret = read(fd, &nr, sizeof(nr));
       if (ret != (ssize_t)sizeof(nr))
               nr = -1; /* interpreted as error */

       if (ph->needs_swap)
               nr = bswap_32(nr);

       fprintf(fp, "# nrcpus avail : %u\n", nr);
}

You inverted it, no?

So, could you please check if the below patch can have your Acked-by?
Namhyung?

Thanks,

- Arnaldo

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 41814547da15..fce6634aebe2 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1438,7 +1438,7 @@ static int process_nrcpus(struct perf_file_section *section __maybe_unused,
 	if (ph->needs_swap)
 		nr = bswap_32(nr);
 
-	ph->env.nr_cpus_online = nr;
+	ph->env.nr_cpus_avail = nr;
 
 	ret = readn(fd, &nr, sizeof(nr));
 	if (ret != sizeof(nr))
@@ -1447,7 +1447,7 @@ static int process_nrcpus(struct perf_file_section *section __maybe_unused,
 	if (ph->needs_swap)
 		nr = bswap_32(nr);
 
-	ph->env.nr_cpus_avail = nr;
+	ph->env.nr_cpus_online = nr;
 	return 0;
 }