[PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC

[PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC

[John Paul Adrian Glaubitz]

On SPARC systems running Linux, individual processors are denoted with
"CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:" so that
the current regexp in ncores() returns 0. Extend the regexp to match
lines with "CPUnn:" as well to properly detect the number of available
cores on these systems.

Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
---
 t/chainlint.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/chainlint.pl b/t/chainlint.pl
index 556ee91a15..63cac942ac 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -718,7 +718,7 @@ sub ncores {
 	# Windows
 	return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
 	# Linux / MSYS2 / Cygwin / WSL
-	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
+	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:||^CPU[\d]*:/, <>)); } if -r '/proc/cpuinfo';
 	# macOS & BSD
 	return qx/sysctl -n hw.ncpu/ if $^O =~ /(?:^darwin$|bsd)/;
 	return 1;
-- 
2.39.2

Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC

[Junio C Hamano]

John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> writes:

> On SPARC systems running Linux, individual processors are denoted with
> "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:" so that
> the current regexp in ncores() returns 0. Extend the regexp to match
> lines with "CPUnn:" as well to properly detect the number of available
> cores on these systems.
>
> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> ---
>  t/chainlint.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/chainlint.pl b/t/chainlint.pl
> index 556ee91a15..63cac942ac 100755
> --- a/t/chainlint.pl
> +++ b/t/chainlint.pl
> @@ -718,7 +718,7 @@ sub ncores {
>  	# Windows
>  	return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
>  	# Linux / MSYS2 / Cygwin / WSL
> -	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
> +	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:||^CPU[\d]*:/, <>)); } if -r '/proc/cpuinfo';

Is the doubled || intended?  Doesn't it introduce an empty pattern
that slurps every single line of /proc/cpuinfo?

I was wondering if we want to first add the "reasonable fallback"
Eric mentioned ealier, and then build on top, whose result may look
like the attached.  You can enable the STDERR thing with your double
"||" added back and see what "cd t && perl chainlint.pl" produces.

Thanks.

diff --git i/t/chainlint.pl w/t/chainlint.pl
index 556ee91a15..775f06281b 100755
--- i/t/chainlint.pl
+++ w/t/chainlint.pl
@@ -718,7 +718,13 @@ sub ncores {
 	# Windows
 	return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
 	# Linux / MSYS2 / Cygwin / WSL
-	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
+	do {
+		local @ARGV='/proc/cpuinfo';
+		my @num = grep(/^processor[\s\d]*:|^CPU[\d]*:/, <>);
+# print STDERR "FOUND <@num>\n";
+		return 1 if (!@num);
+		return scalar(@num);
+	} if -r '/proc/cpuinfo';
 	# macOS & BSD
 	return qx/sysctl -n hw.ncpu/ if $^O =~ /(?:^darwin$|bsd)/;
 	return 1;

Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC

[John Paul Adrian Glaubitz]

Hi Junio,

On Mon, 2024-05-20 at 09:16 -0700, Junio C Hamano wrote:
> John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> writes:
> 
> > On SPARC systems running Linux, individual processors are denoted with
> > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:" so that
> > the current regexp in ncores() returns 0. Extend the regexp to match
> > lines with "CPUnn:" as well to properly detect the number of available
> > cores on these systems.
> > 
> > Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> > ---
> >  t/chainlint.pl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/t/chainlint.pl b/t/chainlint.pl
> > index 556ee91a15..63cac942ac 100755
> > --- a/t/chainlint.pl
> > +++ b/t/chainlint.pl
> > @@ -718,7 +718,7 @@ sub ncores {
> >  	# Windows
> >  	return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
> >  	# Linux / MSYS2 / Cygwin / WSL
> > -	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
> > +	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:||^CPU[\d]*:/, <>)); } if -r '/proc/cpuinfo';
> 
> Is the doubled || intended?  Doesn't it introduce an empty pattern
> that slurps every single line of /proc/cpuinfo?

I'm not a Perl expert by any means, so I wasn't sure what the correct logical OR
operator would be. If it turns out to be wrong, let's fix that.

