perf test topo broken?

perf test topo broken?

[Sukadev Bhattiprolu]

'perf test topo' is broken on my x86_64 system, with a rather cryptic message.

	$ ./perf test -v topo
	36: Test topology in session                                 :
	--- start ---
	test child forked, pid 2705
	templ file: /tmp/perf-test-6rSAkb
	core_id number is too big.  nr 9, cpu_nr 8. You may need to upgrade the perf tool.
	test child interrupted
	---- end ----
	Test topology in session: FAILED!

	$ ./perf --version
	perf version 4.4.rc1.g34258a

(I included nr and cpu_nr values in the error message). The code (shown
below) is comparing a core_id value (nr) with number of cpus online (cpu_nr)
which don't seem to be related on this system.

On my system running 4.4.0-rc1 kernel, I have following cpus:

	$ head /proc/cpuinfo 
	processor	: 0
	vendor_id	: GenuineIntel
	cpu family	: 6
	model		: 44
	model name	: Intel(R) Xeon(R) CPU           E5620  @ 2.40GHz
	stepping	: 2
	microcode	: 0x10
	cpu MHz		: 1596.000
	cache size	: 12288 KB
	physical id	: 0

	$ ls -d /sys/devices/system/cpu/cpu?
	/sys/devices/system/cpu/cpu0  /sys/devices/system/cpu/cpu4
	/sys/devices/system/cpu/cpu1  /sys/devices/system/cpu/cpu5
	/sys/devices/system/cpu/cpu2  /sys/devices/system/cpu/cpu6
	/sys/devices/system/cpu/cpu3  /sys/devices/system/cpu/cpu7

	$ cat /sys/devices/system/cpu/online 
	0-7

	$ cat /sys/devices/system/cpu/cpu?/topology/core_id
	0
	1
	9
	10
	0
	1
	9
	10

Commenting out following code seems to cause the test to pass, but are
core_ids in general related to number of cpus online?

Sukadev

---
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 4383800..d5104da 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1652,11 +1652,14 @@ static int process_cpu_topology(struct perf_file_section *section,
                if (ph->needs_swap)
                        nr = bswap_32(nr);
 
+#if 0
                if (nr > (u32)cpu_nr) {
-                       pr_debug("core_id number is too big."
-                                "You may need to upgrade the perf tool.\n");
+                       pr_debug("core_id number is too big.  nr %d, cpu_nr %d. "
+                                "You may need to upgrade the perf tool.\n",
+                                nr, cpu_nr);
                        goto free_cpu;
                }
+#endif
                ph->env.cpu[i].core_id = nr;
 
                ret = readn(fd, &nr, sizeof(nr));

Re: perf test topo broken?

[Jiri Olsa]

On Thu, Nov 19, 2015 at 02:13:53PM -0800, Sukadev Bhattiprolu wrote:

SNIP

> 
> 	$ cat /sys/devices/system/cpu/online 
> 	0-7
> 
> 	$ cat /sys/devices/system/cpu/cpu?/topology/core_id
> 	0
> 	1
> 	9
> 	10
> 	0
> 	1
> 	9
> 	10

we assume the actualy core_id number is within 0 - [cpu-max-number]

how can one reach above core_id setup? ;-)

thanks,
jirka

Re: perf test topo broken?

[Jiri Olsa]

On Thu, Nov 19, 2015 at 02:13:53PM -0800, Sukadev Bhattiprolu wrote:

SNIP

> Commenting out following code seems to cause the test to pass, but are
> core_ids in general related to number of cpus online?
> 
> Sukadev
> 
> ---
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 4383800..d5104da 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1652,11 +1652,14 @@ static int process_cpu_topology(struct perf_file_section *section,
>                 if (ph->needs_swap)
>                         nr = bswap_32(nr);
>  
> +#if 0
>                 if (nr > (u32)cpu_nr) {
> -                       pr_debug("core_id number is too big."
> -                                "You may need to upgrade the perf tool.\n");
> +                       pr_debug("core_id number is too big.  nr %d, cpu_nr %d. "
> +                                "You may need to upgrade the perf tool.\n",
> +                                nr, cpu_nr);
>                         goto free_cpu;
>                 }
> +#endif
>                 ph->env.cpu[i].core_id = nr;

looks like we can safely remove this check,

I don't see any place we use core_id as array index
or any other place assuming core_id < cpu_nr

Kan Liang?

thanks,
jirka

RE: perf test topo broken?

[Liang, Kan]

> 
> On Thu, Nov 19, 2015 at 02:13:53PM -0800, Sukadev Bhattiprolu wrote:
> 
> SNIP
> 
> > Commenting out following code seems to cause the test to pass, but are
> > core_ids in general related to number of cpus online?
> >
> > Sukadev
> >
> > ---
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c index
> > 4383800..d5104da 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -1652,11 +1652,14 @@ static int process_cpu_topology(struct
> perf_file_section *section,
> >                 if (ph->needs_swap)
> >                         nr = bswap_32(nr);
> >
> > +#if 0
> >                 if (nr > (u32)cpu_nr) {
> > -                       pr_debug("core_id number is too big."
> > -                                "You may need to upgrade the perf tool.\n");
> > +                       pr_debug("core_id number is too big.  nr %d, cpu_nr %d. "
> > +                                "You may need to upgrade the perf tool.\n",
> > +                                nr, cpu_nr);
> >                         goto free_cpu;
> >                 }
> > +#endif
> >                 ph->env.cpu[i].core_id = nr;
> 
> looks like we can safely remove this check,
> 
> I don't see any place we use core_id as array index or any other place
> assuming core_id < cpu_nr
> 
> Kan Liang?

I assumed that the core_id should be less than max_cpu_number.
But in your case it looks the assumption doesn't work.

I think we can safely remove the check as Jirka suggested.


Thanks,
Kan