arm/arm64 perf build issue with mainline

arm/arm64 perf build issue with mainline

[Will Deacon]

Hi all,

Commit 6428c59a97de ("perf tools: Set JOBS based on CPU or processor")
causes weird behaviour on arm/arm64 platforms because we use the "CPU"
prefix for things like:

CPU implementer : 0x41
CPU architecture: 8
CPU variant     : 0x0
CPU part        : 0xd03
CPU revision    : 0

in /proc/cpuinfo. Consequently, a 6 core machine ends up doing:

will@confinement-loaf:~/linux/tools/perf$ make
  BUILD:   Doing 'make -j36' parallel build

which is a little overwhelming. Any chance we can predicate the extra
part of the regex on $(ARCH) being sparc?

Will

Re: arm/arm64 perf build issue with mainline

[Will Deacon]

On Thu, Apr 23, 2015 at 12:29:16PM +0100, Will Deacon wrote:
> Hi all,
> 
> Commit 6428c59a97de ("perf tools: Set JOBS based on CPU or processor")
> causes weird behaviour on arm/arm64 platforms because we use the "CPU"
> prefix for things like:
> 
> CPU implementer : 0x41
> CPU architecture: 8
> CPU variant     : 0x0
> CPU part        : 0xd03
> CPU revision    : 0
> 
> in /proc/cpuinfo. Consequently, a 6 core machine ends up doing:
> 
> will@confinement-loaf:~/linux/tools/perf$ make
>   BUILD:   Doing 'make -j36' parallel build
> 
> which is a little overwhelming. Any chance we can predicate the extra
> part of the regex on $(ARCH) being sparc?

Scratch that, how about using sysconf instead? Patch below...

Will

--->8

>From 28740111e81aa9247bf48e3125dc43cc31d94e6f Mon Sep 17 00:00:00 2001
From: Will Deacon <will.deacon@arm.com>
Date: Thu, 23 Apr 2015 15:00:16 +0100
Subject: [PATCH] tools: perf: use getconf to determine number of online CPUs

Parsing /proc/cpuinfo is a fiddly, arch-dependent business anda recent
change to get it working for Sparc broke arm and arm64 platforms.

Instead, use sysconf to determine the number of online CPUs and avoid
parsing /proc/cpuinfo entirely.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 tools/perf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index c699dc35eef9..c26cb04ce6bd 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -24,7 +24,7 @@ unexport MAKEFLAGS
 # (To override it, run 'make JOBS=1' and similar.)
 #
 ifeq ($(JOBS),)
-  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
+  JOBS := $(shell getconf _NPROCESSORS_ONLN 2>/dev/null)
   ifeq ($(JOBS),0)
     JOBS := 1
   endif
-- 
2.1.4

Re: arm/arm64 perf build issue with mainline

[David Ahern]

On 4/23/15 8:04 AM, Will Deacon wrote:
>  From 28740111e81aa9247bf48e3125dc43cc31d94e6f Mon Sep 17 00:00:00 2001
> From: Will Deacon <will.deacon@arm.com>
> Date: Thu, 23 Apr 2015 15:00:16 +0100
> Subject: [PATCH] tools: perf: use getconf to determine number of online CPUs
>
> Parsing /proc/cpuinfo is a fiddly, arch-dependent business anda recent
> change to get it working for Sparc broke arm and arm64 platforms.
>
> Instead, use sysconf to determine the number of online CPUs and avoid
> parsing /proc/cpuinfo entirely.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>   tools/perf/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index c699dc35eef9..c26cb04ce6bd 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -24,7 +24,7 @@ unexport MAKEFLAGS
>   # (To override it, run 'make JOBS=1' and similar.)
>   #
>   ifeq ($(JOBS),)
> -  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
> +  JOBS := $(shell getconf _NPROCESSORS_ONLN 2>/dev/null)
>     ifeq ($(JOBS),0)
>       JOBS := 1
>     endif
>

Certainly a more robust way of doing it but I am concerned this might 
end up breaking others. For Fedora at least getconf is in the 
glibc-common package and there are users that do not build with glibc. 
It is not clear if the build system for those environments will have 
getconf.

Re: arm/arm64 perf build issue with mainline

[Arnaldo Carvalho de Melo]

Em Thu, Apr 23, 2015 at 08:16:15AM -0600, David Ahern escreveu:
> On 4/23/15 8:04 AM, Will Deacon wrote:
> >+++ b/tools/perf/Makefile
> >@@ -24,7 +24,7 @@ unexport MAKEFLAGS
> >  # (To override it, run 'make JOBS=1' and similar.)
> >  #
> >  ifeq ($(JOBS),)
> >-  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
> >+  JOBS := $(shell getconf _NPROCESSORS_ONLN 2>/dev/null)
> >    ifeq ($(JOBS),0)
> >      JOBS := 1
> >    endif
 
> Certainly a more robust way of doing it but I am concerned this
> might end up breaking others. For Fedora at least getconf is in the
> glibc-common package and there are users that do not build with
> glibc. It is not clear if the build system for those environments
> will have getconf.

Hum, perhaps check if getconf is present, if not fallback to the egrep
-c? I'll try.

- Arnaldo

Re: arm/arm64 perf build issue with mainline

[Will Deacon]

On Fri, Apr 24, 2015 at 05:10:58PM +0100, Arnaldo Carvalho de Melo wrote:
> Em Thu, Apr 23, 2015 at 08:16:15AM -0600, David Ahern escreveu:
> > On 4/23/15 8:04 AM, Will Deacon wrote:
> > >+++ b/tools/perf/Makefile
> > >@@ -24,7 +24,7 @@ unexport MAKEFLAGS
> > >  # (To override it, run 'make JOBS=1' and similar.)
> > >  #
> > >  ifeq ($(JOBS),)
> > >-  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
> > >+  JOBS := $(shell getconf _NPROCESSORS_ONLN 2>/dev/null)
> > >    ifeq ($(JOBS),0)
> > >      JOBS := 1
> > >    endif
>  
> > Certainly a more robust way of doing it but I am concerned this
> > might end up breaking others. For Fedora at least getconf is in the
> > glibc-common package and there are users that do not build with
> > glibc. It is not clear if the build system for those environments
> > will have getconf.
> 
> Hum, perhaps check if getconf is present, if not fallback to the egrep
> -c? I'll try.

I don't have a SPARC machine to hand, but we could check that the character
immediately following "CPU" is a number [0-9].

Will

Re: arm/arm64 perf build issue with mainline

[David Ahern]

On 4/24/15 10:22 AM, Will Deacon wrote:
> I don't have a SPARC machine to hand, but we could check that the character
> immediately following "CPU" is a number [0-9].

I have access to 1 or 2 or ...

Yes, it needs start with CPU and be all caps (there are other Cpu lines) 
and a cpu number follows:

CPU0:           online
CPU1:           online
CPU2:           online
CPU3:           online
...

David

Re: arm/arm64 perf build issue with mainline

[Arnaldo Carvalho de Melo]

Em Fri, Apr 24, 2015 at 11:21:54AM -0600, David Ahern escreveu:
> On 4/24/15 10:22 AM, Will Deacon wrote:
> >I don't have a SPARC machine to hand, but we could check that the character
> >immediately following "CPU" is a number [0-9].
> 
> I have access to 1 or 2 or ...
> 
> Yes, it needs start with CPU and be all caps (there are other Cpu lines) and
> a cpu number follows:
> 
> CPU0:           online
> CPU1:           online
> CPU2:           online
> CPU3:           online
> ...

Ok, any progress here?

- Arnaldo

Re: arm/arm64 perf build issue with mainline

[David Ahern]

On 4/27/15 10:26 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 24, 2015 at 11:21:54AM -0600, David Ahern escreveu:
>> On 4/24/15 10:22 AM, Will Deacon wrote:
>>> I don't have a SPARC machine to hand, but we could check that the character
>>> immediately following "CPU" is a number [0-9].
>>
>> I have access to 1 or 2 or ...
>>
>> Yes, it needs start with CPU and be all caps (there are other Cpu lines) and
>> a cpu number follows:
>>
>> CPU0:           online
>> CPU1:           online
>> CPU2:           online
>> CPU3:           online
>> ...
>
> Ok, any progress here?

I think the right thing to do is to remove JOBS completely.

Barring that use of getconf with a fallback to grepping /proc/cpuinfo.

David

Re: arm/arm64 perf build issue with mainline

[Arnaldo Carvalho de Melo]

Em Mon, Apr 27, 2015 at 11:13:02AM -0600, David Ahern escreveu:
> On 4/27/15 10:26 AM, Arnaldo Carvalho de Melo wrote:
> >Em Fri, Apr 24, 2015 at 11:21:54AM -0600, David Ahern escreveu:
> >>On 4/24/15 10:22 AM, Will Deacon wrote:
> >>>I don't have a SPARC machine to hand, but we could check that the character
> >>>immediately following "CPU" is a number [0-9].
> >>
> >>I have access to 1 or 2 or ...
> >>
> >>Yes, it needs start with CPU and be all caps (there are other Cpu lines) and
> >>a cpu number follows:
> >>
> >>CPU0:           online
> >>CPU1:           online
> >>CPU2:           online
> >>CPU3:           online
> >>...
> >
> >Ok, any progress here?
> 
> I think the right thing to do is to remove JOBS completely.

That is up for discussion, Ingo, Jiri, Namhyung, others?
 
> Barring that use of getconf with a fallback to grepping /proc/cpuinfo.

Ok, but no patch so far doing that, right? 8-)

- Arnaldo

Re: arm/arm64 perf build issue with mainline

[Jiri Olsa]

