Message ID | 6f370467-8d0a-bd3d-29b2-2fefef4e475f@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | semtimedop, powerpc, time64 and older kernels | expand |
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
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)