# [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC 23 messages from 2024-05-20 to 2024-05-27. Participants: John Paul Adrian Glaubitz, Junio C Hamano, Eric Sunshine, Carlo Marcelo Arenas Belón. Thread: https://gitlist.dev/t/61517 ## John Paul Adrian Glaubitz, 2024-05-20 11:11 Subject: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC Message-ID: <20240520111109.99882-1-glaubitz@physik.fu-berlin.de> URL: https://gitlist.dev/e/20240520111109.99882-1-glaubitz%40physik.fu-berlin.de ``` 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 --- 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 ``` ## Junio C Hamano, 2024-05-20 16:16 Subject: Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC Message-ID: URL: https://gitlist.dev/e/xmqqseyclaq8.fsf%40gitster.g In-Reply-To: <20240520111109.99882-1-glaubitz@physik.fu-berlin.de> ``` John Paul Adrian Glaubitz 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 > --- > 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; ``` ## John Paul Adrian Glaubitz, 2024-05-20 16:48 Subject: Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC Message-ID: <503a99f3511559722a3eeef15d31027dfe617fa1.camel@physik.fu-berlin.de> URL: https://gitlist.dev/e/503a99f3511559722a3eeef15d31027dfe617fa1.camel%40physik.fu-berlin.de In-Reply-To: ``` Hi Junio, On Mon, 2024-05-20 at 09:16 -0700, Junio C Hamano wrote: > John Paul Adrian Glaubitz 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 > > --- > > 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 ``` ## Eric Sunshine, 2024-05-20 16:50 Subject: Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQsc4AUJ7-0v%3DrS8VVK9JG1%2B_iSwa_gWUUigs%3DuwYq6Lw%40mail.gmail.com In-Reply-To: ``` On Mon, May 20, 2024 at 12:16 PM Junio C Hamano wrote: > John Paul Adrian Glaubitz 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 > > --- > > 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. ``` ## Eric Sunshine, 2024-05-20 16:52 Subject: Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQ0eLdFnRjp-aPAzhyNDC5dxfCqKf3i5bqcQ8NviE8LPQ%40mail.gmail.com In-Reply-To: <503a99f3511559722a3eeef15d31027dfe617fa1.camel@physik.fu-berlin.de> ``` On Mon, May 20, 2024 at 12:48 PM John Paul Adrian Glaubitz 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? ``` ## Eric Sunshine, 2024-05-20 17:07 Subject: Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSGjE69N0TpB4ub7vY5fXHpDJ6j4-xet8G1zcgRo4OHHA%40mail.gmail.com In-Reply-To: ``` On Mon, May 20, 2024 at 12:50 PM Eric Sunshine wrote: > On Mon, May 20, 2024 at 12:16 PM Junio C Hamano 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). ``` ## Junio C Hamano, 2024-05-20 17:23 Subject: Re: [PATCH] chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC Message-ID: URL: https://gitlist.dev/e/xmqqbk50jt1s.fsf%40gitster.g In-Reply-To: ``` Eric Sunshine 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. ``` ## Eric Sunshine, 2024-05-20 19:01 Subject: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC Message-ID: <20240520190131.94904-3-ericsunshine@charter.net> URL: https://gitlist.dev/e/20240520190131.94904-3-ericsunshine%40charter.net In-Reply-To: <20240520190131.94904-1-ericsunshine@charter.net> ``` From: John Paul Adrian Glaubitz 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 [es: simplified regexp; tweaked commit message] Signed-off-by: Eric Sunshine --- 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 ``` ## Eric Sunshine, 2024-05-20 19:01 Subject: [PATCH 3/3] chainlint.pl: latch CPU count directly reported by /proc/cpuinfo Message-ID: <20240520190131.94904-4-ericsunshine@charter.net> URL: https://gitlist.dev/e/20240520190131.94904-4-ericsunshine%40charter.net In-Reply-To: <20240520190131.94904-1-ericsunshine@charter.net> ``` From: Eric Sunshine 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 Signed-off-by: Eric Sunshine --- 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 ``` ## Eric Sunshine, 2024-05-20 19:01 Subject: [PATCH 1/3] chainlint.pl: make CPU count computation more robust Message-ID: <20240520190131.94904-2-ericsunshine@charter.net> URL: https://gitlist.dev/e/20240520190131.94904-2-ericsunshine%40charter.net In-Reply-To: <20240520190131.94904-1-ericsunshine@charter.net> ``` From: Eric Sunshine 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 --- 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 ``` ## Eric Sunshine, 2024-05-20 19:01 Subject: [PATCH 0/3] improve chainlint.pl CPU count computation Message-ID: <20240520190131.94904-1-ericsunshine@charter.net> URL: https://gitlist.dev/e/20240520190131.94904-1-ericsunshine%40charter.net In-Reply-To: <20240520111109.99882-1-glaubitz@physik.fu-berlin.de> ``` From: Eric Sunshine 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 ``` ## John Paul Adrian Glaubitz, 2024-05-20 19:17 Subject: Re: [PATCH 0/3] improve chainlint.pl CPU count computation Message-ID: <277726443f533446be4391cf2aa3d487974a7a18.camel@physik.fu-berlin.de> URL: https://gitlist.dev/e/277726443f533446be4391cf2aa3d487974a7a18.camel%40physik.fu-berlin.de In-Reply-To: <20240520190131.94904-1-ericsunshine@charter.net> ``` On Mon, 2024-05-20 at 15:01 -0400, Eric Sunshine wrote: > From: Eric Sunshine > > 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 Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913 ``` ## Eric Sunshine, 2024-05-20 19:19 Subject: Re: [PATCH 0/3] improve chainlint.pl CPU count computation Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQYJL%2B6J9PJX-vrNRXJ4TUCJCQRDQstUnPf4OOwOVCUnw%40mail.gmail.com In-Reply-To: <277726443f533446be4391cf2aa3d487974a7a18.camel@physik.fu-berlin.de> ``` On Mon, May 20, 2024 at 3:17 PM John Paul Adrian Glaubitz wrote: > On Mon, 2024-05-20 at 15:01 -0400, Eric Sunshine wrote: > > From: Eric Sunshine > > 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 Thanks for testing. Were you able to check whether it fixes CPU count detection on Alpha, as well? ``` ## John Paul Adrian Glaubitz, 2024-05-20 19:23 Subject: Re: [PATCH 0/3] improve chainlint.pl CPU count computation Message-ID: URL: https://gitlist.dev/e/b3bef1539c4f81780b5f705e55b12898cb084254.camel%40physik.fu-berlin.de In-Reply-To: ``` 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 > wrote: > > On Mon, 2024-05-20 at 15:01 -0400, Eric Sunshine wrote: > > > From: Eric Sunshine > > > 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 > > 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 ``` ## John Paul Adrian Glaubitz, 2024-05-21 14:28 Subject: Re: [PATCH 0/3] improve chainlint.pl CPU count computation Message-ID: <6861baa7663cc6cb78bb2e0be664c07530d179ab.camel@physik.fu-berlin.de> URL: https://gitlist.dev/e/6861baa7663cc6cb78bb2e0be664c07530d179ab.camel%40physik.fu-berlin.de In-Reply-To: ``` 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 ``` ## Eric Sunshine, 2024-05-21 16:18 Subject: Re: [PATCH 0/3] improve chainlint.pl CPU count computation Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTfHktPySO3rHikhOUwH0WW00PN99HLpNJWs9i2_TN7vg%40mail.gmail.com In-Reply-To: <6861baa7663cc6cb78bb2e0be664c07530d179ab.camel@physik.fu-berlin.de> ``` On Tue, May 21, 2024 at 10:28 AM John Paul Adrian Glaubitz 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. ``` ## Carlo Marcelo Arenas Belón, 2024-05-22 08:32 Subject: Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC Message-ID: URL: https://gitlist.dev/e/m4xqop5u2a4okzr2svg7j57ey5x5fyfqrqitwwzhh4uansuu7v%40uy4tznnci6dy In-Reply-To: <20240520190131.94904-3-ericsunshine@charter.net> ``` On Mon, May 20, 2024 at 03:01:30PM UTC, Eric Sunshine wrote: > From: John Paul Adrian Glaubitz > > 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 ``` ## John Paul Adrian Glaubitz, 2024-05-22 08:47 Subject: Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC Message-ID: URL: https://gitlist.dev/e/d655399efccfa6219cd982035c0408f9f084e344.camel%40physik.fu-berlin.de In-Reply-To: ``` 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 > > > > 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 ``` ## Eric Sunshine, 2024-05-22 09:05 Subject: Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTfqk%2BtbTAVyPj8YP1uHx1D1swL84h-4p6OKBV%3DdDRX_Q%40mail.gmail.com In-Reply-To: ``` On Wed, May 22, 2024 at 4:47 AM John Paul Adrian Glaubitz 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 > > > > > > 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. ``` ## Junio C Hamano, 2024-05-22 19:00 Subject: Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC Message-ID: URL: https://gitlist.dev/e/xmqqh6ep4qoy.fsf%40gitster.g In-Reply-To: ``` Eric Sunshine 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. ``` ## Eric Sunshine, 2024-05-22 19:11 Subject: Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTxMTAf85Gyi5eEYAM4pOTq2UTD1%3DoCVBy1q8aSF3cn6Q%40mail.gmail.com In-Reply-To: ``` On Wed, May 22, 2024 at 3:00 PM Junio C Hamano wrote: > Eric Sunshine 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. ``` ## John Paul Adrian Glaubitz, 2024-05-27 19:48 Subject: Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC Message-ID: <518a5c2f34180ee2e6b674d8dc5a66b8a2d345f7.camel@physik.fu-berlin.de> URL: https://gitlist.dev/e/518a5c2f34180ee2e6b674d8dc5a66b8a2d345f7.camel%40physik.fu-berlin.de In-Reply-To: ``` 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 wrote: > > Eric Sunshine 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 ``` ## Eric Sunshine, 2024-05-27 20:12 Subject: Re: [PATCH 2/3] chainlint.pl: fix incorrect CPU count on Linux SPARC Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRyLWzcs0Z1Zhi5_w8%2BY53nfu44KgnMjETgo0ZK16W25w%40mail.gmail.com In-Reply-To: <518a5c2f34180ee2e6b674d8dc5a66b8a2d345f7.camel@physik.fu-berlin.de> ``` On Mon, May 27, 2024 at 3:49 PM John Paul Adrian Glaubitz 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/ ```