> I was wondering if we want to first add the "reasonable fallback"
> Eric mentioned ealier, and then build on top, whose result may look
> like the attached.  You can enable the STDERR thing with your double
> "||" added back and see what "cd t && perl chainlint.pl" produces.
> 
> Thanks.
> 
> diff --git i/t/chainlint.pl w/t/chainlint.pl
> index 556ee91a15..775f06281b 100755
> --- i/t/chainlint.pl
> +++ w/t/chainlint.pl
> @@ -718,7 +718,13 @@ sub ncores {
>  	# Windows
>  	return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
>  	# Linux / MSYS2 / Cygwin / WSL
> -	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
> +	do {
> +		local @ARGV='/proc/cpuinfo';
> +		my @num = grep(/^processor[\s\d]*:|^CPU[\d]*:/, <>);
> +# print STDERR "FOUND <@num>\n";
> +		return 1 if (!@num);
> +		return scalar(@num);
> +	} if -r '/proc/cpuinfo';
>  	# macOS & BSD
>  	return qx/sysctl -n hw.ncpu/ if $^O =~ /(?:^darwin$|bsd)/;
>  	return 1;

This seems to work fine for me as well. If you post it as a patch, I'm more than
happy to give it a Tested-By.

Btw, it would be great if this could be extended to support the output for the
Alpha architecture as well since the testsuite fails the same way [1]. The output
for /proc/cpuinfo looks like this [2]:

(alpha-chroot)root@p100:/# cat /proc/cpuinfo
cpu                     : Alpha
cpu model               : ev67
cpu variation           : 0
cpu revision            : 0
cpu serial number       : JA00000000
system type             : QEMU
system variation        : QEMU_v8.0.92
system revision         : 0
system serial number    : AY00000000
cycle frequency [Hz]    : 250000000
timer frequency [Hz]    : 250.00
page size [bytes]       : 8192
phys. address bits      : 44
max. addr. space #      : 255
BogoMIPS                : 2500.00
platform string         : AlphaServer QEMU user-mode VM
cpus detected           : 8
cpus active             : 4
cpu active mask         : 0000000000000095
L1 Icache               : n/a
L1 Dcache               : n/a
L2 cache                : n/a
L3 cache                : n/a

Thanks so much for helping with the fix!

Adrian

> [1] https://buildd.debian.org/status/fetch.php?pkg=git&arch=alpha&ver=1%3A2.45.1-1&stamp=1716194983&raw=0
> [2] https://lore.kernel.org/all/20230901204251.137307-4-richard.henderson@linaro.org/

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC

[Eric Sunshine]

On Mon, May 20, 2024 at 12:48 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> Btw, it would be great if this could be extended to support the output for the
> Alpha architecture as well since the testsuite fails the same way [1]. The output
> for /proc/cpuinfo looks like this [2]:
>
> (alpha-chroot)root@p100:/# cat /proc/cpuinfo
> cpus detected           : 8
> cpus active             : 4

What would be the correct answer for this case? 4 or 8?

Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC

[Eric Sunshine]

On Mon, May 20, 2024 at 12:16 PM Junio C Hamano <gitster@pobox.com> wrote:
> John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> writes:
> > On SPARC systems running Linux, individual processors are denoted with
> > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:" so that
> > the current regexp in ncores() returns 0. Extend the regexp to match
> > lines with "CPUnn:" as well to properly detect the number of available
> > cores on these systems.
> >
> > Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> > ---
> > diff --git a/t/chainlint.pl b/t/chainlint.pl
> > @@ -718,7 +718,7 @@ sub ncores {
> >       # Windows
> >       return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
> >       # Linux / MSYS2 / Cygwin / WSL
> > -     do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
> > +     do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:||^CPU[\d]*:/, <>)); } if -r '/proc/cpuinfo';
>
> Is the doubled || intended?  Doesn't it introduce an empty pattern
> that slurps every single line of /proc/cpuinfo?

I was also wondering about `||`; it looks like a typo.

More importantly, though, we can simplify the pattern to:

    /^(processor|CPU)[\s\d]*:/

which is much easier to comprehend and gives correct results from the
SPARC /proc/cpuinfo output.

> I was wondering if we want to first add the "reasonable fallback"
> Eric mentioned ealier, and then build on top, whose result may look
> like the attached.

