Message ID | 1570663013-10269-1-git-send-email-pc@us.ibm.com |
---|---|
State | New |
Headers | show |
Series | [powerpc] Use DIRECT_SYSVIPC_SYSCALLS | expand |
* Paul A. Clarke: > diff --git a/sysdeps/unix/sysv/linux/semop.c b/sysdeps/unix/sysv/linux/semop.c > index 687fdcb..e15bd5e 100644 > --- a/sysdeps/unix/sysv/linux/semop.c > +++ b/sysdeps/unix/sysv/linux/semop.c > @@ -26,7 +26,7 @@ > int > semop (int semid, struct sembuf *sops, size_t nsops) > { > -#ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS > +#if defined (__ASSUME_DIRECT_SYSVIPC_SYSCALLS) && defined (__NR_semop) > return INLINE_SYSCALL_CALL (semop, semid, sops, nsops); > #else > return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); Sorry, but I think this is wrong: If a future kernel version defines __NR_semop, we suddenly build glibc in such a way that it is incompatible with kernel 5.0 on POWER (assuming that the user requests the 5.0 baseline). I think the best way forward here is to fix the kernel to provide the semop system call on POWER, and change __ASSUME_DIRECT_SYSVIPC_SYSCALLS so that it requires that kernel version as the minimum on POWER. If you do not want to do that, maybe the right solution is a sysdeps override for POWER, unconditionally using IPCOP_semop for now, with a comment why this is necessary. Thanks, Florian
On 10/10/2019 03:07, Florian Weimer wrote: > * Paul A. Clarke: > >> diff --git a/sysdeps/unix/sysv/linux/semop.c b/sysdeps/unix/sysv/linux/semop.c >> index 687fdcb..e15bd5e 100644 >> --- a/sysdeps/unix/sysv/linux/semop.c >> +++ b/sysdeps/unix/sysv/linux/semop.c >> @@ -26,7 +26,7 @@ >> int >> semop (int semid, struct sembuf *sops, size_t nsops) >> { >> -#ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS >> +#if defined (__ASSUME_DIRECT_SYSVIPC_SYSCALLS) && defined (__NR_semop) >> return INLINE_SYSCALL_CALL (semop, semid, sops, nsops); >> #else >> return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); > > Sorry, but I think this is wrong: If a future kernel version defines > __NR_semop, we suddenly build glibc in such a way that it is > incompatible with kernel 5.0 on POWER (assuming that the user requests > the 5.0 baseline). > > I think the best way forward here is to fix the kernel to provide the > semop system call on POWER, and change __ASSUME_DIRECT_SYSVIPC_SYSCALLS > so that it requires that kernel version as the minimum on POWER. > > If you do not want to do that, maybe the right solution is a sysdeps > override for POWER, unconditionally using IPCOP_semop for now, with a > comment why this is necessary. I have sent a similar patch some months ago that addresses it not only for powerpc, but in all architecture that have wired up the sysvc syscalls on linux 5.1 [1][2]. My understanding back them was that it was added in fact for Linux 5.1 instead of 5.0 (as perf kernel commit 0d6040d4681735dfc47). And I think this patch is missing addressing the compat symbol usage as well. For the code snippet, my understanding from previous discussions is semtimedop and semop does not exist on 32-bit ABIs and the idea is to *not* provide them in future kernels version (only *semtimedop_time64*). I would expect that powerpc32 would follow the same strategy, however indeed powerpc64 might decide do add in the future. As you have pointed out, tying up wire-up semop with __NR_semop definition might indeed create a wrong build. For this case I would prefer to instead of a sysdep override to do something as below: -- #__ASSUME_DIRECT_SYSVIPC_SYSCALLS # ifdef __NR_semop int res = INLINE_SYSCALL_CALL (semop, semid, sops, nsops); if (ret >= 0 || errno != ENOSYS) return ret; # endif #else return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); -- I will probably rework my patches to use this strategy and check if other 64-bit architectures are affected as well. [1] https://sourceware.org/ml/libc-alpha/2019-05/msg00438.html [2] https://sourceware.org/ml/libc-alpha/2019-05/msg00437.html
On 10/10/19 1:07 AM, Florian Weimer wrote: >> diff --git a/sysdeps/unix/sysv/linux/semop.c b/sysdeps/unix/sysv/linux/semop.c >> index 687fdcb..e15bd5e 100644 >> --- a/sysdeps/unix/sysv/linux/semop.c >> +++ b/sysdeps/unix/sysv/linux/semop.c >> @@ -26,7 +26,7 @@ >> int >> semop (int semid, struct sembuf *sops, size_t nsops) >> { >> -#ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS >> +#if defined (__ASSUME_DIRECT_SYSVIPC_SYSCALLS) && defined (__NR_semop) >> return INLINE_SYSCALL_CALL (semop, semid, sops, nsops); >> #else >> return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); > > Sorry, but I think this is wrong: If a future kernel version defines > __NR_semop, we suddenly build glibc in such a way that it is > incompatible with kernel 5.0 on POWER (assuming that the user requests > the 5.0 baseline). This would present a problem for POWER only when __ASSUME_DIRECT_SYSVIPC_SYSCALLS is set. This is only set (or, not unset) in another part of this same patch and only when __LINUX_KERNEL_VERSION >= 0x050000 (that I believe will come only when "--enable-kernel=5.0.0" is passed to configure). Is that not sufficient? (I understood this to be sufficient based on Joseph's comments in [1].) > I think the best way forward here is to fix the kernel to provide the > semop system call on POWER, and change __ASSUME_DIRECT_SYSVIPC_SYSCALLS > so that it requires that kernel version as the minimum on POWER. The same historical thread referenced above started with a patch [2] from 2015 to realize the same effect. In that thread, Michael Ellerman and Arnd Bergman discuss reverting the original support patches from Linux 4.4, which don't appear again, it seems, until Linux 5.0. But, it seems that "semop" was not included in that patchset (Linux kernel commit 0d6040d468173). Is that expected, Arnd? > If you do not want to do that, maybe the right solution is a sysdeps > override for POWER, unconditionally using IPCOP_semop for now, with a > comment why this is necessary. So, something like this, in sysdeps/unix/sysv/linux/semop.c? -- +/* [powerpc64 only] While most SysV IPC syscalls are implemented as direct + syscalls on powerpc64, semop is not. */ +#if defined (__ASSUME_DIRECT_SYSVIPC_SYSCALLS) && !defined (__powerpc64__) return INLINE_SYSCALL_CALL (semop, semid, sops, nsops); #else return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); +#endif -- [1] https://sourceware.org/ml/libc-alpha/2015-12/msg00065.html [2] https://sourceware.org/ml/libc-alpha/2015-12/msg00051.html PC
* Paul Clarke: > On 10/10/19 1:07 AM, Florian Weimer wrote: >>> diff --git a/sysdeps/unix/sysv/linux/semop.c b/sysdeps/unix/sysv/linux/semop.c >>> index 687fdcb..e15bd5e 100644 >>> --- a/sysdeps/unix/sysv/linux/semop.c >>> +++ b/sysdeps/unix/sysv/linux/semop.c >>> @@ -26,7 +26,7 @@ >>> int >>> semop (int semid, struct sembuf *sops, size_t nsops) >>> { >>> -#ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS >>> +#if defined (__ASSUME_DIRECT_SYSVIPC_SYSCALLS) && defined (__NR_semop) >>> return INLINE_SYSCALL_CALL (semop, semid, sops, nsops); >>> #else >>> return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); >> >> Sorry, but I think this is wrong: If a future kernel version defines >> __NR_semop, we suddenly build glibc in such a way that it is >> incompatible with kernel 5.0 on POWER (assuming that the user requests >> the 5.0 baseline). > > This would present a problem for POWER only when > __ASSUME_DIRECT_SYSVIPC_SYSCALLS is set. This is only set (or, not > unset) in another part of this same patch and only when > __LINUX_KERNEL_VERSION >= 0x050000 (that I believe will come only when > "--enable-kernel=5.0.0" is passed to configure). Is that not > sufficient? (I understood this to be sufficient based on Joseph's > comments in [1].) I would expect --enable-kernel=5.0.0 to set a kernel 5.0 baseline even if kernel 5.11 (a made that up) adds the semop system call on POWER. My understanding that this wouldn't work with your patch, it would require kernel 5.11 for a working semop function, not kernel 5.0 as requested. I hope this clarifies my concern. >> If you do not want to do that, maybe the right solution is a sysdeps >> override for POWER, unconditionally using IPCOP_semop for now, with a >> comment why this is necessary. > > So, something like this, in sysdeps/unix/sysv/linux/semop.c? > -- > +/* [powerpc64 only] While most SysV IPC syscalls are implemented as direct > + syscalls on powerpc64, semop is not. */ > +#if defined (__ASSUME_DIRECT_SYSVIPC_SYSCALLS) && !defined (__powerpc64__) > return INLINE_SYSCALL_CALL (semop, semid, sops, nsops); > #else > return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); > +#endif > -- If ou want to restrict it to powerpc64, you'd have to put a file into sysdeps/unix/sysv/linux/powerpc/powerpc64/semop.c with essentially this: int semop (int semid, struct sembuf *sops, size_t nsops) { /* POWER does not support the semop system call even with __ASSUME_DIRECT_SYSVIPC_SYSCALLS. */ return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); } You can't write an __ASSUME check yet because we don't know the correct kernel version yet. Thanks, Florian
On 10/10/19 10:13 AM, Adhemerval Zanella wrote: > On 10/10/2019 03:07, Florian Weimer wrote: >>> diff --git a/sysdeps/unix/sysv/linux/semop.c b/sysdeps/unix/sysv/linux/semop.c >>> index 687fdcb..e15bd5e 100644 >>> --- a/sysdeps/unix/sysv/linux/semop.c >>> +++ b/sysdeps/unix/sysv/linux/semop.c >>> @@ -26,7 +26,7 @@ >>> int >>> semop (int semid, struct sembuf *sops, size_t nsops) >>> { >>> -#ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS >>> +#if defined (__ASSUME_DIRECT_SYSVIPC_SYSCALLS) && defined (__NR_semop) >>> return INLINE_SYSCALL_CALL (semop, semid, sops, nsops); >>> #else >>> return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); >> >> Sorry, but I think this is wrong: If a future kernel version defines >> __NR_semop, we suddenly build glibc in such a way that it is >> incompatible with kernel 5.0 on POWER (assuming that the user requests >> the 5.0 baseline). >> >> I think the best way forward here is to fix the kernel to provide the >> semop system call on POWER, and change __ASSUME_DIRECT_SYSVIPC_SYSCALLS >> so that it requires that kernel version as the minimum on POWER. >> >> If you do not want to do that, maybe the right solution is a sysdeps >> override for POWER, unconditionally using IPCOP_semop for now, with a >> comment why this is necessary. > > I have sent a similar patch some months ago that addresses it not only > for powerpc, but in all architecture that have wired up the sysvc syscalls > on linux 5.1 [1][2]. My understanding back them was that it was added in > fact for Linux 5.1 instead of 5.0 (as perf kernel commit 0d6040d4681735dfc47). That commit is in v5.0: -- $ git checkout v5.0 ... Note: checking out 'v5.0'. ... HEAD is now at 1c163f4c7b3f Linux 5.0 $ git log 0d6040d4681735dfc47 -1 commit 0d6040d4681735dfc47565de288525de405a5c99 Author: Arnd Bergmann <arnd@arndb.de> Date: Mon Dec 31 14:38:26 2018 +0100 arch: add split IPC system calls where needed -- > And I think this patch is missing addressing the compat symbol usage > as well. Likely due to my ignorance. :-/ > For the code snippet, my understanding from previous discussions is > semtimedop and semop does not exist on 32-bit ABIs and the idea is to > *not* provide them in future kernels version (only *semtimedop_time64*). > I would expect that powerpc32 would follow the same strategy, however > indeed powerpc64 might decide do add in the future. > > As you have pointed out, tying up wire-up semop with __NR_semop > definition might indeed create a wrong build. I asked to clarify this concern in my reply to Florian. Since it is also gated by __ASSUME_DIRECT_SYSVIPC_SYSCALLS and that would be gated by __LINUX_VERSION >= 0x050000, which in turn comes from "--enable-kernel=5.0.0", is that not sufficient? > For this case I > would prefer to instead of a sysdep override to do something as below: > -- > #__ASSUME_DIRECT_SYSVIPC_SYSCALLS > # ifdef __NR_semop > int res = INLINE_SYSCALL_CALL (semop, semid, sops, nsops); > if (ret >= 0 || errno != ENOSYS) > return ret; > # endif what happens here if __NR_semop is not defined? > #else > return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); > -- > > I will probably rework my patches to use this strategy and check > if other 64-bit architectures are affected as well. I presume this would make my patch moot (which is fine, if so). Let me know if not. > [1] https://sourceware.org/ml/libc-alpha/2019-05/msg00438.html > [2] https://sourceware.org/ml/libc-alpha/2019-05/msg00437.html PC
On 10/10/2019 13:35, Paul Clarke wrote: > On 10/10/19 10:13 AM, Adhemerval Zanella wrote: >> On 10/10/2019 03:07, Florian Weimer wrote: >>>> diff --git a/sysdeps/unix/sysv/linux/semop.c b/sysdeps/unix/sysv/linux/semop.c >>>> index 687fdcb..e15bd5e 100644 >>>> --- a/sysdeps/unix/sysv/linux/semop.c >>>> +++ b/sysdeps/unix/sysv/linux/semop.c >>>> @@ -26,7 +26,7 @@ >>>> int >>>> semop (int semid, struct sembuf *sops, size_t nsops) >>>> { >>>> -#ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS >>>> +#if defined (__ASSUME_DIRECT_SYSVIPC_SYSCALLS) && defined (__NR_semop) >>>> return INLINE_SYSCALL_CALL (semop, semid, sops, nsops); >>>> #else >>>> return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); >>> >>> Sorry, but I think this is wrong: If a future kernel version defines >>> __NR_semop, we suddenly build glibc in such a way that it is >>> incompatible with kernel 5.0 on POWER (assuming that the user requests >>> the 5.0 baseline). >>> >>> I think the best way forward here is to fix the kernel to provide the >>> semop system call on POWER, and change __ASSUME_DIRECT_SYSVIPC_SYSCALLS >>> so that it requires that kernel version as the minimum on POWER. >>> >>> If you do not want to do that, maybe the right solution is a sysdeps >>> override for POWER, unconditionally using IPCOP_semop for now, with a >>> comment why this is necessary. >> >> I have sent a similar patch some months ago that addresses it not only >> for powerpc, but in all architecture that have wired up the sysvc syscalls >> on linux 5.1 [1][2]. My understanding back them was that it was added in >> fact for Linux 5.1 instead of 5.0 (as perf kernel commit 0d6040d4681735dfc47). > > That commit is in v5.0: > -- > $ git checkout v5.0 > ... > Note: checking out 'v5.0'. > ... > HEAD is now at 1c163f4c7b3f Linux 5.0 > $ git log 0d6040d4681735dfc47 -1 > commit 0d6040d4681735dfc47565de288525de405a5c99 > Author: Arnd Bergmann <arnd@arndb.de> > Date: Mon Dec 31 14:38:26 2018 +0100 > > arch: add split IPC system calls where needed I am sure this command gives you the correct answer, afaik this command gives you the first tag containing the commit: $ git describe --contains 0d6040d4681735dfc47 v5.1-rc1~160^2~3^2~4 In fact a simple test to check if 'semget' is present at the system call table shows: $ git checkout v5.0 [...] HEAD is now at 1c163f4c7b3f Linux 5.0 $ grep semget arch/powerpc/kernel/syscalls/syscall.tbl $ > -- > >> And I think this patch is missing addressing the compat symbol usage >> as well. > > Likely due to my ignorance. :-/ > >> For the code snippet, my understanding from previous discussions is >> semtimedop and semop does not exist on 32-bit ABIs and the idea is to >> *not* provide them in future kernels version (only *semtimedop_time64*). >> I would expect that powerpc32 would follow the same strategy, however >> indeed powerpc64 might decide do add in the future. >> >> As you have pointed out, tying up wire-up semop with __NR_semop >> definition might indeed create a wrong build. > > I asked to clarify this concern in my reply to Florian. Since it is also gated by __ASSUME_DIRECT_SYSVIPC_SYSCALLS and that would be gated by __LINUX_VERSION >= 0x050000, which in turn comes from "--enable-kernel=5.0.0", is that not sufficient? The issue is assuming __NR_semop is wire-up on 5.x and you build glibc with 5.y headers (with x >= y) along with --enable-kernel=5.0.0. The __NR_op will be defines and the wire-up path will be always used. This will make semop fail on kernels 5.0 to 5.(x-1). > >> For this case I >> would prefer to instead of a sysdep override to do something as below: >> -- >> #__ASSUME_DIRECT_SYSVIPC_SYSCALLS >> # ifdef __NR_semop >> int res = INLINE_SYSCALL_CALL (semop, semid, sops, nsops); >> if (ret >= 0 || errno != ENOSYS) >> return ret; >> # endif > > what happens here if __NR_semop is not defined? The fallback ipc call will be used instead, which always works with minimum kernel version supported (3.2). > >> #else >> return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); >> -- >> >> I will probably rework my patches to use this strategy and check >> if other 64-bit architectures are affected as well. > > I presume this would make my patch moot (which is fine, if so). Let me know if not. > >> [1] https://sourceware.org/ml/libc-alpha/2019-05/msg00438.html >> [2] https://sourceware.org/ml/libc-alpha/2019-05/msg00437.html > > PC > I am working on fixing this on my patchset, it should enable the new wire-up sysvipc interface on all architectures that supports it (instead of just powerpc).
On 10/10/2019 13:49, Adhemerval Zanella wrote: > > > On 10/10/2019 13:35, Paul Clarke wrote: >> On 10/10/19 10:13 AM, Adhemerval Zanella wrote: > > I am sure this command gives you the correct answer, afaik this command I am *not* sure [...]
On 10/10/19 11:49 AM, Adhemerval Zanella wrote: > On 10/10/2019 13:35, Paul Clarke wrote: >> On 10/10/19 10:13 AM, Adhemerval Zanella wrote: >>> On 10/10/2019 03:07, Florian Weimer wrote: >>>>> diff --git a/sysdeps/unix/sysv/linux/semop.c b/sysdeps/unix/sysv/linux/semop.c >>>>> index 687fdcb..e15bd5e 100644 >>>>> --- a/sysdeps/unix/sysv/linux/semop.c >>>>> +++ b/sysdeps/unix/sysv/linux/semop.c >>>>> @@ -26,7 +26,7 @@ >>>>> int >>>>> semop (int semid, struct sembuf *sops, size_t nsops) >>>>> { >>>>> -#ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS >>>>> +#if defined (__ASSUME_DIRECT_SYSVIPC_SYSCALLS) && defined (__NR_semop) >>>>> return INLINE_SYSCALL_CALL (semop, semid, sops, nsops); >>>>> #else >>>>> return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops); >>>> >>>> Sorry, but I think this is wrong: If a future kernel version defines >>>> __NR_semop, we suddenly build glibc in such a way that it is >>>> incompatible with kernel 5.0 on POWER (assuming that the user requests >>>> the 5.0 baseline). >>>> >>>> I think the best way forward here is to fix the kernel to provide the >>>> semop system call on POWER, and change __ASSUME_DIRECT_SYSVIPC_SYSCALLS >>>> so that it requires that kernel version as the minimum on POWER. >>>> >>>> If you do not want to do that, maybe the right solution is a sysdeps >>>> override for POWER, unconditionally using IPCOP_semop for now, with a >>>> comment why this is necessary. >>> >>> I have sent a similar patch some months ago that addresses it not only >>> for powerpc, but in all architecture that have wired up the sysvc syscalls >>> on linux 5.1 [1][2]. My understanding back them was that it was added in >>> fact for Linux 5.1 instead of 5.0 (as perf kernel commit 0d6040d4681735dfc47). >> >> That commit is in v5.0: >> -- >> $ git checkout v5.0 >> ... >> Note: checking out 'v5.0'. >> ... >> HEAD is now at 1c163f4c7b3f Linux 5.0 >> $ git log 0d6040d4681735dfc47 -1 >> commit 0d6040d4681735dfc47565de288525de405a5c99 >> Author: Arnd Bergmann <arnd@arndb.de> >> Date: Mon Dec 31 14:38:26 2018 +0100 >> >> arch: add split IPC system calls where needed > > I am [not] sure this command gives you the correct answer, afaik this command > gives you the first tag containing the commit: > > $ git describe --contains 0d6040d4681735dfc47 > v5.1-rc1~160^2~3^2~4 > > In fact a simple test to check if 'semget' is present at the system call > table shows: > > $ git checkout v5.0 > [...] > HEAD is now at 1c163f4c7b3f Linux 5.0 > $ grep semget arch/powerpc/kernel/syscalls/syscall.tbl > $ Ack. My git-fu is still developing... PC
On Thu, Oct 10, 2019 at 6:18 PM Paul Clarke <pc@us.ibm.com> wrote: > On 10/10/19 1:07 AM, Florian Weimer wrote: > > I think the best way forward here is to fix the kernel to provide the > > semop system call on POWER, and change __ASSUME_DIRECT_SYSVIPC_SYSCALLS > > so that it requires that kernel version as the minimum on POWER. > > The same historical thread referenced above started with a patch [2] from 2015 to realize the same effect. In that thread, Michael Ellerman and Arnd Bergman discuss reverting the original support patches from Linux 4.4, which don't appear again, it seems, until Linux 5.0. But, it seems that "semop" was not included in that patchset (Linux kernel commit 0d6040d468173). Is that expected, Arnd? Yes, that was intentional: as semop is a trivial subset of semtimedop/semtimedop_time64, it seemed pointless to add both, as I wrote in the changeset text then: "I'm not adding the new semtimedop() or semop() on 32-bit architectures, those will get implemented using the new semtimedop_time64() version that gets added along with the other time64 calls. Three 64-bit architectures (powerpc, s390 and sparc) get semtimedop()." My expectation then was that glibc would make its semop() interface a wrapper around its own semtimedop() interface, which needs to be implemented anyway. That implementation would then be shared across all architectures. However, I also did not expect any existing C library to migrate from the existing ipc() syscall to the direct calls unless it requires linux-5.1 kernels, which I guess would be 2025 at the earliest for architectures already supported in glibc. In particular, the decision to drop the IPC_64 flag was intended to simplify a libc implementation that only cares about the new syscalls by limiting the special-case architectures to alpha, arm32, microblaze, mips, sparc and xtensa. If you need to deal with an architecture having different IPC_64 semantics for direct and indirect syscalls, that clearly adds complexity instead of reducing it as I had hoped. Arnd
On Thu, 10 Oct 2019, Adhemerval Zanella wrote: > As you have pointed out, tying up wire-up semop with __NR_semop > definition might indeed create a wrong build. For this case I > would prefer to instead of a sysdep override to do something as below: > > -- > #__ASSUME_DIRECT_SYSVIPC_SYSCALLS > # ifdef __NR_semop > int res = INLINE_SYSCALL_CALL (semop, semid, sops, nsops); > if (ret >= 0 || errno != ENOSYS) > return ret; If __ASSUME_DIRECT_SYSVIPC_SYSCALLS does not imply presence of the semop syscall, that should be made clear in the comment on the default definition. If in addition the semop syscall might be added on architectures that do define __ASSUME_DIRECT_SYSVIPC_SYSCALLS but don't currently define semop, to me that indicates a separate __ASSUME_SEMOP_SYSCALL is needed. (Note how we have lots of separate __ASSUME_* macros for socket syscalls, which avoids such complications in using a macro that's supposed to relate to lots of syscalls that aren't all present together.)
* Arnd Bergmann: > On Thu, Oct 10, 2019 at 6:18 PM Paul Clarke <pc@us.ibm.com> wrote: >> On 10/10/19 1:07 AM, Florian Weimer wrote: > >> > I think the best way forward here is to fix the kernel to provide the >> > semop system call on POWER, and change __ASSUME_DIRECT_SYSVIPC_SYSCALLS >> > so that it requires that kernel version as the minimum on POWER. >> >> The same historical thread referenced above started with a patch [2] from 2015 to realize the same effect. In that thread, Michael Ellerman and Arnd Bergman discuss reverting the original support patches from Linux 4.4, which don't appear again, it seems, until Linux 5.0. But, it seems that "semop" was not included in that patchset (Linux kernel commit 0d6040d468173). Is that expected, Arnd? > > Yes, that was intentional: as semop is a trivial subset of > semtimedop/semtimedop_time64, It's not so trivial anymore because you need to pick the right syscall to replace it. 8-/ > it seemed pointless to add both, as I wrote in the changeset text then: > > "I'm not adding the new semtimedop() or semop() on 32-bit architectures, > those will get implemented using the new semtimedop_time64() version > that gets added along with the other time64 calls. > Three 64-bit architectures (powerpc, s390 and sparc) get semtimedop()." > > My expectation then was that glibc would make its semop() interface a > wrapper around its own semtimedop() interface, which needs to be > implemented anyway. That implementation would then be shared across > all architectures. Thanks for this background. This suggests to me that with __ASSUME_DIRECT_SYSVIPC_SYSCALLS, the generic implementation should use semtimedop or semtimedop_time64, and that in the interim, Paul's patch (with a commont explaining why) is safe with regards future evolution of the kernel. Thanks, Florian
On 11/10/2019 06:19, Florian Weimer wrote: > * Arnd Bergmann: > >> On Thu, Oct 10, 2019 at 6:18 PM Paul Clarke <pc@us.ibm.com> wrote: >>> On 10/10/19 1:07 AM, Florian Weimer wrote: >> >>>> I think the best way forward here is to fix the kernel to provide the >>>> semop system call on POWER, and change __ASSUME_DIRECT_SYSVIPC_SYSCALLS >>>> so that it requires that kernel version as the minimum on POWER. >>> >>> The same historical thread referenced above started with a patch [2] from 2015 to realize the same effect. In that thread, Michael Ellerman and Arnd Bergman discuss reverting the original support patches from Linux 4.4, which don't appear again, it seems, until Linux 5.0. But, it seems that "semop" was not included in that patchset (Linux kernel commit 0d6040d468173). Is that expected, Arnd? >> >> Yes, that was intentional: as semop is a trivial subset of >> semtimedop/semtimedop_time64, > > It's not so trivial anymore because you need to pick the right syscall > to replace it. 8-/ > >> it seemed pointless to add both, as I wrote in the changeset text then: >> >> "I'm not adding the new semtimedop() or semop() on 32-bit architectures, >> those will get implemented using the new semtimedop_time64() version >> that gets added along with the other time64 calls. >> Three 64-bit architectures (powerpc, s390 and sparc) get semtimedop()." >> >> My expectation then was that glibc would make its semop() interface a >> wrapper around its own semtimedop() interface, which needs to be >> implemented anyway. That implementation would then be shared across >> all architectures. > > Thanks for this background. This suggests to me that with > __ASSUME_DIRECT_SYSVIPC_SYSCALLS, the generic implementation should use > semtimedop or semtimedop_time64, and that in the interim, Paul's patch > (with a commont explaining why) is safe with regards future evolution of > the kernel. I think it would simpler then to just refactor semop to call semtimedop, so the wire-up support only will need to touch semtimedop code. In any case I will send a updated version on my previous proposal (which is similar of what Paul sent, but with wire-up support for affected architectures).
diff --git a/sysdeps/unix/sysv/linux/powerpc/ipc_priv.h b/sysdeps/unix/sysv/linux/powerpc/ipc_priv.h index 96372bc..e56fcae 100644 --- a/sysdeps/unix/sysv/linux/powerpc/ipc_priv.h +++ b/sysdeps/unix/sysv/linux/powerpc/ipc_priv.h @@ -18,7 +18,11 @@ #include <sys/ipc.h> /* For __key_t */ +#if __LINUX_KERNEL_VERSION < 0x050000 #define __IPC_64 0x100 +#else +#define __IPC_64 0 +#endif struct __old_ipc_perm { diff --git a/sysdeps/unix/sysv/linux/powerpc/kernel-features.h b/sysdeps/unix/sysv/linux/powerpc/kernel-features.h index d177a91..b2c824f 100644 --- a/sysdeps/unix/sysv/linux/powerpc/kernel-features.h +++ b/sysdeps/unix/sysv/linux/powerpc/kernel-features.h @@ -44,8 +44,10 @@ #include_next <kernel-features.h> +#if __LINUX_KERNEL_VERSION < 0x050000 /* powerpc only supports ipc syscall. */ #undef __ASSUME_DIRECT_SYSVIPC_SYSCALLS +#endif #undef __ASSUME_CLONE_DEFAULT #define __ASSUME_CLONE_BACKWARDS 1 diff --git a/sysdeps/unix/sysv/linux/semop.c b/sysdeps/unix/sysv/linux/semop.c index 687fdcb..e15bd5e 100644 --- a/sysdeps/unix/sysv/linux/semop.c +++ b/sysdeps/unix/sysv/linux/semop.c @@ -26,7 +26,7 @@ int semop (int semid, struct sembuf *sops, size_t nsops) { -#ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS +#if defined (__ASSUME_DIRECT_SYSVIPC_SYSCALLS) && defined (__NR_semop) return INLINE_SYSCALL_CALL (semop, semid, sops, nsops); #else return INLINE_SYSCALL_CALL (ipc, IPCOP_semop, semid, nsops, 0, sops);
From: "Paul A. Clarke" <pc@us.ibm.com> Support for direct (non-multiplexed) SysV IPC syscalls was added for powerpc in Linux 5.0. Support in glibc is already present, but disabled for powerpc. 1. Enable this support for powerpc if glibc is built for kernels >= 5.0.0. 2. Stop passing __IPC_64 bit in associated commands, as this is no longer required or expected with this support on powerpc. 3. "semop" is in the set of "SysV IPC" syscalls which are enabled in glibc, but semop is not supported on powerpc even in the Linux 5.0 kernel, so disable this syscall if __NR_semop is not defined. 2019-10-09 Paul A. Clarke <pc@us.ibm.com> * sysdeps/unix/sysv/linux/powerpc/kernel-features.h: Enable (do not disable) __ASSUME_DIRECT_SYSVIPC_SYSCALLS for Linux >= 5.0.0. * sysdeps/unix/sysv/linux/powerpc/ipc_priv.h: Nullify __IPC_64 for Linux >= 5.0.0. * sysdeps/unix/sysv/linux/semop.c: Disable DIRECT_SYSCALL unless __NR_semop is defined. --- Tested on openSUSE Tumbleweed 20190814 with kernel 5.1.3-1-default. A hacked up benchmark (which called msgctl (0,0,0) and thus returned failure, so basically just testing system call overhead), showed about 12% improvement in latency. sysdeps/unix/sysv/linux/powerpc/ipc_priv.h | 4 ++++ sysdeps/unix/sysv/linux/powerpc/kernel-features.h | 2 ++ sysdeps/unix/sysv/linux/semop.c | 2 +- 3 files changed, 7 insertions(+), 1 deletion(-)