semtimedop, powerpc, time64 and older kernels

  Hi Adhemerval,

sysvipc/test-sysvsem started failing on ppc64le on kernels older than
5.1 after:

commit aaa12e9ff02b32d5fbb2f367d7d6b6985a2176d6
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Fri Sep 25 15:04:34 2020 -0300

    sysvipc: Fix semtimeop for !__ASSUME_DIRECT_SYSVIPC_SYSCALLS

    The __NR_ipc syscall does not support 64-bit time operations.  It
    fixes 7c437d3778.

    Checked on i686-linux-gnu on a Linux 5.4.

It fails with:
FAIL: sysvipc/test-sysvsem
original exit status 1
error: test-sysvsem.c:101: semop failed (errno=38)
error: 1 test failures

Looks like semtimedop was added on Linux 5.1, so it makes sense that
older kernels will fail with ENOSYS when calling that. So in such cases
should we also apply time convertion and fallback to semtimedop/ipc in
__semtimedop64 as done when !__ASSUME_DIRECT_SYSVIPC_SYSCALLS?

The patch below seems to solve the issue, at least on ppc64le.

Suggestions?

Thanks,
Matheus Castanho

--- 8< ---

     return r;
  

On 9/30/20 3:01 PM, Matheus Castanho via Libc-alpha wrote:
> Hi Adhemerval,
> 
> sysvipc/test-sysvsem started failing on ppc64le on kernels older than
> 5.1 after:
> 
> commit aaa12e9ff02b32d5fbb2f367d7d6b6985a2176d6
> Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Date:   Fri Sep 25 15:04:34 2020 -0300
> 
>     sysvipc: Fix semtimeop for !__ASSUME_DIRECT_SYSVIPC_SYSCALLS
> 
>     The __NR_ipc syscall does not support 64-bit time operations.  It
>     fixes 7c437d3778.
> 
>     Checked on i686-linux-gnu on a Linux 5.4.
> 
> It fails with:
> FAIL: sysvipc/test-sysvsem
> original exit status 1
> error: test-sysvsem.c:101: semop failed (errno=38)
> error: 1 test failures
> 
> Looks like semtimedop was added on Linux 5.1, so it makes sense that
> older kernels will fail with ENOSYS when calling that. So in such cases
> should we also apply time convertion and fallback to semtimedop/ipc in
> __semtimedop64 as done when !__ASSUME_DIRECT_SYSVIPC_SYSCALLS?
                        -----------------^

Sorry, I meant !_ASSUME_TIME64_SYSCALLS

> 
> The patch below seems to solve the issue, at least on ppc64le.
> 
> Suggestions?
> 
> Thanks,
> Matheus Castanho
> 
> --- 8< ---
> 
> diff --git a/sysdeps/unix/sysv/linux/semtimedop.c
> b/sysdeps/unix/sysv/linux/semtimedop.c
> index a9ad922ee2..510fea1852 100644
> --- a/sysdeps/unix/sysv/linux/semtimedop.c
> +++ b/sysdeps/unix/sysv/linux/semtimedop.c
> @@ -32,7 +32,7 @@ __semtimedop64 (int semid, struct sembuf *sops, size_t
> nsops,
>    int r = INLINE_SYSCALL_CALL (semtimedop_time64, semid, sops, nsops,
>                                timeout);
> 
> -#ifndef __ASSUME_TIME64_SYSCALLS
> +#if !(defined __ASSUME_TIME64_SYSCALLS) || __LINUX_KERNEL_VERSION <
> 0x050100
>    if (r == 0 || errno != ENOSYS)
>      return r;
> 

Also, looks like my email client messed up the diff *sigh*. I'm sending
a proper patch attached this time.

--
Matheus Castanho
  

On Sep 30 2020, Matheus Castanho via Libc-alpha wrote:

> diff --git a/sysdeps/unix/sysv/linux/semtimedop.c b/sysdeps/unix/sysv/linux/semtimedop.c
> index a9ad922ee2..510fea1852 100644
> --- a/sysdeps/unix/sysv/linux/semtimedop.c
> +++ b/sysdeps/unix/sysv/linux/semtimedop.c
> @@ -32,7 +32,7 @@ __semtimedop64 (int semid, struct sembuf *sops, size_t nsops,
>    int r = INLINE_SYSCALL_CALL (semtimedop_time64, semid, sops, nsops,
>  			       timeout);
>  
> -#ifndef __ASSUME_TIME64_SYSCALLS
> +#if !(defined __ASSUME_TIME64_SYSCALLS) || __LINUX_KERNEL_VERSION < 0x050100

Don't use __LINUX_KERNEL_VERSION directly, instead add a new __ASSUME
define in sysdeps/unix/sysv/linux/kernel-features.h and use that.

Andreas.
  

On 30/09/2020 15:29, Matheus Castanho wrote:
> Also, looks like my email client messed up the diff *sigh*. I'm sending
> a proper patch attached this time.
> 
> --
> Matheus Castanho
> 

> From 1c0a497a3f986bc6980581c9eab482ccf7bb190f Mon Sep 17 00:00:00 2001
> From: Matheus Castanho <msc@linux.ibm.com>
> Date: Wed, 30 Sep 2020 14:22:18 -0300
> Subject: [PATCH] sysvipc: Fix semtimedop for Linux < 5.1
> 
> Kernels older than 5.1 will fail with ENOSYS when calling
> semtimedop_time64 syscall in __semtimedop_time64. Just like for
> !__ASSUME_TIME64_SYSCALLS, we should fallback to using the old mechanism
> in such cases.
> ---
>  sysdeps/unix/sysv/linux/semtimedop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/semtimedop.c b/sysdeps/unix/sysv/linux/semtimedop.c
> index a9ad922ee2..510fea1852 100644
> --- a/sysdeps/unix/sysv/linux/semtimedop.c
> +++ b/sysdeps/unix/sysv/linux/semtimedop.c
> @@ -32,7 +32,7 @@ __semtimedop64 (int semid, struct sembuf *sops, size_t nsops,
>    int r = INLINE_SYSCALL_CALL (semtimedop_time64, semid, sops, nsops,
>  			       timeout);
>  
> -#ifndef __ASSUME_TIME64_SYSCALLS
> +#if !(defined __ASSUME_TIME64_SYSCALLS) || __LINUX_KERNEL_VERSION < 0x050100
>    if (r == 0 || errno != ENOSYS)
>      return r;
>  
> -- 
> 2.26.2

Thanks for catching it and although it fixes the regression, we have 
kernel-features.h exactly to avoid using __LINUX_KERNEL_VERSION through the
implementations. Also this is sub-optimal since it forces semtimeopd issues
__NR_semtimeop and then __NR_ipc on powerpc64 and we have 
__ASSUME_DIRECT_SYSVIPC_SYSCALLS exactly to avoid this strategy of handling 
ENOSYS for newer syscalls and thus slowing it down the implementation on 
older kernels (--enable-kernel exists exactly to get rid of this older kernel
support).

I forgot that powerpc64 and s390x used the older multiplexed __NR_ipc and
kernel v5.1 decided to add proper __NR_semtimedop (and it was in fact handled
by 720e9541f5d919).  I think a better fix is the one below, since it:

  1. Issues __NR_semtimeop_time64 iff it is defined (32-bit architectures).
  2. Issues __NR_semtimeop otherwise iff glibc is configured for a kernel that
     supports it (for powerpc64 it will only for --enable-kernel=5.1). 
     Otherwise it will use only 3.
  3. Issues __NR_ipc with IPCOP_semtimedop.

For powerpc64 it will issue either __NR_ipc (default) or __NR_semtimeop
(--enable-kernel=5.1), while for powerpc it will use either
__NR_semtimeop_time64 and fallback to __NR_ipc or just issue
__NR_semtimeop_time64.

I am running some regressions before commit it.

---

diff --git a/sysdeps/unix/sysv/linux/semtimedop.c b/sysdeps/unix/sysv/linux/semtimedop.c
index a9ad922ee2..29647f8ccd 100644
--- a/sysdeps/unix/sysv/linux/semtimedop.c
+++ b/sysdeps/unix/sysv/linux/semtimedop.c
@@ -26,11 +26,15 @@ int
 __semtimedop64 (int semid, struct sembuf *sops, size_t nsops,
                const struct __timespec64 *timeout)
 {
-#ifndef __NR_semtimedop_time64
-# define __NR_semtimedop_time64 __NR_semtimedop
+  int r;
+#if defined __NR_semtimedop_time64
+  r = INLINE_SYSCALL_CALL (semtimedop_time64, semid, sops, nsops, timeout);
+#elif defined __ASSUME_DIRECT_SYSVIPC_SYSCALLS && defined __NR_semtimedop
+  r = INLINE_SYSCALL_CALL (semtimedop, semid, sops, nsops, timeout);
+#else
+  r = INLINE_SYSCALL_CALL (ipc, IPCOP_semtimedop, semid,
+                          SEMTIMEDOP_IPC_ARGS (nsops, sops, timeout));
 #endif
-  int r = INLINE_SYSCALL_CALL (semtimedop_time64, semid, sops, nsops,
-                              timeout);
 
 #ifndef __ASSUME_TIME64_SYSCALLS
   if (r == 0 || errno != ENOSYS)
  

On 9/30/20 4:12 PM, Adhemerval Zanella wrote:
> 
> 
> On 30/09/2020 15:29, Matheus Castanho wrote:
>> Also, looks like my email client messed up the diff *sigh*. I'm sending
>> a proper patch attached this time.
>>
>> --
>> Matheus Castanho
>>
> 
>> From 1c0a497a3f986bc6980581c9eab482ccf7bb190f Mon Sep 17 00:00:00 2001
>> From: Matheus Castanho <msc@linux.ibm.com>
>> Date: Wed, 30 Sep 2020 14:22:18 -0300
>> Subject: [PATCH] sysvipc: Fix semtimedop for Linux < 5.1
>>
>> Kernels older than 5.1 will fail with ENOSYS when calling
>> semtimedop_time64 syscall in __semtimedop_time64. Just like for
>> !__ASSUME_TIME64_SYSCALLS, we should fallback to using the old mechanism
>> in such cases.
>> ---
>>  sysdeps/unix/sysv/linux/semtimedop.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sysdeps/unix/sysv/linux/semtimedop.c b/sysdeps/unix/sysv/linux/semtimedop.c
>> index a9ad922ee2..510fea1852 100644
>> --- a/sysdeps/unix/sysv/linux/semtimedop.c
>> +++ b/sysdeps/unix/sysv/linux/semtimedop.c
>> @@ -32,7 +32,7 @@ __semtimedop64 (int semid, struct sembuf *sops, size_t nsops,
>>    int r = INLINE_SYSCALL_CALL (semtimedop_time64, semid, sops, nsops,
>>  			       timeout);
>>  
>> -#ifndef __ASSUME_TIME64_SYSCALLS
>> +#if !(defined __ASSUME_TIME64_SYSCALLS) || __LINUX_KERNEL_VERSION < 0x050100
>>    if (r == 0 || errno != ENOSYS)
>>      return r;
>>  
>> -- 
>> 2.26.2
> 
> Thanks for catching it and although it fixes the regression, we have 
> kernel-features.h exactly to avoid using __LINUX_KERNEL_VERSION through the
> implementations. Also this is sub-optimal since it forces semtimeopd issues
> __NR_semtimeop and then __NR_ipc on powerpc64 and we have 
> __ASSUME_DIRECT_SYSVIPC_SYSCALLS exactly to avoid this strategy of handling 
> ENOSYS for newer syscalls and thus slowing it down the implementation on 
> older kernels (--enable-kernel exists exactly to get rid of this older kernel
> support).
> 

Thanks for the explanation.

> I forgot that powerpc64 and s390x used the older multiplexed __NR_ipc and
> kernel v5.1 decided to add proper __NR_semtimedop (and it was in fact handled
> by 720e9541f5d919).  I think a better fix is the one below, since it:
> 
>   1. Issues __NR_semtimeop_time64 iff it is defined (32-bit architectures).
>   2. Issues __NR_semtimeop otherwise iff glibc is configured for a kernel that
>      supports it (for powerpc64 it will only for --enable-kernel=5.1). 
>      Otherwise it will use only 3.
>   3. Issues __NR_ipc with IPCOP_semtimedop.
> 
> For powerpc64 it will issue either __NR_ipc (default) or __NR_semtimeop
> (--enable-kernel=5.1), while for powerpc it will use either
> __NR_semtimeop_time64 and fallback to __NR_ipc or just issue
> __NR_semtimeop_time64.

Sounds good to me. That is indeed a better solution. Thanks!

> 
> I am running some regressions before commit it.
> 
> ---
> 
> diff --git a/sysdeps/unix/sysv/linux/semtimedop.c b/sysdeps/unix/sysv/linux/semtimedop.c
> index a9ad922ee2..29647f8ccd 100644
> --- a/sysdeps/unix/sysv/linux/semtimedop.c
> +++ b/sysdeps/unix/sysv/linux/semtimedop.c
> @@ -26,11 +26,15 @@ int
>  __semtimedop64 (int semid, struct sembuf *sops, size_t nsops,
>                 const struct __timespec64 *timeout)
>  {
> -#ifndef __NR_semtimedop_time64
> -# define __NR_semtimedop_time64 __NR_semtimedop
> +  int r;
> +#if defined __NR_semtimedop_time64
> +  r = INLINE_SYSCALL_CALL (semtimedop_time64, semid, sops, nsops, timeout);
> +#elif defined __ASSUME_DIRECT_SYSVIPC_SYSCALLS && defined __NR_semtimedop
> +  r = INLINE_SYSCALL_CALL (semtimedop, semid, sops, nsops, timeout);
> +#else
> +  r = INLINE_SYSCALL_CALL (ipc, IPCOP_semtimedop, semid,
> +                          SEMTIMEDOP_IPC_ARGS (nsops, sops, timeout));
>  #endif
> -  int r = INLINE_SYSCALL_CALL (semtimedop_time64, semid, sops, nsops,
> -                              timeout);
>  
>  #ifndef __ASSUME_TIME64_SYSCALLS
>    if (r == 0 || errno != ENOSYS)
> 

The change looks good and fixes the issue. Tested on ppc64le.

Reviewed-by: Matheus Castanho <msc@linux.ibm.com>

--
Matheus Castanho