This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] [powerpc] Use DIRECT_SYSVIPC_SYSCALLS
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).