Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added missing include for s390x #284

Closed
wants to merge 1 commit into from

Conversation

junghans
Copy link
Collaborator

@junghans junghans commented Aug 2, 2017

@streichler
Copy link
Contributor

@junghans is that really the right fix? This seems like another instance of what is apparently a more general problem with s390x linux distributions not having asm include files in the right place. See https://stackoverflow.com/questions/14795608/asm-errno-h-no-such-file-or-directory for more details. (This link was in my browser history because I hit it on the last s390x-related issue I was digging into.)

The fact that there's an asm->asm-generic indirection in most Linux distributions makes me worry that using asm-generic directly will be less portable.

@junghans
Copy link
Collaborator Author

junghans commented Aug 2, 2017

That is good question for @nealef !

@nealef
Copy link

nealef commented Aug 2, 2017

Frankly, I can't find anything in /usr/include that has an #include <asm-generic/unistd.h> in it. Why it's required is that it needs the __NR_mbind value. For s390x, this is only defined in asm-generic/unistd.h. In x86_64 it is in asm/unistd-32.h and asm/unistd-64.h.

@streichler
Copy link
Contributor

At least on x86_64, syscall.h includes <asm/unistd.h>, which gets steered to an architecture-specific asm directory that sometimes (but not always) goes back to asm-generic.

@nealef
Copy link

nealef commented Aug 2, 2017

But I can't find any .h file in the /usr/include tree on x86_64 that explicitly includes asm-generic/unistd.h. On x86_64 asm/unistd_xx.h and asm-generic/unistd.h both define things like __NR_mbind. On s390x only the latter does which is why the build fails without the patch.

@streichler
Copy link
Contributor

So on my Ubuntu 14.04, I think an explicit include of <asm-generic/unistd.h> is risky, as it's not the version you get for either 64- or 32-bit builds:

$ echo '#include <sys/syscall.h>' | gcc -E -x c - | grep unistd.h
# 1 "/usr/include/x86_64-linux-gnu/asm/unistd.h" 1 3 4
# 12 "/usr/include/x86_64-linux-gnu/asm/unistd.h" 3 4
# 13 "/usr/include/x86_64-linux-gnu/asm/unistd.h" 2 3 4
$ echo '#include <sys/syscall.h>' | gcc -m32 -E -x c - | grep unistd.h
# 1 "/usr/include/i386-linux-gnu/asm/unistd.h" 1 3 4
# 9 "/usr/include/i386-linux-gnu/asm/unistd.h" 2 3 4

I understand that something is broken with the s390x build, but I'm doubtful that every piece of code that includes sys/syscall.h is supposed to detect s390x and include asm-generic/unistd.h first. Can you give a little more detail about the system you're building on, and hopefully we can figure out how it differs from the s390x system that's been used (indirectly, I think?) by @junghans so far?

@nealef
Copy link

nealef commented Aug 3, 2017

Linux rhel70.devlab.sinenomine.net 3.10.0-229.20.1.el7.s390x #1 SMP Thu Sep 24 12:26:46 EDT 2015 s390x s390x s390x GNU/Linux

[root@rhel70 ~]# cat /etc/os-release
NAME="Red Hat Enterprise Linux Server"
VERSION="7.1 (Maipo)"
ID="rhel"
ID_LIKE="fedora"
VERSION_ID="7.1"
PRETTY_NAME="Employee SKU"
ANSI_COLOR="0;31"
CPE_NAME="cpe:/o:redhat:enterprise_linux:7.1:GA:server"
HOME_URL="https://www.redhat.com/"
BUG_REPORT_URL="https://bugzilla.redhat.com/"
REDHAT_BUGZILLA_PRODUCT="Red Hat Enterprise Linux 7"
REDHAT_BUGZILLA_PRODUCT_VERSION=7.1
REDHAT_SUPPORT_PRODUCT="Red Hat Enterprise Linux"
REDHAT_SUPPORT_PRODUCT_VERSION="7.1"

[root@rhel70 ~]# find /usr/include/ -name "*.h" | xargs grep __NR_mbind
/usr/include/asm-generic/unistd.h:#define __NR_mbind 235
/usr/include/asm-generic/unistd.h:__SC_COMP(__NR_mbind, sys_mbind, compat_sys_mbind)

[root@rhel70 ~]# find /usr/include/ -name "*.h" | xargs grep "asm-generic/unistd"

@streichler
Copy link
Contributor

Do you have a kernel-headers-... package installed? That appears to be where this is supposed to come from on Fedora/RHEL? See https://www.rpmfind.net/linux/RPM/fedora/updates/testing/26/s390x/k/kernel-headers-4.11.11-300.fc26.s390x.html as an example.

@nealef
Copy link

nealef commented Aug 3, 2017

Yes, and this is not my only RHEL system (I have 7.2 and 7.3 systems as well as this one):

rpm -q kernel-headers
kernel-headers-3.10.0-229.20.1.el7.s390x

@nealef
Copy link

nealef commented Aug 3, 2017

rpm -q -f /usr/include/asm-generic/unistd.h
kernel-headers-3.10.0-229.20.1.el7.s390x

@streichler
Copy link
Contributor

Hmm, I think I actually asked the wrong question there. We're not trying to figure out why asm-generic/unistd.h exists on your system - we're trying to figure out why syscall.h isn't including it.

I'm really limited here by my inability to log into any s390x systems to poke around. Could I ask you to do a bit of detective work and figure out if/how things like libnuma and hwloc deal with this? I'm pretty sure both of them need mbind as well.

@nealef
Copy link

nealef commented Aug 3, 2017

hwloc uses __NR_[gs]set_affinity which is defined in <unistd.h>. numactl is not built on s390x.

@nealef
Copy link

nealef commented Aug 3, 2017

I am checking with the IBM developers to see if __NR_mbind should be defined in asm/unistd.h

@nealef
Copy link

nealef commented Aug 3, 2017

I checked with IBM, that call didn't come until a later kernel level (commit 3a368f742da13955bed4a2efed85ed7c1d826bcc) and hasn't been incorporated into any RHEL 7.3 kernel. Not sure what 7.4 will have. The commit date was March 2014.

@nealef
Copy link

nealef commented Aug 3, 2017

Therefore, you can close and ignore this PR.

@junghans
Copy link
Collaborator Author

junghans commented Aug 3, 2017

Ok, should I patch it locally for now in the spec and drop it again in the next version?

@nealef
Copy link

nealef commented Aug 3, 2017 via email

@junghans junghans closed this Aug 3, 2017
@junghans junghans deleted the s390x branch August 3, 2017 15:30
@junghans
Copy link
Collaborator Author

junghans commented Aug 3, 2017

Thanks @streichler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants