threads / discuss / 58838

chainlink.pl /proc/cpuinfo regexp fails on s390x

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

## tl;dr

10 messages between Nov 22, 2022 and Nov 23, 2022.

replies: 9people: 3as markdown or json

Andreas Hasenack· Nov 22, 2022, 17:37 UTC · lore
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
Eric Sunshine· Nov 22, 2022, 17:57 UTC · re: Andreas Hasenack · lore

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

On Tue, Nov 22, 2022 at 12:45 PM Andreas Hasenack <andreas@canonical.com> wrote:
Show 19 quoted lines
> 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.

Show 12 quoted lines
> 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

Andreas Hasenack· Nov 22, 2022, 18:04 UTC · re: Eric Sunshine · lore

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

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!
Eric Sunshine· Nov 22, 2022, 18:13 UTC · re: Andreas Hasenack · lore

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

On Tue, Nov 22, 2022 at 1:04 PM Andreas Hasenack <andreas@canonical.com> wrote:
Show 6 quoted lines
> 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/
Andreas Hasenack· Nov 22, 2022, 18:42 UTC · re: Eric Sunshine · lore

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

Hi,
On Tue, Nov 22, 2022 at 3:14 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 17 quoted lines
>
> 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!
Eric Sunshine· Nov 22, 2022, 19:39 UTC · re: Andreas Hasenack · lore

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

On Tue, Nov 22, 2022 at 1:42 PM Andreas Hasenack <andreas@canonical.com> wrote:
Show 11 quoted lines
> 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.
Andreas Schwab· Nov 22, 2022, 23:30 UTC · re: Andreas Hasenack · lore

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

On Nov 22 2022, Andreas Hasenack wrote:
Show 9 quoted lines
> 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."
Eric Sunshine· Nov 22, 2022, 23:50 UTC · re: Andreas Schwab · lore

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

On Tue, Nov 22, 2022 at 6:37 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
Show 7 quoted lines
> 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/
Andreas Schwab· Nov 23, 2022, 09:27 UTC · re: Eric Sunshine · lore

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

On Nov 22 2022, Eric Sunshine wrote:
Show 16 quoted lines
> 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."
Eric Sunshine· Nov 23, 2022, 19:10 UTC · re: Andreas Schwab · lore

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

On Wed, Nov 23, 2022 at 4:27 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
Show 19 quoted lines
> 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.

← back to recent threads