I'm fine with a well-focused patch which just fixes the reported
problem; the "reasonable fallback" change can be layered atop at any
time. But, of course, if Adrian wants to tackle it as part of this
series, I would not object.

> diff --git i/t/chainlint.pl w/t/chainlint.pl
> @@ -718,7 +718,13 @@ sub ncores {
>         # Windows
>         return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
>         # Linux / MSYS2 / Cygwin / WSL
> -       do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
> +       do {
> +               local @ARGV='/proc/cpuinfo';
> +               my @num = grep(/^processor[\s\d]*:|^CPU[\d]*:/, <>);
> +# print STDERR "FOUND <@num>\n";
> +               return 1 if (!@num);
> +               return scalar(@num);
> +       } if -r '/proc/cpuinfo';
>         # macOS & BSD
>         return qx/sysctl -n hw.ncpu/ if $^O =~ /(?:^darwin$|bsd)/;
>         return 1;

I had a more all-inclusive change in mind. These number-of-cpu checks
are in order from least to most costly but they are not necessarily
mutually exclusive. As such, my thinking was that the logic would fall
through to the next check if the preceding check produced zero or
nonsense.

Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC

[Eric Sunshine]

On Mon, May 20, 2024 at 12:50 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, May 20, 2024 at 12:16 PM Junio C Hamano <gitster@pobox.com> wrote:
> >         # Windows
> >         return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
> >         # Linux / MSYS2 / Cygwin / WSL
> > -       do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
> > +       do {
> > +               local @ARGV='/proc/cpuinfo';
> > +               my @num = grep(/^processor[\s\d]*:|^CPU[\d]*:/, <>);
> > +# print STDERR "FOUND <@num>\n";
> > +               return 1 if (!@num);
> > +               return scalar(@num);
> > +       } if -r '/proc/cpuinfo';
> >         # macOS & BSD
> >         return qx/sysctl -n hw.ncpu/ if $^O =~ /(?:^darwin$|bsd)/;
> >         return 1;
>
> I had a more all-inclusive change in mind. These number-of-cpu checks
> are in order from least to most costly but they are not necessarily
> mutually exclusive. As such, my thinking was that the logic would fall
> through to the next check if the preceding check produced zero or
> nonsense.

Hmph. Looking at this more closely, I guess I did make these more
mutually-exclusive than I had thought, so falling through to the next
check probably doesn't buy us much. In any case, for robustness, I
still think that each check needs to have a sensible fallback. An
alternative would be for the caller of ncores() to fallback to 1 if
ncores() returns 0 (or nonsense).

Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> I was wondering if we want to first add the "reasonable fallback"
>> Eric mentioned ealier, and then build on top, whose result may look
>> like the attached.
>
> I'm fine with a well-focused patch which just fixes the reported
> problem; the "reasonable fallback" change can be layered atop at any
> time.

Yeah, I never suggested to do these in a single patch.  Since I
would think that it is easier to do and review a patch that cleans
up the code and adds a reasonable fallback before adding new support
for sparc or alpha (after all, such a clean-up is also for longer
term maintainability---by definition, it must be easier to add new
support to the result of a clean-up than the original, or it is not
a clean-up), I suggested to first add such a change.  What you saw
was how the result of "then build on top" would have looked like.

> I had a more all-inclusive change in mind. These number-of-cpu checks
> are in order from least to most costly but they are not necessarily
> mutually exclusive. As such, my thinking was that the logic would fall
> through to the next check if the preceding check produced zero or
> nonsense.

OK.  All the more reason to clean-up first, then?  If we pile more
on top of the current structure, it would make the later clean-up
more cumbersome, wouldn't it?

Thanks.

[PATCH 0/3] improve chainlint.pl CPU count computation

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

This series replaces a patch[1] sent by John Paul Adrian Glaubitz to fix
chainlint.pl CPU count computation on Linux SPARC.

Unlike its predecessor, this series also fixes an underlying problem in
which ncores() could return 0 which would result in chainlint.pl not
processing any of its input test scripts. Patch [3/3] also fixes CPU
count detection on Alpha[2].

Patch [2/3] of this series is more or less Adrian's original patch[1] so
it retains his authorship, though I simplified the regular-expression
and tweaked the commit message.

[1]: https://lore.kernel.org/git/20240520111109.99882-1-glaubitz@physik.fu-berlin.de/
[2]: https://lore.kernel.org/git/503a99f3511559722a3eeef15d31027dfe617fa1.camel@physik.fu-berlin.de/

Eric Sunshine (2):
  chainlint.pl: make CPU count computation more robust
  chainlint.pl: latch CPU count directly reported by /proc/cpuinfo

John Paul Adrian Glaubitz (1):
  chainlint.pl: fix incorrect CPU count on Linux SPARC

 t/chainlint.pl | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

-- 
2.45.1

[PATCH 1/3] chainlint.pl: make CPU count computation more robust

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

There have been reports[1,2] of chainlint.pl failing to produce output
when output is expected. In fact, the underlying problem is more severe:
in these cases, it isn't doing any work at all, thus not checking Git
tests for semantic problems. In the reported cases, the problem was
tracked down to ncores() returning 0 for the CPU count, which resulted
in chainlint.pl not performing any work (since it thought it had no
cores on which to process).

In the reported cases, the reason for the failure was that the regular
expression counting the number of processors reported by /proc/cpuinfo
failed to find any matches, hence it counted 0 processors. Although
fixing each case as it is reported allows chaining.pl to work correctly
on that architecture, it does nothing to improve the overall robustness
of the core count computation which may still return 0 on some yet
untested architecture.

Address this shortcoming by ensuring that ncores() returns a sensible
fallback value in all cases.

[1]: https://lore.kernel.org/git/pull.1385.git.git.1669148861635.gitgitgadget@gmail.com/
[2]: https://lore.kernel.org/git/8baa12f8d044265f1ddeabd64209e7ac0d3700ae.camel@physik.fu-berlin.de/

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.pl | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/t/chainlint.pl b/t/chainlint.pl
index 556ee91a15..d9a2691889 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -716,11 +716,22 @@ sub fd_colors {
 
 sub ncores {
 	# Windows
-	return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
+	if (exists($ENV{NUMBER_OF_PROCESSORS})) {
+		my $ncpu = $ENV{NUMBER_OF_PROCESSORS};
+		return $ncpu > 0 ? $ncpu : 1;
+	}
 	# Linux / MSYS2 / Cygwin / WSL
-	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
+	if (open my $fh, '<', '/proc/cpuinfo') {
+		my $cpuinfo = do { local $/; <$fh> };
+		close($fh);
+		my @matches = ($cpuinfo =~ /^processor[\s\d]*:/mg);
+		return @matches ? scalar(@matches) : 1;
+	}
 	# macOS & BSD
-	return qx/sysctl -n hw.ncpu/ if $^O =~ /(?:^darwin$|bsd)/;
+	if ($^O =~ /(?:^darwin$|bsd)/) {
+		my $ncpu = qx/sysctl -n hw.ncpu/;
+		return $ncpu > 0 ? $ncpu : 1;
+	}
 	return 1;
 }
 
-- 
2.45.1

[PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC

[Eric Sunshine]

From: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

On SPARC systems running Linux, individual processors are denoted with
"CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:". As a
result, the regexp in ncores() matches 0 times. Address this shortcoming
by extending the regexp to also match lines with "CPUnn:".

Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
[es: simplified regexp; tweaked commit message]
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/chainlint.pl b/t/chainlint.pl
index d9a2691889..d593cb95e7 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -724,7 +724,7 @@ sub ncores {
 	if (open my $fh, '<', '/proc/cpuinfo') {
 		my $cpuinfo = do { local $/; <$fh> };
 		close($fh);
-		my @matches = ($cpuinfo =~ /^processor[\s\d]*:/mg);
+		my @matches = ($cpuinfo =~ /^(processor|CPU)[\s\d]*:/mg);
 		return @matches ? scalar(@matches) : 1;
 	}
 	# macOS & BSD
-- 
2.45.1

Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC

[Carlo Marcelo Arenas Belón]

On Mon, May 20, 2024 at 03:01:30PM UTC, Eric Sunshine wrote:
> From: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> 
> On SPARC systems running Linux, individual processors are denoted with
> "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:".

not sure if worth a reroll, but the "usual" syntax is "processor  : NN"
the regexp used checks for numbers before the colon to account for the
syntax used on s390x which is the only one with numbers before the colon.

Carlo

Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC

[John Paul Adrian Glaubitz]

Hi Carlo,

On Wed, 2024-05-22 at 01:32 -0700, Carlo Marcelo Arenas Belón wrote:
> On Mon, May 20, 2024 at 03:01:30PM UTC, Eric Sunshine wrote:
> > From: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> > 
> > On SPARC systems running Linux, individual processors are denoted with
> > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:".
> 
> not sure if worth a reroll, but the "usual" syntax is "processor  : NN"
> the regexp used checks for numbers before the colon to account for the
> syntax used on s390x which is the only one with numbers before the colon.

Good catch. I think this can just be fixed by whoever commits the patches
or is that done automatically?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC

[Eric Sunshine]

On Wed, May 22, 2024 at 4:47 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Wed, 2024-05-22 at 01:32 -0700, Carlo Marcelo Arenas Belón wrote:
> > On Mon, May 20, 2024 at 03:01:30PM UTC, Eric Sunshine wrote:
> > > From: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> > >
> > > On SPARC systems running Linux, individual processors are denoted with
> > > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:".
> >
> > not sure if worth a reroll, but the "usual" syntax is "processor  : NN"
> > the regexp used checks for numbers before the colon to account for the
> > syntax used on s390x which is the only one with numbers before the colon.
>
> Good catch. I think this can just be fixed by whoever commits the patches
> or is that done automatically?

Inclusion of the word "usual" is such a minor flaw in the commit
message that I doubt it warrants a reroll and the associated cost on
reviewers and on the maintainer (Junio), especially since it does not
negatively impact the intent conveyed by the commit messages nor the
correctness of the actual patch.

As such, I'm not worried about it. Whether Junio reads this and wants
to correct it in his tree is up to him, of course.

Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> > > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:".
>> >
>> > not sure if worth a reroll, but the "usual" syntax is "processor  : NN"
> ...
> Inclusion of the word "usual" is such a minor flaw in the commit
> message that I doubt it warrants a reroll and the associated cost on
> reviewers and on the maintainer (Junio), especially since it does not
> negatively impact the intent conveyed by the commit messages nor the
> correctness of the actual patch.
>
> As such, I'm not worried about it. Whether Junio reads this and wants
> to correct it in his tree is up to him, of course.

I think "usual" is not what was pointed out. The order between the
colon and NN is.

Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC

[Eric Sunshine]

On Wed, May 22, 2024 at 3:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> >> > > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:".
> >> >
> >> > not sure if worth a reroll, but the "usual" syntax is "processor  : NN"
> > ...
> > Inclusion of the word "usual" is such a minor flaw in the commit
> > message that I doubt it warrants a reroll and the associated cost on
> > reviewers and on the maintainer (Junio), especially since it does not
> > negatively impact the intent conveyed by the commit messages nor the
> > correctness of the actual patch.
> >
> > As such, I'm not worried about it. Whether Junio reads this and wants
> > to correct it in his tree is up to him, of course.
>
> I think "usual" is not what was pointed out. The order between the
> colon and NN is.

Yes, I understood that, but it is the word "usual" which makes the
text "processor NN:" questionable since "processor NN:" is not
typical. Without the word "usual", stating "processor NN:" is not
especially problematic since the existing regex (which is being
changed by this patch) _does_ match "processor NN:" (among others such
as "processor:").

If we want to be more accurate, better wording might be:

    On SPARC systems running Linux, individual processors are denoted
    with "CPUnn:" in /proc/cpuinfo, however, the regexp in ncores()
    matches only "processor:" or "processor NN:". As a result, no
    processors are found on SPARC. Address this shortcoming by
    extending the regexp to also match lines with "CPUnn:".

but I doubt it is worth a reroll.

Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC

[John Paul Adrian Glaubitz]

Hi,

On Wed, 2024-05-22 at 15:11 -0400, Eric Sunshine wrote:
> On Wed, May 22, 2024 at 3:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > > > > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:".
> > > > > 
> > > > > not sure if worth a reroll, but the "usual" syntax is "processor  : NN"
> > > ...
> > > Inclusion of the word "usual" is such a minor flaw in the commit
> > > message that I doubt it warrants a reroll and the associated cost on
> > > reviewers and on the maintainer (Junio), especially since it does not
> > > negatively impact the intent conveyed by the commit messages nor the
> > > correctness of the actual patch.
> > > 
> > > As such, I'm not worried about it. Whether Junio reads this and wants
> > > to correct it in his tree is up to him, of course.
> > 
> > I think "usual" is not what was pointed out. The order between the
> > colon and NN is.
> 
> Yes, I understood that, but it is the word "usual" which makes the
> text "processor NN:" questionable since "processor NN:" is not
> typical. Without the word "usual", stating "processor NN:" is not
> especially problematic since the existing regex (which is being
> changed by this patch) _does_ match "processor NN:" (among others such
> as "processor:").
> 
> If we want to be more accurate, better wording might be:
> 
>     On SPARC systems running Linux, individual processors are denoted
>     with "CPUnn:" in /proc/cpuinfo, however, the regexp in ncores()
>     matches only "processor:" or "processor NN:". As a result, no
>     processors are found on SPARC. Address this shortcoming by
>     extending the regexp to also match lines with "CPUnn:".
> 
> but I doubt it is worth a reroll.

So, could we get this series merged now or is there anything missing?

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC

[Eric Sunshine]

On Mon, May 27, 2024 at 3:49 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> So, could we get this series merged now or is there anything missing?

This series has already migrated from the "seen" branch to the "next"
branch in Junio's tree, and according to his latest "What's Cooking"
report[*], he will be merging it to his "master" branch soon, after
which it should be incorporated into an actual release.

[*]: topic "es/chainlint-ncores-fix " in
https://lore.kernel.org/git/xmqq8qzyifnx.fsf@gitster.g/

[PATCH 3/3] chainlint.pl: latch CPU count directly reported by /proc/cpuinfo

[Eric Sunshine]

From: Eric Sunshine <sunshine@sunshineco.com>

On Linux, ncores() computes the number of CPUs by counting the
"processor" or "CPU" lines emitted by /proc/cpuinfo. However, on some
platforms, /proc/cpuinfo does not enumerate the CPUs at all, but
instead merely mentions the total number of CPUs. In such cases, pluck
the CPU count directly from the /proc/cpuinfo line which reports the
number of active CPUs. (In particular, check for "cpus active: NN" and
"ncpus active: NN" since both variants have been seen in the
wild[1,2].)

[1]: https://lore.kernel.org/git/503a99f3511559722a3eeef15d31027dfe617fa1.camel@physik.fu-berlin.de/
[2]: https://lore.kernel.org/git/7acbd5c6c68bd7ba020e2d1cc457a8954fd6edf4.camel@physik.fu-berlin.de/

Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.pl | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/chainlint.pl b/t/chainlint.pl
index d593cb95e7..1bbd985b78 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -724,6 +724,9 @@ sub ncores {
 	if (open my $fh, '<', '/proc/cpuinfo') {
 		my $cpuinfo = do { local $/; <$fh> };
 		close($fh);
+		if ($cpuinfo =~ /^n?cpus active\s*:\s*(\d+)/m) {
+			return $1 if $1 > 0;
+		}
 		my @matches = ($cpuinfo =~ /^(processor|CPU)[\s\d]*:/mg);
 		return @matches ? scalar(@matches) : 1;
 	}
-- 
2.45.1

Re: [PATCH 0/3] improve chainlint.pl CPU count computation

[John Paul Adrian Glaubitz]

On Mon, 2024-05-20 at 15:01 -0400, Eric Sunshine wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> This series replaces a patch[1] sent by John Paul Adrian Glaubitz to fix
> chainlint.pl CPU count computation on Linux SPARC.
> 
> Unlike its predecessor, this series also fixes an underlying problem in
> which ncores() could return 0 which would result in chainlint.pl not
> processing any of its input test scripts. Patch [3/3] also fixes CPU
> count detection on Alpha[2].
> 
> Patch [2/3] of this series is more or less Adrian's original patch[1] so
> it retains his authorship, though I simplified the regular-expression
> and tweaked the commit message.
> 
> [1]: https://lore.kernel.org/git/20240520111109.99882-1-glaubitz@physik.fu-berlin.de/
> [2]: https://lore.kernel.org/git/503a99f3511559722a3eeef15d31027dfe617fa1.camel@physik.fu-berlin.de/
> 
> Eric Sunshine (2):
>   chainlint.pl: make CPU count computation more robust
>   chainlint.pl: latch CPU count directly reported by /proc/cpuinfo
> 
> John Paul Adrian Glaubitz (1):
>   chainlint.pl: fix incorrect CPU count on Linux SPARC
> 
>  t/chainlint.pl | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 

Works as expected on my Linux SPARC machine running Debian unstable.

Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH 0/3] improve chainlint.pl CPU count computation

[Eric Sunshine]

On Mon, May 20, 2024 at 3:17 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Mon, 2024-05-20 at 15:01 -0400, Eric Sunshine wrote:
> > From: Eric Sunshine <sunshine@sunshineco.com>
> > This series replaces a patch[1] sent by John Paul Adrian Glaubitz to fix
> > chainlint.pl CPU count computation on Linux SPARC.
> >
> > Unlike its predecessor, this series also fixes an underlying problem in
> > which ncores() could return 0 which would result in chainlint.pl not
> > processing any of its input test scripts. Patch [3/3] also fixes CPU
> > count detection on Alpha[2].
>
> Works as expected on my Linux SPARC machine running Debian unstable.
>
> Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Thanks for testing. Were you able to check whether it fixes CPU count
detection on Alpha, as well?

Re: [PATCH 0/3] improve chainlint.pl CPU count computation

[John Paul Adrian Glaubitz]

On Mon, 2024-05-20 at 15:19 -0400, Eric Sunshine wrote:
> On Mon, May 20, 2024 at 3:17 PM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
> > On Mon, 2024-05-20 at 15:01 -0400, Eric Sunshine wrote:
> > > From: Eric Sunshine <sunshine@sunshineco.com>
> > > This series replaces a patch[1] sent by John Paul Adrian Glaubitz to fix
> > > chainlint.pl CPU count computation on Linux SPARC.
> > > 
> > > Unlike its predecessor, this series also fixes an underlying problem in
> > > which ncores() could return 0 which would result in chainlint.pl not
> > > processing any of its input test scripts. Patch [3/3] also fixes CPU
> > > count detection on Alpha[2].
> > 
> > Works as expected on my Linux SPARC machine running Debian unstable.
> > 
> > Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> 
> Thanks for testing. Were you able to check whether it fixes CPU count
> detection on Alpha, as well?

I can test on Alpha, but that will take a little longer as I don't have
my setup ready. Will try to report back by tomorrow.

Let me CC Michael Cree and Matt Turner who both own fast Alpha machines
and might report back faster.

@Michael, Matt: Could you test this patch series against the current git
                development tree? It should fix the testsuite on Alpha.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH 0/3] improve chainlint.pl CPU count computation

[John Paul Adrian Glaubitz]

Hi Eric,

On Mon, 2024-05-20 at 21:23 +0200, John Paul Adrian Glaubitz wrote:
> On Mon, 2024-05-20 at 15:19 -0400, Eric Sunshine wrote:
> > Thanks for testing. Were you able to check whether it fixes CPU count
> > detection on Alpha, as well?
> 
> I can test on Alpha, but that will take a little longer as I don't have
> my setup ready. Will try to report back by tomorrow.

I have tested it now on single-core Alpha and it works as expected, so I
think it's safe to land the patches.

I currently cannot test on SMP as I need to build a custom kernel for
that first which disables a problematic kernel option.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH 0/3] improve chainlint.pl CPU count computation

[Eric Sunshine]

On Tue, May 21, 2024 at 10:28 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Mon, 2024-05-20 at 21:23 +0200, John Paul Adrian Glaubitz wrote:
> > On Mon, 2024-05-20 at 15:19 -0400, Eric Sunshine wrote:
> > > Thanks for testing. Were you able to check whether it fixes CPU count
> > > detection on Alpha, as well?
> >
> > I can test on Alpha, but that will take a little longer as I don't have
> > my setup ready. Will try to report back by tomorrow.
>
> I have tested it now on single-core Alpha and it works as expected, so I
> think it's safe to land the patches.

Thank you for testing.