On Mon, Apr 27, 2015 at 02:40:17PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Apr 27, 2015 at 11:13:02AM -0600, David Ahern escreveu:
> > On 4/27/15 10:26 AM, Arnaldo Carvalho de Melo wrote:
> > >Em Fri, Apr 24, 2015 at 11:21:54AM -0600, David Ahern escreveu:
> > >>On 4/24/15 10:22 AM, Will Deacon wrote:
> > >>>I don't have a SPARC machine to hand, but we could check that the character
> > >>>immediately following "CPU" is a number [0-9].
> > >>
> > >>I have access to 1 or 2 or ...
> > >>
> > >>Yes, it needs start with CPU and be all caps (there are other Cpu lines) and
> > >>a cpu number follows:
> > >>
> > >>CPU0:           online
> > >>CPU1:           online
> > >>CPU2:           online
> > >>CPU3:           online
> > >>...
> > >
> > >Ok, any progress here?
> > 
> > I think the right thing to do is to remove JOBS completely.
> 
> That is up for discussion, Ingo, Jiri, Namhyung, others?
>  
> > Barring that use of getconf with a fallback to grepping /proc/cpuinfo.
> 
> Ok, but no patch so far doing that, right? 8-)

heh, I've got the idea that you're preparing something ;-)

as for me I've got used to the automatic -jX being added,
so I'd prefer we fix that JOBS setup for arm

I'll try to send something ;-)

jirka

Re: arm/arm64 perf build issue with mainline

[Arnaldo Carvalho de Melo]

Em Mon, Apr 27, 2015 at 07:49:06PM +0200, Jiri Olsa escreveu:
> On Mon, Apr 27, 2015 at 02:40:17PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Apr 27, 2015 at 11:13:02AM -0600, David Ahern escreveu:
> > > 
> > > I think the right thing to do is to remove JOBS completely.
> > 
> > That is up for discussion, Ingo, Jiri, Namhyung, others?
> >  
> > > Barring that use of getconf with a fallback to grepping /proc/cpuinfo.
> > 
> > Ok, but no patch so far doing that, right? 8-)
> 
> heh, I've got the idea that you're preparing something ;-)
> 
> as for me I've got used to the automatic -jX being added,
> so I'd prefer we fix that JOBS setup for arm
> 
> I'll try to send something ;-)

