chainlink.pl /proc/cpuinfo regexp fails on s390x

chainlink.pl /proc/cpuinfo regexp fails on s390x

[Andreas Hasenack]

Hi,

(please CC me on replies, as I'm not subscribed)

git commit 29fb2ec384a867ca577335a12f4b45c184e7b642[1], present in
2.38.0 and later, introduced a function that gets the number of cores
from /proc/cpuinfo. It essentially does this on linux:

    do { local @ARGV='/proc/cpuinfo'; return
scalar(grep(/^processor\s*:/, <>)); } if -r '/proc/cpuinfo';

On s390x, the ^processor lines are like this:

processor 0: version = FF, identification = 148F67, machine = 2964

In other arches (I checked amd64, armhf and arm64), they are like this instead:

processor : 0

As a result, that function is returning 0 on s390x, and that value is
used for the number of jobs the script should execute. Since it's
zero, it exits without doing anything, and that breaks the test and
the build[3] on s390x.

This is trivial and I don't think a PR is necessary, but let me know
if you want one.

That regexp could perhaps be:

/^processor\s*\d*\s*:/

or

/^processor[\s\d]*:/

or something else.


1. https://github.com/git/git/commit/29fb2ec384a867ca577335a12f4b45c184e7b642
2. https://github.com/git/git/commit/29fb2ec384a867ca577335a12f4b45c184e7b642#diff-e7042d714d4be11a06d153e6f2daeb3c3a9766b972522baab8ba113b962086cfR574
3. https://launchpadlibrarian.net/635348769/buildlog_ubuntu-lunar-s390x.git_1%3A2.38.1-1ubuntu1_BUILDING.txt.gz

Re: chainlink.pl /proc/cpuinfo regexp fails on s390x

[Eric Sunshine]

On Tue, Nov 22, 2022 at 12:45 PM Andreas Hasenack <andreas@canonical.com> wrote:
> git commit 29fb2ec384a867ca577335a12f4b45c184e7b642[1], present in
> 2.38.0 and later, introduced a function that gets the number of cores
> from /proc/cpuinfo. It essentially does this on linux:
>
>     do { local @ARGV='/proc/cpuinfo'; return
> scalar(grep(/^processor\s*:/, <>)); } if -r '/proc/cpuinfo';
>
> On s390x, the ^processor lines are like this:
>
> processor 0: version = FF, identification = 148F67, machine = 2964
>
> In other arches (I checked amd64, armhf and arm64), they are like this instead:
>
> processor : 0
>
> As a result, that function is returning 0 on s390x, and that value is
> used for the number of jobs the script should execute. Since it's
> zero, it exits without doing anything, and that breaks the test and
> the build[3] on s390x.

Thanks for the well-described explanation. I think this is the same
problem reported in [1], however, that thread hasn't seen sufficient
diagnostic information yet to figure out what was going on. So, your
report is very helpful.

There is discussion about altogether ditching the code in question
(see the discussion starting at about [2], for instance), but in the
meantime, fixing this would be a good idea.

> This is trivial and I don't think a PR is necessary, but let me know
> if you want one.
>
> That regexp could perhaps be:
>
> /^processor\s*\d*\s*:/
>
> or
>
> /^processor[\s\d]*:/
>
> or something else.

We would be happy to take a patch if you're interested in submitting
one. Otherwise, I can submit a patch to fix this case. Let me know
your preference.

[1]: https://lore.kernel.org/git/f1a5f758-d81f-5985-9b5d-2f0dbfaac071@opensuse.org/
[2]: https://lore.kernel.org/git/221122.86cz9fbyln.gmgdl@evledraar.gmail.com/T/#m7626ed18fa0e93a8b0ada9cf6d2d18b08045cb4b

Re: chainlink.pl /proc/cpuinfo regexp fails on s390x

[Andreas Hasenack]

Hi Eric,

On Tue, Nov 22, 2022 at 2:57 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Tue, Nov 22, 2022 at 12:45 PM Andreas Hasenack <andreas@canonical.com> wrote:
> > git commit 29fb2ec384a867ca577335a12f4b45c184e7b642[1], present in

(...)

> We would be happy to take a patch if you're interested in submitting
> one. Otherwise, I can submit a patch to fix this case. Let me know
> your preference.

Can that be a simple PR in https://github.com/git/git/pulls?

Thanks!

Re: chainlink.pl /proc/cpuinfo regexp fails on s390x

[Eric Sunshine]

On Tue, Nov 22, 2022 at 1:04 PM Andreas Hasenack <andreas@canonical.com> wrote:
> On Tue, Nov 22, 2022 at 2:57 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > We would be happy to take a patch if you're interested in submitting
> > one. Otherwise, I can submit a patch to fix this case. Let me know
> > your preference.
>
> Can that be a simple PR in https://github.com/git/git/pulls?

The project doesn't take pull requests directly, but GitGitGadget[1]
will convert a pull request into a mailing list patch. It looks like
https://github.com/git/git/ is one of the repositories with which
GitGitGadget works, so presumably it should work. You could probably
come up with a well-written commit message by paraphrasing your bug
report.

[1]: https://gitgitgadget.github.io/

Re: chainlink.pl /proc/cpuinfo regexp fails on s390x

[Andreas Hasenack]

Hi,

On Tue, Nov 22, 2022 at 3:14 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Tue, Nov 22, 2022 at 1:04 PM Andreas Hasenack <andreas@canonical.com> wrote:
> > On Tue, Nov 22, 2022 at 2:57 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > We would be happy to take a patch if you're interested in submitting
> > > one. Otherwise, I can submit a patch to fix this case. Let me know
> > > your preference.
> >
> > Can that be a simple PR in https://github.com/git/git/pulls?
>
> The project doesn't take pull requests directly, but GitGitGadget[1]
> will convert a pull request into a mailing list patch. It looks like
> https://github.com/git/git/ is one of the repositories with which
> GitGitGadget works, so presumably it should work. You could probably
> come up with a well-written commit message by paraphrasing your bug
> report.
>
> [1]: https://gitgitgadget.github.io/

Ok, let's see how this goes. I opened a PR in
https://github.com/git/git/pull/1385 and if someone could add the
required /allow, I can take the next steps.

Thanks!

Re: chainlink.pl /proc/cpuinfo regexp fails on s390x

[Eric Sunshine]

On Tue, Nov 22, 2022 at 1:42 PM Andreas Hasenack <andreas@canonical.com> wrote:
> On Tue, Nov 22, 2022 at 3:14 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > The project doesn't take pull requests directly, but GitGitGadget[1]
> > will convert a pull request into a mailing list patch. It looks like
> > https://github.com/git/git/ is one of the repositories with which
> > GitGitGadget works, so presumably it should work. You could probably
> > come up with a well-written commit message by paraphrasing your bug
> > report.
>
> Ok, let's see how this goes. I opened a PR in
> https://github.com/git/git/pull/1385 and if someone could add the
> required /allow, I can take the next steps.

It looks like Dscho already /allow'd you, but GitGitGadget is
complaining that you don't have a public email address on file with
GitHub, which GitGitGadget requires so mailing list replies can make
it back to you. Once you correct that, you should be able to /submit.

The patch itself and the commit message look fine.

Re: chainlink.pl /proc/cpuinfo regexp fails on s390x

[Andreas Schwab]

On Nov 22 2022, Andreas Hasenack wrote:

> That regexp could perhaps be:
>
> /^processor\s*\d*\s*:/
>
> or
>
> /^processor[\s\d]*:/
>
> or something else.

Something else.

$ getconf _NPROCESSORS_ONLN

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: chainlink.pl /proc/cpuinfo regexp fails on s390x

[Eric Sunshine]

On Tue, Nov 22, 2022 at 6:37 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Nov 22 2022, Andreas Hasenack wrote:
> > /^processor[\s\d]*:/
> > or something else.
>
> Something else.
>
> $ getconf _NPROCESSORS_ONLN

Yes, that works too and was (I'm pretty sure) considered during
development. The reason /proc/cpu was chosen over `getconf` was that
opening & reading /proc/cpu should be faster since it doesn't involve
spawning a process. If we did use `getconf`, we'd have to be careful
to degrade gracefully if `getconf` isn't available or if the
configuration parameter (i.e. "_NPROCESSORS_ONLN") isn't known on the
platform.

That said, the submitted patch[1] is probably good enough for now,
especially since there's talk about possibly removing this code
entirely.

[1]: https://lore.kernel.org/git/pull.1385.git.git.1669148861635.gitgitgadget@gmail.com/

Re: chainlink.pl /proc/cpuinfo regexp fails on s390x

[Andreas Schwab]

On Nov 22 2022, Eric Sunshine wrote:

> On Tue, Nov 22, 2022 at 6:37 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>> On Nov 22 2022, Andreas Hasenack wrote:
>> > /^processor[\s\d]*:/
>> > or something else.
>>
>> Something else.
>>
>> $ getconf _NPROCESSORS_ONLN
>
> Yes, that works too and was (I'm pretty sure) considered during
> development. The reason /proc/cpu was chosen over `getconf` was that
> opening & reading /proc/cpu should be faster since it doesn't involve
> spawning a process. If we did use `getconf`, we'd have to be careful
> to degrade gracefully if `getconf` isn't available or if the
> configuration parameter (i.e. "_NPROCESSORS_ONLN") isn't known on the
> platform.

getconf is surely more portable than poking in /proc, especially
/proc/cpuinfo is the antipode of portability.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: chainlink.pl /proc/cpuinfo regexp fails on s390x

[Eric Sunshine]

On Wed, Nov 23, 2022 at 4:27 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Nov 22 2022, Eric Sunshine wrote:
> > On Tue, Nov 22, 2022 at 6:37 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> >> On Nov 22 2022, Andreas Hasenack wrote:
> >> > /^processor[\s\d]*:/
> >> > or something else.
> >>
> >> Something else.
> >> $ getconf _NPROCESSORS_ONLN
> >
> > Yes, that works too and was (I'm pretty sure) considered during
> > development. The reason /proc/cpu was chosen over `getconf` was that
> > opening & reading /proc/cpu should be faster since it doesn't involve
> > spawning a process. If we did use `getconf`, we'd have to be careful
> > to degrade gracefully if `getconf` isn't available or if the
> > configuration parameter (i.e. "_NPROCESSORS_ONLN") isn't known on the
> > platform.
>
> getconf is surely more portable than poking in /proc, especially
> /proc/cpuinfo is the antipode of portability.

No doubt, but for the immediate issue, this tightly-focused fix is
more appropriate and less likely to lead to unexpected additional
problems. I'm not arguing against `getconf`, but saying only that
_this_ patch is fine as-is.

It may very well be a good idea to replace the /proc/cpuinfo probe by
`getconf` in the future (or remove it altogether as mentioned
upstream), but that's outside the scope of this patch and the
immediate problem.