[acme@ssdandy linux]$ (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU' /proc/cpuinfo) 2>/dev/null
8
[acme@ssdandy linux]$ (not-found-getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU' /proc/cpuinfo) 2>/dev/null
8
[acme@ssdandy linux]$ 

Testing it in the makefile...

- Arnaldo

Re: arm/arm64 perf build issue with mainline

[Jiri Olsa]

On Mon, Apr 27, 2015 at 03:39:53PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Apr 27, 2015 at 07:49:06PM +0200, Jiri Olsa escreveu:
> > On Mon, Apr 27, 2015 at 02:40:17PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Apr 27, 2015 at 11:13:02AM -0600, David Ahern escreveu:
> > > > 
> > > > I think the right thing to do is to remove JOBS completely.
> > > 
> > > That is up for discussion, Ingo, Jiri, Namhyung, others?
> > >  
> > > > Barring that use of getconf with a fallback to grepping /proc/cpuinfo.
> > > 
> > > Ok, but no patch so far doing that, right? 8-)
> > 
> > heh, I've got the idea that you're preparing something ;-)
> > 
> > as for me I've got used to the automatic -jX being added,
> > so I'd prefer we fix that JOBS setup for arm
> > 
> > I'll try to send something ;-)
> 
> [acme@ssdandy linux]$ (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU' /proc/cpuinfo) 2>/dev/null
> 8
> [acme@ssdandy linux]$ (not-found-getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU' /proc/cpuinfo) 2>/dev/null
> 8
> [acme@ssdandy linux]$ 
> 

how about this one

jirka

---
diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index c699dc35eef9..ed79b10de1d6 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -24,7 +24,7 @@ unexport MAKEFLAGS
 # (To override it, run 'make JOBS=1' and similar.)
 #
 ifeq ($(JOBS),)
-  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
+  JOBS := $(shell ./ncpus.sh)
   ifeq ($(JOBS),0)
     JOBS := 1
   endif
diff --git a/tools/perf/ncpus.sh b/tools/perf/ncpus.sh
new file mode 100755
index 000000000000..4466b657336b
--- /dev/null
+++ b/tools/perf/ncpus.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+
+set -x
+
+GETCONF=`which getconf 2>/dev/null`
+if [ $? -eq 0 ]; then
+	$GETCONF _NPROCESSORS_ONLN
+	exit 0
+fi
+
+egrep -c '^processor|^CPU[0-9]' /proc/cpuinfo 2>/dev/null

Re: arm/arm64 perf build issue with mainline

[Arnaldo Carvalho de Melo]

Em Mon, Apr 27, 2015 at 08:41:20PM +0200, Jiri Olsa escreveu:
> On Mon, Apr 27, 2015 at 03:39:53PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Apr 27, 2015 at 07:49:06PM +0200, Jiri Olsa escreveu:
> > > On Mon, Apr 27, 2015 at 02:40:17PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Mon, Apr 27, 2015 at 11:13:02AM -0600, David Ahern escreveu:
> > > > > 
> > > > > I think the right thing to do is to remove JOBS completely.
> > > > 
> > > > That is up for discussion, Ingo, Jiri, Namhyung, others?
> > > >  
> > > > > Barring that use of getconf with a fallback to grepping /proc/cpuinfo.
> > > > 
> > > > Ok, but no patch so far doing that, right? 8-)
> > > 
> > > heh, I've got the idea that you're preparing something ;-)
> > > 
> > > as for me I've got used to the automatic -jX being added,
> > > so I'd prefer we fix that JOBS setup for arm
> > > 
> > > I'll try to send something ;-)
> > 
> > [acme@ssdandy linux]$ (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU' /proc/cpuinfo) 2>/dev/null
> > 8
> > [acme@ssdandy linux]$ (not-found-getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU' /proc/cpuinfo) 2>/dev/null
> > 8
> > [acme@ssdandy linux]$ 
> > 
> 
> how about this one

I came up with this one, that doesn't introduces a new file:

>From e8155c06652a05f2307d53823a7937be5dad4e32 Mon Sep 17 00:00:00 2001
From: Will Deacon <will.deacon@arm.com>
Date: Thu, 23 Apr 2015 15:00:16 +0100
Subject: [PATCH] tools perf: Use getconf to determine number of online CPUs

Parsing /proc/cpuinfo is a fiddly, arch-dependent business anda recent
change to get it working for Sparc broke arm and arm64 platforms.

Use sysconf to determine the number of online CPUs only parsing
/proc/cpuinfo when sysconf is not available.

Signed-off-by: Will Deacon <will.deacon@arm.com>
Cc: David Ahern <david.ahern@oracle.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Mark Rutland <Mark.Rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/r/20150423140454.GJ1652@arm.com
[ Made it fall back to parsing /proc when getconf not found ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index c699dc35eef9..bc846b83c295 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -24,7 +24,7 @@ unexport MAKEFLAGS
 # (To override it, run 'make JOBS=1' and similar.)
 #
 ifeq ($(JOBS),)
-  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
+  JOBS := $(shell (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU' /proc/cpuinfo) 2>/dev/null)
   ifeq ($(JOBS),0)
     JOBS := 1
   endif
-- 
1.8.3.1

Re: arm/arm64 perf build issue with mainline

[Jiri Olsa]

On Mon, Apr 27, 2015 at 03:45:35PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > > [acme@ssdandy linux]$ 
> > > 
> > 
> > how about this one
> 
> I came up with this one, that doesn't introduces a new file:

ook

> 
> From e8155c06652a05f2307d53823a7937be5dad4e32 Mon Sep 17 00:00:00 2001
> From: Will Deacon <will.deacon@arm.com>
> Date: Thu, 23 Apr 2015 15:00:16 +0100
> Subject: [PATCH] tools perf: Use getconf to determine number of online CPUs
> 
> Parsing /proc/cpuinfo is a fiddly, arch-dependent business anda recent
> change to get it working for Sparc broke arm and arm64 platforms.
> 
> Use sysconf to determine the number of online CPUs only parsing
> /proc/cpuinfo when sysconf is not available.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> Cc: David Ahern <david.ahern@oracle.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Mark Rutland <Mark.Rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Link: http://lkml.kernel.org/r/20150423140454.GJ1652@arm.com
> [ Made it fall back to parsing /proc when getconf not found ]
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index c699dc35eef9..bc846b83c295 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -24,7 +24,7 @@ unexport MAKEFLAGS
>  # (To override it, run 'make JOBS=1' and similar.)
>  #
>  ifeq ($(JOBS),)
> -  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
> +  JOBS := $(shell (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU' /proc/cpuinfo) 2>/dev/null)

how about the 'CPU[0-9]' someone asked for?

jirka

Re: arm/arm64 perf build issue with mainline

[Arnaldo Carvalho de Melo]

Em Mon, Apr 27, 2015 at 08:52:08PM +0200, Jiri Olsa escreveu:
> On Mon, Apr 27, 2015 at 03:45:35PM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > > > [acme@ssdandy linux]$ 
> > > > 
> > > 
> > > how about this one
> > 
> > I came up with this one, that doesn't introduces a new file:
> 
> ook
> 
> > 
> > From e8155c06652a05f2307d53823a7937be5dad4e32 Mon Sep 17 00:00:00 2001
> > From: Will Deacon <will.deacon@arm.com>
> > Date: Thu, 23 Apr 2015 15:00:16 +0100
> > Subject: [PATCH] tools perf: Use getconf to determine number of online CPUs
> > 
> > Parsing /proc/cpuinfo is a fiddly, arch-dependent business anda recent
> > change to get it working for Sparc broke arm and arm64 platforms.
> > 
> > Use sysconf to determine the number of online CPUs only parsing
> > /proc/cpuinfo when sysconf is not available.
> > 
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > Cc: David Ahern <david.ahern@oracle.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Mark Rutland <Mark.Rutland@arm.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Link: http://lkml.kernel.org/r/20150423140454.GJ1652@arm.com
> > [ Made it fall back to parsing /proc when getconf not found ]
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  tools/perf/Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> > index c699dc35eef9..bc846b83c295 100644
> > --- a/tools/perf/Makefile
> > +++ b/tools/perf/Makefile
> > @@ -24,7 +24,7 @@ unexport MAKEFLAGS
> >  # (To override it, run 'make JOBS=1' and similar.)
> >  #
> >  ifeq ($(JOBS),)
> > -  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
> > +  JOBS := $(shell (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU' /proc/cpuinfo) 2>/dev/null)
> 
> how about the 'CPU[0-9]' someone asked for?

Ok, will fold that in, are you ok with going with this patch plus this
[0-9] change? I.e. acked?


- Arnaldo

Re: arm/arm64 perf build issue with mainline

[Jiri Olsa]

On Mon, Apr 27, 2015 at 04:00:04PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> > > index c699dc35eef9..bc846b83c295 100644
> > > --- a/tools/perf/Makefile
> > > +++ b/tools/perf/Makefile
> > > @@ -24,7 +24,7 @@ unexport MAKEFLAGS
> > >  # (To override it, run 'make JOBS=1' and similar.)
> > >  #
> > >  ifeq ($(JOBS),)
> > > -  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
> > > +  JOBS := $(shell (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU' /proc/cpuinfo) 2>/dev/null)
> > 
> > how about the 'CPU[0-9]' someone asked for?
> 
> Ok, will fold that in, are you ok with going with this patch plus this
> [0-9] change? I.e. acked?

yep

jirka

[tip:perf/core] perf tools: Use getconf to determine number of online CPUs

[tip-bot for Will Deacon]

Commit-ID:  762abdc0c6c013425958cd9f5105f4e32268d434
Gitweb:     http://git.kernel.org/tip/762abdc0c6c013425958cd9f5105f4e32268d434
Author:     Will Deacon <will.deacon@arm.com>
AuthorDate: Thu, 23 Apr 2015 15:00:16 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 4 May 2015 12:43:40 -0300

perf tools: Use getconf to determine number of online CPUs

Parsing /proc/cpuinfo is a fiddly, arch-dependent business and a recent
change to get it working for Sparc broke arm and arm64 platforms.

Use sysconf to determine the number of online CPUs only parsing
/proc/cpuinfo when sysconf is not available.

Signed-off-by: Will Deacon <will.deacon@arm.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <david.ahern@oracle.com>
Cc: Mark Rutland <Mark.Rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/r/20150423140454.GJ1652@arm.com
[ Made it fall back to parsing /proc when getconf not found ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index c699dc3..d31a7bb 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -24,7 +24,7 @@ unexport MAKEFLAGS
 # (To override it, run 'make JOBS=1' and similar.)
 #
 ifeq ($(JOBS),)
-  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
+  JOBS := $(shell (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU[0-9]' /proc/cpuinfo) 2>/dev/null)
   ifeq ($(JOBS),0)
     JOBS := 1
   endif

[tip:perf/urgent] perf tools: Use getconf to determine number of online CPUs

[tip-bot for Will Deacon]

Commit-ID:  466c1eb07f42bd27825af24d86b46d05e5e350b9
Gitweb:     http://git.kernel.org/tip/466c1eb07f42bd27825af24d86b46d05e5e350b9
Author:     Will Deacon <will.deacon@arm.com>
AuthorDate: Thu, 23 Apr 2015 15:00:16 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 12 May 2015 18:11:16 -0300

perf tools: Use getconf to determine number of online CPUs

Parsing /proc/cpuinfo is a fiddly, arch-dependent business and a recent
change to get it working for Sparc broke arm and arm64 platforms.

Use sysconf to determine the number of online CPUs only parsing
/proc/cpuinfo when sysconf is not available.

Signed-off-by: Will Deacon <will.deacon@arm.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <david.ahern@oracle.com>
Cc: Mark Rutland <Mark.Rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/r/20150423140454.GJ1652@arm.com
[ Made it fall back to parsing /proc when getconf not found ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index c699dc3..d31a7bb 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -24,7 +24,7 @@ unexport MAKEFLAGS
 # (To override it, run 'make JOBS=1' and similar.)
 #
 ifeq ($(JOBS),)
-  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
+  JOBS := $(shell (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU[0-9]' /proc/cpuinfo) 2>/dev/null)
   ifeq ($(JOBS),0)
     JOBS := 1
   endif

Re: arm/arm64 perf build issue with mainline

[David Ahern]

On 4/23/15 5:29 AM, Will Deacon wrote:
> Hi all,
>
> Commit 6428c59a97de ("perf tools: Set JOBS based on CPU or processor")
> causes weird behaviour on arm/arm64 platforms because we use the "CPU"
> prefix for things like:
>
> CPU implementer : 0x41
> CPU architecture: 8
> CPU variant     : 0x0
> CPU part        : 0xd03
> CPU revision    : 0
>
> in /proc/cpuinfo. Consequently, a 6 core machine ends up doing:
>
> will@confinement-loaf:~/linux/tools/perf$ make
>    BUILD:   Doing 'make -j36' parallel build
>
> which is a little overwhelming. Any chance we can predicate the extra
> part of the regex on $(ARCH) being sparc?


Frankly, I think the JOBS parameter needs to be removed. It's 
non-standard way of controlling parallelism in the build and it makes 
the assumption that if a system has N processors all of those can be 
used to build perf which is not true if you are building perf as part of 
bigger image builds -- like Yocto for example.

Ingo: As I recall you put this in? Opinions on removing it? Users can 
always add the standard '-j N' for parallelism just like they do for 
kernel builds.

David

Re: arm/arm64 perf build issue with mainline

[Ingo Molnar]

* David Ahern <david.ahern@oracle.com> wrote:

> On 4/23/15 5:29 AM, Will Deacon wrote:
> >Hi all,
> >
> >Commit 6428c59a97de ("perf tools: Set JOBS based on CPU or processor")
> >causes weird behaviour on arm/arm64 platforms because we use the "CPU"
> >prefix for things like:
> >
> >CPU implementer : 0x41
> >CPU architecture: 8
> >CPU variant     : 0x0
> >CPU part        : 0xd03
> >CPU revision    : 0
> >
> >in /proc/cpuinfo. Consequently, a 6 core machine ends up doing:
> >
> >will@confinement-loaf:~/linux/tools/perf$ make
> >   BUILD:   Doing 'make -j36' parallel build
> >
> >which is a little overwhelming. Any chance we can predicate the extra
> >part of the regex on $(ARCH) being sparc?

That regex needs to be fixed or replaced with a more robust 'number of 
CPUs on the system' discovery method.

> Frankly, I think the JOBS parameter needs to be removed. It's 
> non-standard way of controlling parallelism in the build and it 
> makes the assumption that if a system has N processors all of those 
> can be used to build perf which is not true if you are building perf 
> as part of bigger image builds -- like Yocto for example.
> 
> Ingo: As I recall you put this in? Opinions on removing it?

I disagree strongly!

> [...] Users can always add the standard '-j N' for parallelism just 
> like they do for kernel builds.

So for a few oddball cases we remove something that improves usability 
quite visibly? 'make' or 'make install' will do the right thing today 
on 99.9% of the systems, and it should do that in the future as well.

We should not add extra usability barriers that hurts the regular 
case.

This usability concept permeates all of perf: that's why 'perf top' 
will measure all CPUs by default, although there are certainly cases 
where that's not what the user wants.

Thanks,

	Ingo

Re: arm/arm64 perf build issue with mainline

[Will Deacon]

On Mon, May 11, 2015 at 09:21:20AM +0100, Ingo Molnar wrote:
> * David Ahern <david.ahern@oracle.com> wrote:
> > On 4/23/15 5:29 AM, Will Deacon wrote:
> > >Commit 6428c59a97de ("perf tools: Set JOBS based on CPU or processor")
> > >causes weird behaviour on arm/arm64 platforms because we use the "CPU"
> > >prefix for things like:
> > >
> > >CPU implementer : 0x41
> > >CPU architecture: 8
> > >CPU variant     : 0x0
> > >CPU part        : 0xd03
> > >CPU revision    : 0
> > >
> > >in /proc/cpuinfo. Consequently, a 6 core machine ends up doing:
> > >
> > >will@confinement-loaf:~/linux/tools/perf$ make
> > >   BUILD:   Doing 'make -j36' parallel build
> > >
> > >which is a little overwhelming. Any chance we can predicate the extra
> > >part of the regex on $(ARCH) being sparc?
> 
> That regex needs to be fixed or replaced with a more robust 'number of 
> CPUs on the system' discovery method.

That was already proposed here (as part of the fallback from getconf):

  https://lkml.kernel.org/r/20150427190356.GD474@krava.redhat.com

but I'm not sure what happened to the patch.

Will

Re: arm/arm64 perf build issue with mainline

[Ingo Molnar]

* Will Deacon <will.deacon@arm.com> wrote:

> On Mon, May 11, 2015 at 09:21:20AM +0100, Ingo Molnar wrote:
> > * David Ahern <david.ahern@oracle.com> wrote:
> > > On 4/23/15 5:29 AM, Will Deacon wrote:
> > > >Commit 6428c59a97de ("perf tools: Set JOBS based on CPU or processor")
> > > >causes weird behaviour on arm/arm64 platforms because we use the "CPU"
> > > >prefix for things like:
> > > >
> > > >CPU implementer : 0x41
> > > >CPU architecture: 8
> > > >CPU variant     : 0x0
> > > >CPU part        : 0xd03
> > > >CPU revision    : 0
> > > >
> > > >in /proc/cpuinfo. Consequently, a 6 core machine ends up doing:
> > > >
> > > >will@confinement-loaf:~/linux/tools/perf$ make
> > > >   BUILD:   Doing 'make -j36' parallel build
> > > >
> > > >which is a little overwhelming. Any chance we can predicate the extra
> > > >part of the regex on $(ARCH) being sparc?
> > 
> > That regex needs to be fixed or replaced with a more robust 'number of 
> > CPUs on the system' discovery method.
> 
> That was already proposed here (as part of the fallback from getconf):
> 
>   https://lkml.kernel.org/r/20150427190356.GD474@krava.redhat.com
> 
> but I'm not sure what happened to the patch.

Sending out the latest/best version as a reminder for Arnaldo will 
sure help it along.

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: arm/arm64 perf build issue with mainline

[acme]

Em Mon, May 11, 2015 at 02:33:04PM +0200, Ingo Molnar escreveu:
> 
> * Will Deacon <will.deacon@arm.com> wrote:
> 
> > On Mon, May 11, 2015 at 09:21:20AM +0100, Ingo Molnar wrote:
> > > * David Ahern <david.ahern@oracle.com> wrote:
> > > > On 4/23/15 5:29 AM, Will Deacon wrote:
> > > > >Commit 6428c59a97de ("perf tools: Set JOBS based on CPU or processor")
> > > > >causes weird behaviour on arm/arm64 platforms because we use the "CPU"
> > > > >prefix for things like:
> > > > >
> > > > >CPU implementer : 0x41
> > > > >CPU architecture: 8
> > > > >CPU variant     : 0x0
> > > > >CPU part        : 0xd03
> > > > >CPU revision    : 0
> > > > >
> > > > >in /proc/cpuinfo. Consequently, a 6 core machine ends up doing:
> > > > >
> > > > >will@confinement-loaf:~/linux/tools/perf$ make
> > > > >   BUILD:   Doing 'make -j36' parallel build
> > > > >
> > > > >which is a little overwhelming. Any chance we can predicate the extra
> > > > >part of the regex on $(ARCH) being sparc?
> > > 
> > > That regex needs to be fixed or replaced with a more robust 'number of 
> > > CPUs on the system' discovery method.
> > 
> > That was already proposed here (as part of the fallback from getconf):
> > 
> >   https://lkml.kernel.org/r/20150427190356.GD474@krava.redhat.com
> > 
> > but I'm not sure what happened to the patch.
> 
> Sending out the latest/best version as a reminder for Arnaldo will 
> sure help it along.
> 
> Acked-by: Ingo Molnar <mingo@kernel.org>

IIRC it was merged already, lemme check...

- Arnaldo

Re: arm/arm64 perf build issue with mainline

[acme]

Em Mon, May 11, 2015 at 12:58:14PM -0300, acme@redhat.com escreveu:
> Em Mon, May 11, 2015 at 02:33:04PM +0200, Ingo Molnar escreveu:
> > > That was already proposed here (as part of the fallback from getconf):
> > > 
> > >   https://lkml.kernel.org/r/20150427190356.GD474@krava.redhat.com
> > > 
> > > but I'm not sure what happened to the patch.
> > 
> > Sending out the latest/best version as a reminder for Arnaldo will 
> > sure help it along.
> > 
> > Acked-by: Ingo Molnar <mingo@kernel.org>
> 
> IIRC it was merged already, lemme check...

Yes, only for perf/core:


[acme@zoo linux]$ git show 762abdc0c6c013425958cd9f5105f4e32268d434
commit 762abdc0c6c013425958cd9f5105f4e32268d434
Author: Will Deacon <will.deacon@arm.com>
Date:   Thu Apr 23 15:00:16 2015 +0100

    perf tools: Use getconf to determine number of online CPUs
    
    Parsing /proc/cpuinfo is a fiddly, arch-dependent business and a recent
    change to get it working for Sparc broke arm and arm64 platforms.
    
    Use sysconf to determine the number of online CPUs only parsing
    /proc/cpuinfo when sysconf is not available.
    
    Signed-off-by: Will Deacon <will.deacon@arm.com>
    Acked-by: Jiri Olsa <jolsa@kernel.org>
    Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
    Cc: David Ahern <david.ahern@oracle.com>
    Cc: Mark Rutland <Mark.Rutland@arm.com>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Link: http://lkml.kernel.org/r/20150423140454.GJ1652@arm.com
    [ Made it fall back to parsing /proc when getconf not found ]
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index c699dc35eef9..d31a7bbd7cee 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -24,7 +24,7 @@ unexport MAKEFLAGS
 # (To override it, run 'make JOBS=1' and similar.)
 #
 ifeq ($(JOBS),)
-  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
+  JOBS := $(shell (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU[0-9]' /proc/cpuinfo) 2>/dev/null)
   ifeq ($(JOBS),0)
     JOBS := 1
   endif
[acme@zoo linux]$ 

Re: arm/arm64 perf build issue with mainline

[Will Deacon]

On Mon, May 11, 2015 at 04:59:58PM +0100, acme@redhat.com wrote:
> Em Mon, May 11, 2015 at 12:58:14PM -0300, acme@redhat.com escreveu:
> > Em Mon, May 11, 2015 at 02:33:04PM +0200, Ingo Molnar escreveu:
> > > > That was already proposed here (as part of the fallback from getconf):
> > > > 
> > > >   https://lkml.kernel.org/r/20150427190356.GD474@krava.redhat.com
> > > > 
> > > > but I'm not sure what happened to the patch.
> > > 
> > > Sending out the latest/best version as a reminder for Arnaldo will 
> > > sure help it along.
> > > 
> > > Acked-by: Ingo Molnar <mingo@kernel.org>
> > 
> > IIRC it was merged already, lemme check...
> 
> Yes, only for perf/core:

Ah, that explains why I couldn't see it in my tree. Could we get it in
for 4.1 please, as this is a regression for arm/arm64 (where the
auto-detection used to work fine)?

Cheers,

Will

> 
> [acme@zoo linux]$ git show 762abdc0c6c013425958cd9f5105f4e32268d434
> commit 762abdc0c6c013425958cd9f5105f4e32268d434
> Author: Will Deacon <will.deacon@arm.com>
> Date:   Thu Apr 23 15:00:16 2015 +0100
> 
>     perf tools: Use getconf to determine number of online CPUs
>     
>     Parsing /proc/cpuinfo is a fiddly, arch-dependent business and a recent
>     change to get it working for Sparc broke arm and arm64 platforms.
>     
>     Use sysconf to determine the number of online CPUs only parsing
>     /proc/cpuinfo when sysconf is not available.
>     
>     Signed-off-by: Will Deacon <will.deacon@arm.com>
>     Acked-by: Jiri Olsa <jolsa@kernel.org>
>     Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>     Cc: David Ahern <david.ahern@oracle.com>
>     Cc: Mark Rutland <Mark.Rutland@arm.com>
>     Cc: Namhyung Kim <namhyung@kernel.org>
>     Link: http://lkml.kernel.org/r/20150423140454.GJ1652@arm.com
>     [ Made it fall back to parsing /proc when getconf not found ]
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index c699dc35eef9..d31a7bbd7cee 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -24,7 +24,7 @@ unexport MAKEFLAGS
>  # (To override it, run 'make JOBS=1' and similar.)
>  #
>  ifeq ($(JOBS),)
> -  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
> +  JOBS := $(shell (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU[0-9]' /proc/cpuinfo) 2>/dev/null)
>    ifeq ($(JOBS),0)
>      JOBS := 1
>    endif
> [acme@zoo linux]$ 
> 

Re: arm/arm64 perf build issue with mainline

[Arnaldo Carvalho de Melo]

Em Mon, May 11, 2015 at 06:29:28PM +0100, Will Deacon escreveu:
> On Mon, May 11, 2015 at 04:59:58PM +0100, acme@redhat.com wrote:
> > Em Mon, May 11, 2015 at 12:58:14PM -0300, acme@redhat.com escreveu:
> > > Em Mon, May 11, 2015 at 02:33:04PM +0200, Ingo Molnar escreveu:
> > > > > That was already proposed here (as part of the fallback from getconf):
> > > > > 
> > > > >   https://lkml.kernel.org/r/20150427190356.GD474@krava.redhat.com
> > > > > 
> > > > > but I'm not sure what happened to the patch.
> > > > 
> > > > Sending out the latest/best version as a reminder for Arnaldo will 
> > > > sure help it along.
> > > > 
> > > > Acked-by: Ingo Molnar <mingo@kernel.org>
> > > 
> > > IIRC it was merged already, lemme check...
> > 
> > Yes, only for perf/core:
> 
> Ah, that explains why I couldn't see it in my tree. Could we get it in
> for 4.1 please, as this is a regression for arm/arm64 (where the
> auto-detection used to work fine)?

Ok, I'll cherry-pick it into a perf/urgent branch, will see if there are
more cherries and then ask Ingo to pull it.

- Arnaldo
 
> Cheers,
> 
> Will
> 
> > 
> > [acme@zoo linux]$ git show 762abdc0c6c013425958cd9f5105f4e32268d434
> > commit 762abdc0c6c013425958cd9f5105f4e32268d434
> > Author: Will Deacon <will.deacon@arm.com>
> > Date:   Thu Apr 23 15:00:16 2015 +0100
> > 
> >     perf tools: Use getconf to determine number of online CPUs
> >     
> >     Parsing /proc/cpuinfo is a fiddly, arch-dependent business and a recent
> >     change to get it working for Sparc broke arm and arm64 platforms.
> >     
> >     Use sysconf to determine the number of online CPUs only parsing
> >     /proc/cpuinfo when sysconf is not available.
> >     
> >     Signed-off-by: Will Deacon <will.deacon@arm.com>
> >     Acked-by: Jiri Olsa <jolsa@kernel.org>
> >     Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> >     Cc: David Ahern <david.ahern@oracle.com>
> >     Cc: Mark Rutland <Mark.Rutland@arm.com>
> >     Cc: Namhyung Kim <namhyung@kernel.org>
> >     Link: http://lkml.kernel.org/r/20150423140454.GJ1652@arm.com
> >     [ Made it fall back to parsing /proc when getconf not found ]
> >     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> > index c699dc35eef9..d31a7bbd7cee 100644
> > --- a/tools/perf/Makefile
> > +++ b/tools/perf/Makefile
> > @@ -24,7 +24,7 @@ unexport MAKEFLAGS
> >  # (To override it, run 'make JOBS=1' and similar.)
> >  #
> >  ifeq ($(JOBS),)
> > -  JOBS := $(shell egrep -c '^processor|^CPU' /proc/cpuinfo 2>/dev/null)
> > +  JOBS := $(shell (getconf _NPROCESSORS_ONLN || egrep -c '^processor|^CPU[0-9]' /proc/cpuinfo) 2>/dev/null)
> >    ifeq ($(JOBS),0)
> >      JOBS := 1
> >    endif
> > [acme@zoo linux]$ 
> >