| Message ID | 20200707150827.20899-1-lukma@denx.de |
|---|---|
| Headers | show |
| Series | y2038: nptl: futex: Provide support for futex_time64 | expand |
Dear Community, > Please find this early RFC for converting 'futex' syscall based code > (pthreads/nptl/sem/gai) to support 64 bit time. > When applicable the futex_time64 syscall is used. > > The main purpose of this RFC is to assess if taken approach for > conversion is correct and acceptable by the glibc community. > > Quesitons/issues: > > 1. This whole patch set shall be squashed into a single patch, > otherwise, the glibc will not build between separate commits. I've > divided it to separate patches on the purpose - to facilitate review. > > 2. Question about rewritting lll_* macros in lowlevel-*.h - I'm > wondering if there is maybe a better way to do it. Please pay > attention to the *_4 suffix. > > 3. What would be the best point in the glibc release cycle to apply > this patch set as it convets the core functionality of the library? > > Just after the stable release? Any feedback on this RFC patch series? Comments are more than welcome. > > > Lukasz Majewski (10): > doc: Fix wording and formatting in ./support/README > y2038: Convert timespec_* from posix-timer.h to be Y2038 safe > y2038: Convert __lll_futex* functions to use futex_time64 when > available > y2038: Replace struct timespec with __timespec64 in futex-internal.h > y2038: Convert pthread_* functions to support 64 bit time > y2038: Convert sem_{timed|clock}wait to support 64 bit timeout > y2038: Convert __lll_clocklock_wait function to support 64 bit time > y2038: Convert aio_suspend to support 64 bit timeout > y2038: Convert gai_suspend to support 64 bit timeout > y2038: x86: Fix __lll_clocklock_elision to support 64 bit time > > nptl/lll_timedlock_wait.c | 6 +- > nptl/pthreadP.h | 53 +++++++++++++- > nptl/pthread_clockjoin.c | 22 +++++- > nptl/pthread_cond_wait.c | 46 ++++++++++-- > nptl/pthread_join_common.c | 11 +-- > nptl/pthread_mutex_timedlock.c | 39 +++++++--- > nptl/pthread_rwlock_clockrdlock.c | 21 +++++- > nptl/pthread_rwlock_clockwrlock.c | 21 +++++- > nptl/pthread_rwlock_common.c | 4 +- > nptl/pthread_rwlock_timedrdlock.c | 21 +++++- > nptl/pthread_rwlock_timedwrlock.c | 21 +++++- > nptl/pthread_timedjoin.c | 20 +++++- > nptl/sem_clockwait.c | 21 +++++- > nptl/sem_timedwait.c | 18 ++++- > nptl/sem_waitcommon.c | 4 +- > nptl/semaphoreP.h | 12 ++++ > resolv/gai_misc.h | 7 ++ > resolv/gai_suspend.c | 33 +++++++-- > support/README | 4 +- > sysdeps/nptl/aio_misc.h | 11 ++- > sysdeps/nptl/futex-internal.h | 10 +-- > sysdeps/nptl/lowlevellock-futex.h | 80 > +++++++++++++++++++-- sysdeps/nptl/lowlevellock.h | > 2 +- sysdeps/pthread/aio_suspend.c | 36 ++++++++-- > sysdeps/pthread/posix-timer.h | 11 +-- > sysdeps/unix/sysv/linux/x86/elision-timed.c | 2 +- > sysdeps/unix/sysv/linux/x86/lowlevellock.h | 2 +- > 27 files changed, 462 insertions(+), 76 deletions(-) > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 07/07/2020 12:08, Lukasz Majewski wrote: > Please find this early RFC for converting 'futex' syscall based code > (pthreads/nptl/sem/gai) to support 64 bit time. > When applicable the futex_time64 syscall is used. > > The main purpose of this RFC is to assess if taken approach for conversion is > correct and acceptable by the glibc community. > > Quesitons/issues: > > 1. This whole patch set shall be squashed into a single patch, otherwise, the > glibc will not build between separate commits. I've divided it to separate > patches on the purpose - to facilitate review. Another possibility could to work by adjusting each symbol and the required futex_* / lll_lock machinery instead. For instance, add 64-bit time_t support pthread_mutex_{timed,clock}lock, which in turn required to adjust futex_lock_pi/lll_futex_timed_wait/lll_futex_clock_wait_bitset. In this way we can tests the change better since they are incremental. > > 2. Question about rewritting lll_* macros in lowlevel-*.h - I'm wondering if > there is maybe a better way to do it. Please pay attention to the *_4 suffix. For lll_* I really think we should convert them to proper inline function instead, the required code change to adjust the macro is becoming really convoluted. I can help you on refactoring to code so the time64 changes should be simpler. Also, futex is a syscall used extensively and I think we should optimize the fallback code to avoid issue the 64-bit time one if the kernel does not support it (as we do for clock_gettime). I have send a patchset with some y2038 fixes and I added a generic support to simplify it [1]. We will probably need some adjustments to make it work on libpthread. [1] https://sourceware.org/pipermail/libc-alpha/2020-July/116259.html > > 3. What would be the best point in the glibc release cycle to apply this patch > set as it convets the core functionality of the library? > > Just after the stable release? I think it is late for 2.32, we should postpone it to 2.33.
Hi Adhemerval, > On 07/07/2020 12:08, Lukasz Majewski wrote: > > Please find this early RFC for converting 'futex' syscall based code > > (pthreads/nptl/sem/gai) to support 64 bit time. > > When applicable the futex_time64 syscall is used. > > > > The main purpose of this RFC is to assess if taken approach for > > conversion is correct and acceptable by the glibc community. > > > > Quesitons/issues: > > > > 1. This whole patch set shall be squashed into a single patch, > > otherwise, the glibc will not build between separate commits. I've > > divided it to separate patches on the purpose - to facilitate > > review. > > Another possibility could to work by adjusting each symbol and the > required futex_* / lll_lock machinery instead. For instance, add > 64-bit time_t support pthread_mutex_{timed,clock}lock, which in turn > required to adjust > futex_lock_pi/lll_futex_timed_wait/lll_futex_clock_wait_bitset. I've looked for such possibility, but the real issue IMHO is the need to convert struct timespec to struct __timespec64 in functions definitions/declarations. In the end you would need to convert lll_futex_syscall() which need to support __ASSUME_TIME64_SYSCALLS and futex_time64. After this you are forced to convert all other pthread syscalls, which use timeout argument. > > In this way we can tests the change better since they are incremental. Please see above comment - lowlevellock-futex.h written with C preprocessor macros is the real issue here. > > > > > 2. Question about rewritting lll_* macros in lowlevel-*.h - I'm > > wondering if there is maybe a better way to do it. Please pay > > attention to the *_4 suffix. > > For lll_* I really think we should convert them to proper inline > function instead, the required code change to adjust the macro is > becoming really convoluted. I fully agree here - the code as proposed[1] - is rather not clean and safe. > I can help you on refactoring to code so > the time64 changes should be simpler. Ok. Thanks. Now for instance we do have: __pthread_mutex_clocklock_common (funcion) (pthread_mutex_timedlock.c) | \|/ lll_timedwait (macro) __lll_clocklock | | \|/ \|/ __lll_clocklock_wait -> eligible for "conversion" Y2038 (function!) lll_futex_timed_wait -> (macro) lll_futex_syscall -> here is the call to futex syscall (macro) The code itself has many levels with inline functions convoluted with macros. > > Also, futex is a syscall used extensively and I think we should > optimize the fallback code to avoid issue the 64-bit time one if the > kernel does not support it (as we do for clock_gettime). I think that this is not the case for most systems. The 64 bit call for futex_time64 (and other 64 bit syscalls) were introduced in Linux 5.1 - which is now more than a year ago. The newest LTS Linux (5.4) now supports it - so it is likely that new BSPs will use it. Especially, yocto LTS - dunfell (3.1) supports by default 5.4 kernel. > > I have send a patchset with some y2038 fixes and I added a generic > support to simplify it [1]. We will probably need some adjustments to > make it work on libpthread. > I will share my comments on it in the patch itself. > [1] https://sourceware.org/pipermail/libc-alpha/2020-July/116259.html > > > > > 3. What would be the best point in the glibc release cycle to apply > > this patch set as it convets the core functionality of the library? > > > > Just after the stable release? > > I think it is late for 2.32, we should postpone it to 2.33. I'm fine with postponing it as long as some work will be done in parallel - the futex system call is crucial in many parts of glibc library. Sooner we convert it and pull patches into glibc master, then sooner it matures. Links: [1] -https://patchwork.ozlabs.org/project/glibc/patch/20200707150827.20899-4-lukma@denx.de/ Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 14/07/2020 04:56, Lukasz Majewski wrote: > Hi Adhemerval, > >> On 07/07/2020 12:08, Lukasz Majewski wrote: >>> Please find this early RFC for converting 'futex' syscall based code >>> (pthreads/nptl/sem/gai) to support 64 bit time. >>> When applicable the futex_time64 syscall is used. >>> >>> The main purpose of this RFC is to assess if taken approach for >>> conversion is correct and acceptable by the glibc community. >>> >>> Quesitons/issues: >>> >>> 1. This whole patch set shall be squashed into a single patch, >>> otherwise, the glibc will not build between separate commits. I've >>> divided it to separate patches on the purpose - to facilitate >>> review. >> >> Another possibility could to work by adjusting each symbol and the >> required futex_* / lll_lock machinery instead. For instance, add >> 64-bit time_t support pthread_mutex_{timed,clock}lock, which in turn >> required to adjust >> futex_lock_pi/lll_futex_timed_wait/lll_futex_clock_wait_bitset. > > I've looked for such possibility, but the real issue IMHO is the > need to convert struct timespec to struct __timespec64 in functions > definitions/declarations. > > In the end you would need to convert lll_futex_syscall() which need to > support __ASSUME_TIME64_SYSCALLS and futex_time64. > After this you are forced to convert all other pthread syscalls, which > use timeout argument. My idea would be to implement the internal pthread_mutex_{timed,clock}_time64 and make pthread_mutex_{timed,clock} call them. For the internal time64 variant the idea would to add new time64 helper functions, in this case futex_lock_pi_time64, lll_futex_timed_wait_time64, and lll_futex_clock_wait_bitset_time64. What I mean is we would incrementally add new functions without touching to old lll/futex definitions. Once we move all the internal implementation to time64 we can then remove them altogether. > >> >> In this way we can tests the change better since they are incremental. > > Please see above comment - lowlevellock-futex.h written with C > preprocessor macros is the real issue here. > >> >>> >>> 2. Question about rewritting lll_* macros in lowlevel-*.h - I'm >>> wondering if there is maybe a better way to do it. Please pay >>> attention to the *_4 suffix. >> >> For lll_* I really think we should convert them to proper inline >> function instead, the required code change to adjust the macro is >> becoming really convoluted. > > I fully agree here - the code as proposed[1] - is rather not clean and > safe. > >> I can help you on refactoring to code so >> the time64 changes should be simpler. > > Ok. Thanks. > > Now for instance we do have: > > __pthread_mutex_clocklock_common (funcion) (pthread_mutex_timedlock.c) > | > \|/ > lll_timedwait (macro) __lll_clocklock > | | > \|/ \|/ > __lll_clocklock_wait -> eligible for "conversion" Y2038 (function!) > lll_futex_timed_wait -> (macro) > lll_futex_syscall -> here is the call to futex syscall (macro) > > The code itself has many levels with inline functions convoluted with > macros. Yeah, this is messy indeed. In fact now that we don't have a NaCL port anymore I don't seem much point in the extra lll_* indirections. For instance, on pthread_mutex_{timed,clock} I think we can move and rename both time64 lll_futex_timed_wait and lll_futex_clock_wait_bitset to sysdeps/nptl/futex-internal.h and call INTERNAL_SYSCALL_* directly. Something like: static inline int futex_private_flag (int fl, int priv) { #if IS_IN (libc) || IS_IN (rtld) return fl | FUTEX_PRIVATE_FLAG; #else return (fl | FUTEX_PRIVATE_FLAG) ^ priv; #endif } static __always_inline int futex_reltimed_wait_time64 (unsigned int* futex_word, unsigned int expected, const struct __timespec64* reltime, int priv) { #ifndef __NR_futex_time64 # define __NR_futex_time64 __NR_futex #endif int r = INTERNAL_SYSCALL_CALL (futex, futex_word, futex_private_flag (FUTEX_WAIT, priv), expected, reltime); #ifndef __ASSUME_TIME64_SYSCALLS if (r == -ENOSYS) { struct timespec ts32, *pts32 = NULL; if (timeout != NULL) { if (! in_time_t_range (timeout->tv_sec)) return -EINVAL; ts32 = valid_timespec64_to_timespec (ts64); pts32 = &ts32; } r = INTERNAL_SYSCALL_CALL (futex, futex_word, futex_private_flag (FUTEX_WAIT, priv), expected, pts32); } #endif switch (r) { case 0: case -EAGAIN: case -EINTR: case -ETIMEDOUT: return -r; case -EFAULT: /* Must have been caused by a glibc or application bug. */ case -EINVAL: /* Either due to wrong alignment or due to the timeout not being normalized. Must have been caused by a glibc or application bug. */ case -ENOSYS: /* Must have been caused by a glibc bug. */ /* No other errors are documented at this time. */ default: futex_fatal_error (); } } > >> >> Also, futex is a syscall used extensively and I think we should >> optimize the fallback code to avoid issue the 64-bit time one if the >> kernel does not support it (as we do for clock_gettime). > > I think that this is not the case for most systems. > > The 64 bit call for futex_time64 (and other 64 bit syscalls) were > introduced in Linux 5.1 - which is now more than a year ago. > > The newest LTS Linux (5.4) now supports it - so it is likely that > new BSPs will use it. Especially, yocto LTS - dunfell (3.1) supports by > default 5.4 kernel. It really depends of the kind of deployment and time32 applications will stick for a while, so IMHO we need to not penalize them too much with the move to use the time64 syscalls as default. For riscv32 and other ABI wuth time64 as default ABI it should not be a problem, but at least for i686 I foresee it might indeed impact 32-bit application that relies heavily on libpthread (specially on patterns where locks are used for highly granulated contention). And this might happen most with virtualized environments where the host kernel is not updated as the client one (I think last conversation with Florian he say it is a common scenario for RHEL). In any case I think we can incrementally add such optimizations. > >> >> I have send a patchset with some y2038 fixes and I added a generic >> support to simplify it [1]. We will probably need some adjustments to >> make it work on libpthread. >> > > I will share my comments on it in the patch itself. > >> [1] https://sourceware.org/pipermail/libc-alpha/2020-July/116259.html Thanks. >> >>> >>> 3. What would be the best point in the glibc release cycle to apply >>> this patch set as it convets the core functionality of the library? >>> >>> Just after the stable release? >> >> I think it is late for 2.32, we should postpone it to 2.33. > > I'm fine with postponing it as long as some work will be done in > parallel - the futex system call is crucial in many parts of glibc > library. Sooner we convert it and pull patches into glibc master, then > sooner it matures. > > > Links: > [1] > -https://patchwork.ozlabs.org/project/glibc/patch/20200707150827.20899-4-lukma@denx.de/ > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de >
Hi Adhemerval, > On 14/07/2020 04:56, Lukasz Majewski wrote: > > Hi Adhemerval, > > > >> On 07/07/2020 12:08, Lukasz Majewski wrote: > >>> Please find this early RFC for converting 'futex' syscall based > >>> code (pthreads/nptl/sem/gai) to support 64 bit time. > >>> When applicable the futex_time64 syscall is used. > >>> > >>> The main purpose of this RFC is to assess if taken approach for > >>> conversion is correct and acceptable by the glibc community. > >>> > >>> Quesitons/issues: > >>> > >>> 1. This whole patch set shall be squashed into a single patch, > >>> otherwise, the glibc will not build between separate commits. I've > >>> divided it to separate patches on the purpose - to facilitate > >>> review. > >> > >> Another possibility could to work by adjusting each symbol and the > >> required futex_* / lll_lock machinery instead. For instance, add > >> 64-bit time_t support pthread_mutex_{timed,clock}lock, which in > >> turn required to adjust > >> futex_lock_pi/lll_futex_timed_wait/lll_futex_clock_wait_bitset. > > > > I've looked for such possibility, but the real issue IMHO is the > > need to convert struct timespec to struct __timespec64 in functions > > definitions/declarations. > > > > In the end you would need to convert lll_futex_syscall() which need > > to support __ASSUME_TIME64_SYSCALLS and futex_time64. > > After this you are forced to convert all other pthread syscalls, > > which use timeout argument. > > My idea would be to implement the internal > pthread_mutex_{timed,clock}_time64 and make > pthread_mutex_{timed,clock} call them. Let's consider __pthread_mutex_timedlock() from ./nptl/pthread_mutex_timedlock.c Its conversion to 64 bit time has been proposed in RFC: https://patchwork.ozlabs.org/project/glibc/patch/20200707150827.20899-6-lukma@denx.de/ > For the internal time64 > variant the idea would to add new time64 helper functions, in this > case futex_lock_pi_time64, lll_futex_timed_wait_time64, and > lll_futex_clock_wait_bitset_time64. I've written some comments down below regarding this issue. > > What I mean is we would incrementally add new functions without > touching to old lll/futex definitions. Once we move all the internal > implementation to time64 we can then remove them altogether.\ My concern is that we will introduce "helper" functions (with 64 or time64 suffix) for internal functions - a lot of new code in the situation where we shall use __timespe64 anyway as an internal glibc representation of time. > > > > >> > >> In this way we can tests the change better since they are > >> incremental. > > > > Please see above comment - lowlevellock-futex.h written with C > > preprocessor macros is the real issue here. > > > >> > >>> > >>> 2. Question about rewritting lll_* macros in lowlevel-*.h - I'm > >>> wondering if there is maybe a better way to do it. Please pay > >>> attention to the *_4 suffix. > >> > >> For lll_* I really think we should convert them to proper inline > >> function instead, the required code change to adjust the macro is > >> becoming really convoluted. > > > > I fully agree here - the code as proposed[1] - is rather not clean > > and safe. > > > >> I can help you on refactoring to code so > >> the time64 changes should be simpler. > > > > Ok. Thanks. > > > > Now for instance we do have: > > > > __pthread_mutex_clocklock_common (funcion) > > (pthread_mutex_timedlock.c) | > > \|/ > > lll_timedwait (macro) __lll_clocklock > > | | > > \|/ \|/ > > __lll_clocklock_wait -> eligible for "conversion" Y2038 > > (function!) lll_futex_timed_wait -> (macro) > > lll_futex_syscall -> here is the call to futex syscall (macro) > > > > The code itself has many levels with inline functions convoluted > > with macros. > > Yeah, this is messy indeed. In fact now that we don't have a NaCL > port anymore I don't seem much point in the extra lll_* indirections. Just wondering - what was the rationale for extra lll_* indirection for the NaCL port? > > For instance, on pthread_mutex_{timed,clock} I think we can move and > rename both time64 lll_futex_timed_wait and > lll_futex_clock_wait_bitset to sysdeps/nptl/futex-internal.h and call > INTERNAL_SYSCALL_* directly. > > Something like: > > static inline int > futex_private_flag (int fl, int priv) > { > #if IS_IN (libc) || IS_IN (rtld) > return fl | FUTEX_PRIVATE_FLAG; > #else > return (fl | FUTEX_PRIVATE_FLAG) ^ priv; > #endif > } > > static __always_inline int > futex_reltimed_wait_time64 (unsigned int* futex_word, unsigned int > expected, const struct __timespec64* reltime, int priv) > { > #ifndef __NR_futex_time64 > # define __NR_futex_time64 __NR_futex > #endif > int r = INTERNAL_SYSCALL_CALL (futex, futex_word, > futex_private_flag (FUTEX_WAIT, > priv), expected, reltime); > #ifndef __ASSUME_TIME64_SYSCALLS > if (r == -ENOSYS) > { > struct timespec ts32, *pts32 = NULL; > if (timeout != NULL) > { > if (! in_time_t_range (timeout->tv_sec)) > return -EINVAL; > ts32 = valid_timespec64_to_timespec (ts64); > pts32 = &ts32; > } > > r = INTERNAL_SYSCALL_CALL (futex, futex_word, > futex_private_flag (FUTEX_WAIT, > priv), expected, pts32); > } > #endif > > switch (r) > { > case 0: > case -EAGAIN: > case -EINTR: > case -ETIMEDOUT: > return -r; > > case -EFAULT: /* Must have been caused by a glibc or > application bug. */ case -EINVAL: /* Either due to wrong alignment > or due to the timeout not being normalized. Must have been caused by > a glibc or application bug. */ > case -ENOSYS: /* Must have been caused by a glibc bug. */ > /* No other errors are documented at this time. */ > default: > futex_fatal_error (); > } > } If having the invocations to futex and futex_time64 syscalls is not the problem in many places - like futex-internal.h and lowlevel-futex.h and also if removing the lll_* indirection is welcome, then I'm fine with it. With the above patch we can also rename struct timespec to __timespec64 for eligible functions - like futex_lock_pi64. Will you have time to prepare the cleanup patch for lowlevellock-futex.h in the near future? Or maybe to prepare the patch with the code you wrote above? > > > > >> > >> Also, futex is a syscall used extensively and I think we should > >> optimize the fallback code to avoid issue the 64-bit time one if > >> the kernel does not support it (as we do for clock_gettime). > > > > I think that this is not the case for most systems. > > > > The 64 bit call for futex_time64 (and other 64 bit syscalls) were > > introduced in Linux 5.1 - which is now more than a year ago. > > > > The newest LTS Linux (5.4) now supports it - so it is likely that > > new BSPs will use it. Especially, yocto LTS - dunfell (3.1) > > supports by default 5.4 kernel. > > It really depends of the kind of deployment and time32 applications > will stick for a while, so IMHO we need to not penalize them too much > with the move to use the time64 syscalls as default. I'm wondering what are the numbers - i.e. what is the exact penalty for this? > > For riscv32 and other ABI wuth time64 as default ABI it should not be > a problem, Yes. I do agree. It is also not a problem for new ports for ARM. > but at least for i686 I foresee it might indeed impact > 32-bit application that relies heavily on libpthread (specially on > patterns where locks are used for highly granulated contention). And > this might happen most with virtualized environments where the host > kernel is not updated as the client one (I think last conversation > with Florian he say it is a common scenario for RHEL). Could you elaborate on this? Please correct me if I'm wrong, but if the client is updated to kernel 5.1+ and recent glibc, then it shall support 64 bit time no matter how old is the host VM system. Problem with performance - with missing syscalls - would start when the old kernel is left in place and only glibc with rootfs is updated. > > In any case I think we can incrementally add such optimizations. Yes, if it solves the real issue. > > > > >> > >> I have send a patchset with some y2038 fixes and I added a generic > >> support to simplify it [1]. We will probably need some adjustments > >> to make it work on libpthread. > >> > > > > I will share my comments on it in the patch itself. > > > >> [1] > >> https://sourceware.org/pipermail/libc-alpha/2020-July/116259.html > > Thanks. > > >> > >>> > >>> 3. What would be the best point in the glibc release cycle to > >>> apply this patch set as it convets the core functionality of the > >>> library? > >>> > >>> Just after the stable release? > >> > >> I think it is late for 2.32, we should postpone it to 2.33. > > > > I'm fine with postponing it as long as some work will be done in > > parallel - the futex system call is crucial in many parts of glibc > > library. Sooner we convert it and pull patches into glibc master, > > then sooner it matures. > > > > > > Links: > > [1] > > -https://patchwork.ozlabs.org/project/glibc/patch/20200707150827.20899-4-lukma@denx.de/ > > > > > > Best regards, > > > > Lukasz Majewski > > > > -- > > > > DENX Software Engineering GmbH, Managing Director: Wolfgang > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > > lukma@denx.de > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 15/07/2020 05:47, Lukasz Majewski wrote: > Hi Adhemerval, > >> On 14/07/2020 04:56, Lukasz Majewski wrote: >>> Hi Adhemerval, >>> >>>> On 07/07/2020 12:08, Lukasz Majewski wrote: >>>>> Please find this early RFC for converting 'futex' syscall based >>>>> code (pthreads/nptl/sem/gai) to support 64 bit time. >>>>> When applicable the futex_time64 syscall is used. >>>>> >>>>> The main purpose of this RFC is to assess if taken approach for >>>>> conversion is correct and acceptable by the glibc community. >>>>> >>>>> Quesitons/issues: >>>>> >>>>> 1. This whole patch set shall be squashed into a single patch, >>>>> otherwise, the glibc will not build between separate commits. I've >>>>> divided it to separate patches on the purpose - to facilitate >>>>> review. >>>> >>>> Another possibility could to work by adjusting each symbol and the >>>> required futex_* / lll_lock machinery instead. For instance, add >>>> 64-bit time_t support pthread_mutex_{timed,clock}lock, which in >>>> turn required to adjust >>>> futex_lock_pi/lll_futex_timed_wait/lll_futex_clock_wait_bitset. >>> >>> I've looked for such possibility, but the real issue IMHO is the >>> need to convert struct timespec to struct __timespec64 in functions >>> definitions/declarations. >>> >>> In the end you would need to convert lll_futex_syscall() which need >>> to support __ASSUME_TIME64_SYSCALLS and futex_time64. >>> After this you are forced to convert all other pthread syscalls, >>> which use timeout argument. >> >> My idea would be to implement the internal >> pthread_mutex_{timed,clock}_time64 and make >> pthread_mutex_{timed,clock} call them. > > Let's consider __pthread_mutex_timedlock() from > ./nptl/pthread_mutex_timedlock.c > > Its conversion to 64 bit time has been proposed in RFC: > https://patchwork.ozlabs.org/project/glibc/patch/20200707150827.20899-6-lukma@denx.de/ > >> For the internal time64 >> variant the idea would to add new time64 helper functions, in this >> case futex_lock_pi_time64, lll_futex_timed_wait_time64, and >> lll_futex_clock_wait_bitset_time64. > > I've written some comments down below regarding this issue. > >> >> What I mean is we would incrementally add new functions without >> touching to old lll/futex definitions. Once we move all the internal >> implementation to time64 we can then remove them altogether.\ > > My concern is that we will introduce "helper" functions (with 64 or > time64 suffix) for internal functions - a lot of new code in the > situation where we shall use __timespe64 anyway as an internal glibc > representation of time. Indeed it will add some extra code to handle the conversion to and from the 32-bit time to 64-bit time types, but once all the affected function are converted we can cleanup and consolidate the unused code (as you did for alpha for instance). > >> >>> >>>> >>>> In this way we can tests the change better since they are >>>> incremental. >>> >>> Please see above comment - lowlevellock-futex.h written with C >>> preprocessor macros is the real issue here. >>> >>>> >>>>> >>>>> 2. Question about rewritting lll_* macros in lowlevel-*.h - I'm >>>>> wondering if there is maybe a better way to do it. Please pay >>>>> attention to the *_4 suffix. >>>> >>>> For lll_* I really think we should convert them to proper inline >>>> function instead, the required code change to adjust the macro is >>>> becoming really convoluted. >>> >>> I fully agree here - the code as proposed[1] - is rather not clean >>> and safe. >>> >>>> I can help you on refactoring to code so >>>> the time64 changes should be simpler. >>> >>> Ok. Thanks. >>> >>> Now for instance we do have: >>> >>> __pthread_mutex_clocklock_common (funcion) >>> (pthread_mutex_timedlock.c) | >>> \|/ >>> lll_timedwait (macro) __lll_clocklock >>> | | >>> \|/ \|/ >>> __lll_clocklock_wait -> eligible for "conversion" Y2038 >>> (function!) lll_futex_timed_wait -> (macro) >>> lll_futex_syscall -> here is the call to futex syscall (macro) >>> >>> The code itself has many levels with inline functions convoluted >>> with macros. >> >> Yeah, this is messy indeed. In fact now that we don't have a NaCL >> port anymore I don't seem much point in the extra lll_* indirections. > > Just wondering - what was the rationale for extra lll_* indirection for > the NaCL port? Afaik NaCL was lousily based on Linux kABI, but each syscall and atomic operation (and thus all lll_* code) was in fact a libcall to the NaCL runtime. > >> >> For instance, on pthread_mutex_{timed,clock} I think we can move and >> rename both time64 lll_futex_timed_wait and >> lll_futex_clock_wait_bitset to sysdeps/nptl/futex-internal.h and call >> INTERNAL_SYSCALL_* directly. >> >> Something like: >> >> static inline int >> futex_private_flag (int fl, int priv) >> { >> #if IS_IN (libc) || IS_IN (rtld) >> return fl | FUTEX_PRIVATE_FLAG; >> #else >> return (fl | FUTEX_PRIVATE_FLAG) ^ priv; >> #endif >> } >> >> static __always_inline int >> futex_reltimed_wait_time64 (unsigned int* futex_word, unsigned int >> expected, const struct __timespec64* reltime, int priv) >> { >> #ifndef __NR_futex_time64 >> # define __NR_futex_time64 __NR_futex >> #endif >> int r = INTERNAL_SYSCALL_CALL (futex, futex_word, >> futex_private_flag (FUTEX_WAIT, >> priv), expected, reltime); >> #ifndef __ASSUME_TIME64_SYSCALLS >> if (r == -ENOSYS) >> { >> struct timespec ts32, *pts32 = NULL; >> if (timeout != NULL) >> { >> if (! in_time_t_range (timeout->tv_sec)) >> return -EINVAL; >> ts32 = valid_timespec64_to_timespec (ts64); >> pts32 = &ts32; >> } >> >> r = INTERNAL_SYSCALL_CALL (futex, futex_word, >> futex_private_flag (FUTEX_WAIT, >> priv), expected, pts32); >> } >> #endif >> >> switch (r) >> { >> case 0: >> case -EAGAIN: >> case -EINTR: >> case -ETIMEDOUT: >> return -r; >> >> case -EFAULT: /* Must have been caused by a glibc or >> application bug. */ case -EINVAL: /* Either due to wrong alignment >> or due to the timeout not being normalized. Must have been caused by >> a glibc or application bug. */ >> case -ENOSYS: /* Must have been caused by a glibc bug. */ >> /* No other errors are documented at this time. */ >> default: >> futex_fatal_error (); >> } >> } > > If having the invocations to futex and futex_time64 syscalls is not the > problem in many places - like futex-internal.h and lowlevel-futex.h and > also if removing the lll_* indirection is welcome, then I'm fine with > it. > With the above patch we can also rename struct timespec to __timespec64 > for eligible functions - like futex_lock_pi64. > > > Will you have time to prepare the cleanup patch for > lowlevellock-futex.h in the near future? Yes, it is on my list. I am trying to handle the mess of stat functions before starting on the lowlevellock-futex.h. > > Or maybe to prepare the patch with the code you wrote above? > >> >>> >>>> >>>> Also, futex is a syscall used extensively and I think we should >>>> optimize the fallback code to avoid issue the 64-bit time one if >>>> the kernel does not support it (as we do for clock_gettime). >>> >>> I think that this is not the case for most systems. >>> >>> The 64 bit call for futex_time64 (and other 64 bit syscalls) were >>> introduced in Linux 5.1 - which is now more than a year ago. >>> >>> The newest LTS Linux (5.4) now supports it - so it is likely that >>> new BSPs will use it. Especially, yocto LTS - dunfell (3.1) >>> supports by default 5.4 kernel. >> >> It really depends of the kind of deployment and time32 applications >> will stick for a while, so IMHO we need to not penalize them too much >> with the move to use the time64 syscalls as default. > > I'm wondering what are the numbers - i.e. what is the exact penalty for > this? It is mainly trading a ENOSYS syscall error, which would be cheaper in terms of syscall operation but even though it has all the syscall extra overhead; with a relaxed atomic operation. I think it is a good tradeoff. > >> >> For riscv32 and other ABI wuth time64 as default ABI it should not be >> a problem, > > Yes. I do agree. It is also not a problem for new ports for ARM. > >> but at least for i686 I foresee it might indeed impact >> 32-bit application that relies heavily on libpthread (specially on >> patterns where locks are used for highly granulated contention). And >> this might happen most with virtualized environments where the host >> kernel is not updated as the client one (I think last conversation >> with Florian he say it is a common scenario for RHEL). > > Could you elaborate on this? > > Please correct me if I'm wrong, but if the client is updated to kernel > 5.1+ and recent glibc, then it shall support 64 bit time no matter how > old is the host VM system. > > Problem with performance - with missing syscalls - would start when the > old kernel is left in place and only glibc with rootfs is updated. I am trying to recall which was the original issue, it was brought when we discussed raising the minimal supported kernel to 3.2. But I do agree with you that when time is applicable this issue should be observable and I am just including it on this patchset because I see that the code complexity and runtime overhead is quite small. > >> >> In any case I think we can incrementally add such optimizations. > > Yes, if it solves the real issue. > >> >>> >>>> >>>> I have send a patchset with some y2038 fixes and I added a generic >>>> support to simplify it [1]. We will probably need some adjustments >>>> to make it work on libpthread. >>>> >>> >>> I will share my comments on it in the patch itself. >>> >>>> [1] >>>> https://sourceware.org/pipermail/libc-alpha/2020-July/116259.html >> >> Thanks. >> >>>> >>>>> >>>>> 3. What would be the best point in the glibc release cycle to >>>>> apply this patch set as it convets the core functionality of the >>>>> library? >>>>> >>>>> Just after the stable release? >>>> >>>> I think it is late for 2.32, we should postpone it to 2.33. >>> >>> I'm fine with postponing it as long as some work will be done in >>> parallel - the futex system call is crucial in many parts of glibc >>> library. Sooner we convert it and pull patches into glibc master, >>> then sooner it matures. >>> >>> >>> Links: >>> [1] >>> -https://patchwork.ozlabs.org/project/glibc/patch/20200707150827.20899-4-lukma@denx.de/ >>> >>> >>> Best regards, >>> >>> Lukasz Majewski >>> >>> -- >>> >>> DENX Software Engineering GmbH, Managing Director: Wolfgang >>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, >>> Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: >>> lukma@denx.de >> > > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de >
Hi Adhemerval, > On 15/07/2020 05:47, Lukasz Majewski wrote: > > Hi Adhemerval, > > > >> On 14/07/2020 04:56, Lukasz Majewski wrote: > >>> Hi Adhemerval, > >>> > >>>> On 07/07/2020 12:08, Lukasz Majewski wrote: > >>>>> Please find this early RFC for converting 'futex' syscall based > >>>>> code (pthreads/nptl/sem/gai) to support 64 bit time. > >>>>> When applicable the futex_time64 syscall is used. > >>>>> > >>>>> The main purpose of this RFC is to assess if taken approach for > >>>>> conversion is correct and acceptable by the glibc community. > >>>>> > >>>>> Quesitons/issues: > >>>>> > >>>>> 1. This whole patch set shall be squashed into a single patch, > >>>>> otherwise, the glibc will not build between separate commits. > >>>>> I've divided it to separate patches on the purpose - to > >>>>> facilitate review. > >>>> > >>>> Another possibility could to work by adjusting each symbol and > >>>> the required futex_* / lll_lock machinery instead. For > >>>> instance, add 64-bit time_t support > >>>> pthread_mutex_{timed,clock}lock, which in turn required to adjust > >>>> futex_lock_pi/lll_futex_timed_wait/lll_futex_clock_wait_bitset. > >>>> > >>> > >>> I've looked for such possibility, but the real issue IMHO is the > >>> need to convert struct timespec to struct __timespec64 in > >>> functions definitions/declarations. > >>> > >>> In the end you would need to convert lll_futex_syscall() which > >>> need to support __ASSUME_TIME64_SYSCALLS and futex_time64. > >>> After this you are forced to convert all other pthread syscalls, > >>> which use timeout argument. > >> > >> My idea would be to implement the internal > >> pthread_mutex_{timed,clock}_time64 and make > >> pthread_mutex_{timed,clock} call them. > > > > Let's consider __pthread_mutex_timedlock() from > > ./nptl/pthread_mutex_timedlock.c > > > > Its conversion to 64 bit time has been proposed in RFC: > > https://patchwork.ozlabs.org/project/glibc/patch/20200707150827.20899-6-lukma@denx.de/ > > > >> For the internal time64 > >> variant the idea would to add new time64 helper functions, in this > >> case futex_lock_pi_time64, lll_futex_timed_wait_time64, and > >> lll_futex_clock_wait_bitset_time64. > > > > I've written some comments down below regarding this issue. > > > >> > >> What I mean is we would incrementally add new functions without > >> touching to old lll/futex definitions. Once we move all the > >> internal implementation to time64 we can then remove them > >> altogether.\ > > > > My concern is that we will introduce "helper" functions (with 64 or > > time64 suffix) for internal functions - a lot of new code in the > > situation where we shall use __timespe64 anyway as an internal glibc > > representation of time. > > Indeed it will add some extra code to handle the conversion to and > from the 32-bit time to 64-bit time types, but once all the affected > function are converted we can cleanup and consolidate the unused code > (as you did for alpha for instance). > > > > >> > >>> > >>>> > >>>> In this way we can tests the change better since they are > >>>> incremental. > >>> > >>> Please see above comment - lowlevellock-futex.h written with C > >>> preprocessor macros is the real issue here. > >>> > >>>> > >>>>> > >>>>> 2. Question about rewritting lll_* macros in lowlevel-*.h - I'm > >>>>> wondering if there is maybe a better way to do it. Please pay > >>>>> attention to the *_4 suffix. > >>>> > >>>> For lll_* I really think we should convert them to proper inline > >>>> function instead, the required code change to adjust the macro is > >>>> becoming really convoluted. > >>> > >>> I fully agree here - the code as proposed[1] - is rather not clean > >>> and safe. > >>> > >>>> I can help you on refactoring to code so > >>>> the time64 changes should be simpler. > >>> > >>> Ok. Thanks. > >>> > >>> Now for instance we do have: > >>> > >>> __pthread_mutex_clocklock_common (funcion) > >>> (pthread_mutex_timedlock.c) | > >>> \|/ > >>> lll_timedwait (macro) __lll_clocklock > >>> | | > >>> \|/ \|/ > >>> __lll_clocklock_wait -> eligible for "conversion" Y2038 > >>> (function!) lll_futex_timed_wait -> (macro) > >>> lll_futex_syscall -> here is the call to futex syscall > >>> (macro) > >>> > >>> The code itself has many levels with inline functions convoluted > >>> with macros. > >> > >> Yeah, this is messy indeed. In fact now that we don't have a NaCL > >> port anymore I don't seem much point in the extra lll_* > >> indirections. > > > > Just wondering - what was the rationale for extra lll_* indirection > > for the NaCL port? > > Afaik NaCL was lousily based on Linux kABI, but each syscall and > atomic operation (and thus all lll_* code) was in fact a libcall to > the NaCL runtime. > > > > >> > >> For instance, on pthread_mutex_{timed,clock} I think we can move > >> and rename both time64 lll_futex_timed_wait and > >> lll_futex_clock_wait_bitset to sysdeps/nptl/futex-internal.h and > >> call INTERNAL_SYSCALL_* directly. > >> > >> Something like: > >> > >> static inline int > >> futex_private_flag (int fl, int priv) > >> { > >> #if IS_IN (libc) || IS_IN (rtld) > >> return fl | FUTEX_PRIVATE_FLAG; > >> #else > >> return (fl | FUTEX_PRIVATE_FLAG) ^ priv; > >> #endif > >> } > >> > >> static __always_inline int > >> futex_reltimed_wait_time64 (unsigned int* futex_word, unsigned > >> int expected, const struct __timespec64* reltime, int priv) > >> { > >> #ifndef __NR_futex_time64 > >> # define __NR_futex_time64 __NR_futex > >> #endif > >> int r = INTERNAL_SYSCALL_CALL (futex, futex_word, > >> futex_private_flag (FUTEX_WAIT, > >> priv), expected, reltime); > >> #ifndef __ASSUME_TIME64_SYSCALLS > >> if (r == -ENOSYS) > >> { > >> struct timespec ts32, *pts32 = NULL; > >> if (timeout != NULL) > >> { > >> if (! in_time_t_range (timeout->tv_sec)) > >> return -EINVAL; > >> ts32 = valid_timespec64_to_timespec (ts64); > >> pts32 = &ts32; > >> } > >> > >> r = INTERNAL_SYSCALL_CALL (futex, futex_word, > >> futex_private_flag (FUTEX_WAIT, > >> priv), expected, pts32); > >> } > >> #endif > >> > >> switch (r) > >> { > >> case 0: > >> case -EAGAIN: > >> case -EINTR: > >> case -ETIMEDOUT: > >> return -r; > >> > >> case -EFAULT: /* Must have been caused by a glibc or > >> application bug. */ case -EINVAL: /* Either due to wrong alignment > >> or due to the timeout not being normalized. Must have been caused > >> by a glibc or application bug. */ > >> case -ENOSYS: /* Must have been caused by a glibc bug. */ > >> /* No other errors are documented at this time. */ > >> default: > >> futex_fatal_error (); > >> } > >> } > > > > If having the invocations to futex and futex_time64 syscalls is not > > the problem in many places - like futex-internal.h and > > lowlevel-futex.h and also if removing the lll_* indirection is > > welcome, then I'm fine with it. > > With the above patch we can also rename struct timespec to > > __timespec64 for eligible functions - like futex_lock_pi64. > > > > > > Will you have time to prepare the cleanup patch for > > lowlevellock-futex.h in the near future? > > Yes, it is on my list. I am trying to handle the mess of stat > functions Regarding stat - it is indeed a mess - many versions of the same functions for generic, LFS, and architectures (like ARM). Conversion to 64 bit time support is challenging there as well. But as fair as I can tell - more than "code base" cleanup - the raise of glibc minimal kernel supported version would help much more in this situation as code for many "use cases" would be just removed. A side remark - now the oldest LTS kernel supported is 4.4 and oldest supported kernel for glibc is 3.2. Maybe it is a good time to bump the minimal supported kernel for glibc? > before starting on the lowlevellock-futex.h. If you don't have time to do the conversion in next two weeks then - I can in the meantime do a conversion for the above code snippet - futex_reltimed_wait_time64() (and up to pthread_mutex_{timed,clock}). This work shall be orthogonal to lowlevellock-futex.h cleanup. What do you think? > > > > > Or maybe to prepare the patch with the code you wrote above? > > > >> > >>> > >>>> > >>>> Also, futex is a syscall used extensively and I think we should > >>>> optimize the fallback code to avoid issue the 64-bit time one if > >>>> the kernel does not support it (as we do for clock_gettime). > >>> > >>> I think that this is not the case for most systems. > >>> > >>> The 64 bit call for futex_time64 (and other 64 bit syscalls) were > >>> introduced in Linux 5.1 - which is now more than a year ago. > >>> > >>> The newest LTS Linux (5.4) now supports it - so it is likely that > >>> new BSPs will use it. Especially, yocto LTS - dunfell (3.1) > >>> supports by default 5.4 kernel. > >> > >> It really depends of the kind of deployment and time32 applications > >> will stick for a while, so IMHO we need to not penalize them too > >> much with the move to use the time64 syscalls as default. > > > > I'm wondering what are the numbers - i.e. what is the exact penalty > > for this? > > It is mainly trading a ENOSYS syscall error, which would be cheaper in > terms of syscall operation but even though it has all the syscall > extra overhead; with a relaxed atomic operation. I think it is a good > tradeoff. Yes, I do agree. > > > > >> > >> For riscv32 and other ABI wuth time64 as default ABI it should not > >> be a problem, > > > > Yes. I do agree. It is also not a problem for new ports for ARM. > > > >> but at least for i686 I foresee it might indeed impact > >> 32-bit application that relies heavily on libpthread (specially on > >> patterns where locks are used for highly granulated contention). > >> And this might happen most with virtualized environments where the > >> host kernel is not updated as the client one (I think last > >> conversation with Florian he say it is a common scenario for > >> RHEL). > > > > Could you elaborate on this? > > > > Please correct me if I'm wrong, but if the client is updated to > > kernel 5.1+ and recent glibc, then it shall support 64 bit time no > > matter how old is the host VM system. > > > > Problem with performance - with missing syscalls - would start when > > the old kernel is left in place and only glibc with rootfs is > > updated. > > I am trying to recall which was the original issue, it was brought > when we discussed raising the minimal supported kernel to 3.2. But I > do agree with you that when time is applicable this issue should be > observable and I am just including it on this patchset because I see > that the code complexity and runtime overhead is quite small. Ok. > > > > >> > >> In any case I think we can incrementally add such optimizations. > > > > Yes, if it solves the real issue. > > > >> > >>> > >>>> > >>>> I have send a patchset with some y2038 fixes and I added a > >>>> generic support to simplify it [1]. We will probably need some > >>>> adjustments to make it work on libpthread. > >>>> > >>> > >>> I will share my comments on it in the patch itself. > >>> > >>>> [1] > >>>> https://sourceware.org/pipermail/libc-alpha/2020-July/116259.html > >>>> > >> > >> Thanks. > >> > >>>> > >>>>> > >>>>> 3. What would be the best point in the glibc release cycle to > >>>>> apply this patch set as it convets the core functionality of the > >>>>> library? > >>>>> > >>>>> Just after the stable release? > >>>> > >>>> I think it is late for 2.32, we should postpone it to 2.33. > >>> > >>> I'm fine with postponing it as long as some work will be done in > >>> parallel - the futex system call is crucial in many parts of glibc > >>> library. Sooner we convert it and pull patches into glibc master, > >>> then sooner it matures. > >>> > >>> > >>> Links: > >>> [1] > >>> -https://patchwork.ozlabs.org/project/glibc/patch/20200707150827.20899-4-lukma@denx.de/ > >>> > >>> > >>> Best regards, > >>> > >>> Lukasz Majewski > >>> > >>> -- > >>> > >>> DENX Software Engineering GmbH, Managing Director: Wolfgang > >>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > >>> Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > >>> lukma@denx.de > >> > > > > > > > > > > Best regards, > > > > Lukasz Majewski > > > > -- > > > > DENX Software Engineering GmbH, Managing Director: Wolfgang > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > > lukma@denx.de > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Fri, Jul 17, 2020 at 9:27 AM Lukasz Majewski <lukma@denx.de> wrote: > > > > Yes, it is on my list. I am trying to handle the mess of stat > > functions > > Regarding stat - it is indeed a mess - many versions of the same > functions for generic, LFS, and architectures (like ARM). Conversion to > 64 bit time support is challenging there as well. > > But as fair as I can tell - more than "code base" cleanup - the raise > of glibc minimal kernel supported version would help much more in this > situation as code for many "use cases" would be just removed. > > A side remark - now the oldest LTS kernel supported is 4.4 and oldest > supported kernel for glibc is 3.2. > Maybe it is a good time to bump the minimal supported kernel for glibc? > > > before starting on the lowlevellock-futex.h. > > If you don't have time to do the conversion in next two weeks then > - I can in the meantime do a conversion for the above code snippet - > futex_reltimed_wait_time64() (and up to pthread_mutex_{timed,clock}). > This work shall be orthogonal to lowlevellock-futex.h cleanup. > > What do you think? I don't think it makes any difference for stat: linux-3.2 and linux-5.0 appear to have the exact same kernel interface for newstat/stat64 and newfstatat/fstatat64 on all architectures, though there are two special cases that need to be handled: - mips-n32 and ix86-x32 are lacking the stat64 version, so they only have the non-LFS newstat/newfstatat version but not stat64/fstatat64. The x32 version is wide enough to support 64-bit offsets but uses the non-LFS name for it. - alpha, parisc and sparc are 64-bit architecture with stat64/fstatat64, but they lack newstat/newfstatat. While statx() was added in in linux-4.11, it was not enabled in ia64 until linux-5.1 when we sanitized the syscall tables across architectures, so you can't rely on that until v5.1 is the minimum kernel for glibc, and that will still be a long time. There may be other reasons to increase the minimum runtimer kernel level or the minimum kernel header version for glibc in the meantime. Arnd
On Fri, 17 Jul 2020 10:11:50 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Jul 17, 2020 at 9:27 AM Lukasz Majewski <lukma@denx.de> wrote: > > > > > > Yes, it is on my list. I am trying to handle the mess of stat > > > functions > > > > Regarding stat - it is indeed a mess - many versions of the same > > functions for generic, LFS, and architectures (like ARM). > > Conversion to 64 bit time support is challenging there as well. > > > > But as fair as I can tell - more than "code base" cleanup - the > > raise of glibc minimal kernel supported version would help much > > more in this situation as code for many "use cases" would be just > > removed. > > > > A side remark - now the oldest LTS kernel supported is 4.4 and > > oldest supported kernel for glibc is 3.2. > > Maybe it is a good time to bump the minimal supported kernel for > > glibc? > > > before starting on the lowlevellock-futex.h. > > > > If you don't have time to do the conversion in next two weeks then > > - I can in the meantime do a conversion for the above code snippet - > > futex_reltimed_wait_time64() (and up to > > pthread_mutex_{timed,clock}). This work shall be orthogonal to > > lowlevellock-futex.h cleanup. > > > > What do you think? > > I don't think it makes any difference for stat: linux-3.2 and > linux-5.0 appear to have the exact same kernel interface for > newstat/stat64 and newfstatat/fstatat64 on all architectures, though > there are two special cases that need to be handled: > > - mips-n32 and ix86-x32 are lacking the stat64 version, so they only > have the non-LFS newstat/newfstatat version but not > stat64/fstatat64. The x32 version is wide enough to support 64-bit > offsets but uses the non-LFS name for it. > > - alpha, parisc and sparc are 64-bit architecture with > stat64/fstatat64, but they lack newstat/newfstatat. > > While statx() was added in in linux-4.11, it was not enabled in ia64 > until linux-5.1 when we sanitized the syscall tables across > architectures, so you can't rely on that until v5.1 is the minimum > kernel for glibc, and that will still be a long time. Yes, I was thinking about the statx availability since 4.11 - it supports 64 bit time out of the box even on 32 bit machines, so can be easily used to internally (i.e. in-glibc) implement stat calls for them. Unfortunately, I've overlooked the case for ia64. Thanks for pointing this out. > > There may be other reasons to increase the minimum runtimer kernel > level or the minimum kernel header version for glibc in the meantime. > > Arnd Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Fri, 17 Jul 2020, Lukasz Majewski wrote: > But as fair as I can tell - more than "code base" cleanup - the raise > of glibc minimal kernel supported version would help much more in this > situation as code for many "use cases" would be just removed. > > A side remark - now the oldest LTS kernel supported is 4.4 and oldest > supported kernel for glibc is 3.2. > Maybe it is a good time to bump the minimal supported kernel for glibc? Carlos was proposing to remove the runtime check that prevents glibc from running if the kernel version is too old, on the basis that often new features may have been backported to an older kernel or may not actually be used in glibc for many programs, so a newer glibc could often work in a container under a kernel reporting an older version number. I think it makes sense to do that before increasing the minimum supported kernel version. An increase to 4.4 allows very little cleanup relevant for e.g. x86_64. The main cleanup allowed is removing support for socketcall for many socket operations (and thus probably allowing some of them to be implemented through syscalls.list for all architectures rather than needing custom C implementations). That requires careful checks on the status of each such syscall for each architecture in 4.4, however - note the comment in sparc/kernel-features.h about some syscalls having been added for 32-bit in that version only for 32-bit kernels and not in the compat syscall table for 64-bit kernels, so we'd need to keep support for at least getpeername and getsockname using socketcall until 4.20 (Linux kernel commit 1f2b5b8e2df4591fbca430aff9c5a072dcc0f408) or later is the minimum version.
On 17/07/2020 05:11, Arnd Bergmann wrote: > On Fri, Jul 17, 2020 at 9:27 AM Lukasz Majewski <lukma@denx.de> wrote: >>> >>> Yes, it is on my list. I am trying to handle the mess of stat >>> functions >> >> Regarding stat - it is indeed a mess - many versions of the same >> functions for generic, LFS, and architectures (like ARM). Conversion to >> 64 bit time support is challenging there as well. >> >> But as fair as I can tell - more than "code base" cleanup - the raise >> of glibc minimal kernel supported version would help much more in this >> situation as code for many "use cases" would be just removed. >> >> A side remark - now the oldest LTS kernel supported is 4.4 and oldest >> supported kernel for glibc is 3.2. >> Maybe it is a good time to bump the minimal supported kernel for glibc? >> >>> before starting on the lowlevellock-futex.h. >> >> If you don't have time to do the conversion in next two weeks then >> - I can in the meantime do a conversion for the above code snippet - >> futex_reltimed_wait_time64() (and up to pthread_mutex_{timed,clock}). >> This work shall be orthogonal to lowlevellock-futex.h cleanup. >> >> What do you think? > > I don't think it makes any difference for stat: linux-3.2 and linux-5.0 > appear to have the exact same kernel interface for newstat/stat64 > and newfstatat/fstatat64 on all architectures, though there are two > special cases that need to be handled: > > - mips-n32 and ix86-x32 are lacking the stat64 version, so they only > have the non-LFS newstat/newfstatat version but not stat64/fstatat64. > The x32 version is wide enough to support 64-bit offsets but uses > the non-LFS name for it. > > - alpha, parisc and sparc are 64-bit architecture with stat64/fstatat64, > but they lack newstat/newfstatat. > > While statx() was added in in linux-4.11, it was not enabled in ia64 > until linux-5.1 when we sanitized the syscall tables across architectures, > so you can't rely on that until v5.1 is the minimum kernel for glibc, > and that will still be a long time. > > There may be other reasons to increase the minimum runtimer kernel > level or the minimum kernel header version for glibc in the meantime. Indeed raising the kernel does not help us much here, as you pointed out we need to handle some outliers. My current work to simplify the code base and keep me sane while adding y2038 support is to: 1. Consolidate the {f}xstat{at}{64} routines. With some ifdef checks to use the right syscall the only architectures that require special implementations are alpha and mips64 (which support either specific glibc _STAT_VER version or special function conversions. 2. Remove internal usage of the {f}xstat{at}{64} routines and replace with {f}stat{at}64. 3. Move the {f}xstat{at}{64} to compat symbols and add {f}stat{at}64 symbol for 2.33. My plan is also do decouple the struct stat definition from the kernel and define a glibc generic one for *all* architectures (we can do it because there are new symbols). 4. Implement fstat, lstat, and fstat on fstatat basis. This will simplify the y2038 support since only the all syscall selection logic will at fstatat. 5. Add y2038 support by adding a new internal struct __stat{64}_time{64}. These will require some for symbol for non-LFS architectures, but with the implementation consolidation their implementation should be way simpler. For the generic fstatat{64} my plan is to adjust the current struct kernel_stat to defined for *all* architectures (not only when it is required, such as mips). It will decouple the glibc from kernel, although it will require some routines to copy out the result (which we are do for some architectures anyway). In a high level what I have devised so far is: --- * fstatat.c /* XSTAT_IS_XSTAT64 defines whether architecture support LFS or not. */ #if !XSTAT_IS_XSTAT64 /* Copies the fields from the kernel struct stat KST to glibc struct stat ST, returning EOVERFLOW if st_ino, st_size, or st_blocks does not fit in the non-LFS ST. */ extern int __kstat_to_stat (struct kernel_stat *kst, struct stat *st); /* This is build only for ABI with non-LFS support: arm, csky, i386, hppa, m68k, mips32, microblaze, nios2, s390, sh, powerpc32, and sparc32. */ int __fxstatat (int vers, int fd, const char *file, struct stat *st, int flag) { struct kernel_stat kst; int r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &kst, flag); return r ?: __kstat_to_stat (&kst, st); } #endif /* XSTAT_IS_XSTAT64 */ -- * fstatat64.c int __fxstatat64 (int vers, int fd, const char *file, struct stat64 *st, int flag) { #ifdef __NR_newfstatat /* 64-bit kABI, e.g. aarch64, ia64, mips64*, powerpc64*, s390x, riscv64, and x86_64. */ r = INLINE_SYSCALL_CALL (newfstatat, fd, file, &kst, flag); #if defined __NR_fstatat64 /* 64-bit kABI outlier, e.g. alpha and sparc64. */ r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &kst, flag) #else struct statx tmp; int r = INLINE_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag, STATX_BASIC_STATS, &tmp); if (r == 0 || errno != ENOSYS) { if (r == 0) __cp_stat64_statx (st, &tmp); return r; } /* Maybe I need to handle mips64-n32 here... */ r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &kst, flag); #endif return r ?: __kstat_to_stat64 (&kst, st); } -- I am currently working on the 4. item on the list above and this is just the part to consolidate the implementations and make it sane. The y2038 part should be way simpler than trying to work with current implementation: the __fxstatat64 would be the same, but for the statx fallback to use fstatat64 we will need an extra convention to check for overflow.
On 17/07/2020 04:27, Lukasz Majewski wrote: >> >> Yes, it is on my list. I am trying to handle the mess of stat >> functions > > Regarding stat - it is indeed a mess - many versions of the same > functions for generic, LFS, and architectures (like ARM). Conversion to > 64 bit time support is challenging there as well. > > But as fair as I can tell - more than "code base" cleanup - the raise > of glibc minimal kernel supported version would help much more in this > situation as code for many "use cases" would be just removed. > > A side remark - now the oldest LTS kernel supported is 4.4 and oldest > supported kernel for glibc is 3.2. > Maybe it is a good time to bump the minimal supported kernel for glibc? > >> before starting on the lowlevellock-futex.h. > > If you don't have time to do the conversion in next two weeks then > - I can in the meantime do a conversion for the above code snippet - > futex_reltimed_wait_time64() (and up to pthread_mutex_{timed,clock}). > This work shall be orthogonal to lowlevellock-futex.h cleanup. > > What do you think? Sure, I am not sure if I will be able to start the lowlevellock work.
Hi Adhemerval, > On 17/07/2020 05:11, Arnd Bergmann wrote: > > On Fri, Jul 17, 2020 at 9:27 AM Lukasz Majewski <lukma@denx.de> > > wrote: > >>> > >>> Yes, it is on my list. I am trying to handle the mess of stat > >>> functions > >> > >> Regarding stat - it is indeed a mess - many versions of the same > >> functions for generic, LFS, and architectures (like ARM). > >> Conversion to 64 bit time support is challenging there as well. > >> > >> But as fair as I can tell - more than "code base" cleanup - the > >> raise of glibc minimal kernel supported version would help much > >> more in this situation as code for many "use cases" would be just > >> removed. > >> > >> A side remark - now the oldest LTS kernel supported is 4.4 and > >> oldest supported kernel for glibc is 3.2. > >> Maybe it is a good time to bump the minimal supported kernel for > >> glibc? > >>> before starting on the lowlevellock-futex.h. > >> > >> If you don't have time to do the conversion in next two weeks then > >> - I can in the meantime do a conversion for the above code snippet > >> - futex_reltimed_wait_time64() (and up to > >> pthread_mutex_{timed,clock}). This work shall be orthogonal to > >> lowlevellock-futex.h cleanup. > >> > >> What do you think? > > > > I don't think it makes any difference for stat: linux-3.2 and > > linux-5.0 appear to have the exact same kernel interface for > > newstat/stat64 and newfstatat/fstatat64 on all architectures, > > though there are two special cases that need to be handled: > > > > - mips-n32 and ix86-x32 are lacking the stat64 version, so they only > > have the non-LFS newstat/newfstatat version but not > > stat64/fstatat64. The x32 version is wide enough to support 64-bit > > offsets but uses the non-LFS name for it. > > > > - alpha, parisc and sparc are 64-bit architecture with > > stat64/fstatat64, but they lack newstat/newfstatat. > > > > While statx() was added in in linux-4.11, it was not enabled in ia64 > > until linux-5.1 when we sanitized the syscall tables across > > architectures, so you can't rely on that until v5.1 is the minimum > > kernel for glibc, and that will still be a long time. > > > > There may be other reasons to increase the minimum runtimer kernel > > level or the minimum kernel header version for glibc in the > > meantime. > > Indeed raising the kernel does not help us much here, as you pointed > out we need to handle some outliers. My current work to simplify the > code base and keep me sane while adding y2038 support is to: > > 1. Consolidate the {f}xstat{at}{64} routines. With some ifdef > checks to use the right syscall the only architectures that require > special implementations are alpha and mips64 (which support either > specific glibc _STAT_VER version or special function conversions. > > 2. Remove internal usage of the {f}xstat{at}{64} routines and > replace with {f}stat{at}64. > > 3. Move the {f}xstat{at}{64} to compat symbols and add {f}stat{at}64 > symbol for 2.33. My plan is also do decouple the struct stat > definition from the kernel and define a glibc generic one for > *all* architectures (we can do it because there are new symbols). > > 4. Implement fstat, lstat, and fstat on fstatat basis. This will > simplify the y2038 support since only the all syscall selection logic > will at fstatat. > > 5. Add y2038 support by adding a new internal struct > __stat{64}_time{64}. These will require some for symbol for non-LFS > architectures, but with the implementation consolidation their > implementation should be way simpler. > > For the generic fstatat{64} my plan is to adjust the current struct > kernel_stat to defined for *all* architectures (not only when it is > required, such as mips). It will decouple the glibc from kernel, > although it will require some routines to copy out the result (which > we are do for some architectures anyway). In a high level what I > have devised so far is: > > --- > * fstatat.c > > /* XSTAT_IS_XSTAT64 defines whether architecture support LFS or not. > */ #if !XSTAT_IS_XSTAT64 > > /* Copies the fields from the kernel struct stat KST to glibc struct > stat ST, returning EOVERFLOW if st_ino, st_size, or st_blocks does > not fit in the non-LFS ST. */ > extern int __kstat_to_stat (struct kernel_stat *kst, struct stat *st); > > /* This is build only for ABI with non-LFS support: arm, csky, i386, > hppa, m68k, mips32, microblaze, nios2, s390, sh, powerpc32, and > sparc32. */ > int > __fxstatat (int vers, int fd, const char *file, struct stat *st, int > flag) { > struct kernel_stat kst; > int r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &kst, flag); > return r ?: __kstat_to_stat (&kst, st); > } > > #endif /* XSTAT_IS_XSTAT64 */ > > -- > * fstatat64.c > > int > __fxstatat64 (int vers, int fd, const char *file, struct stat64 *st, > int flag) { > #ifdef __NR_newfstatat > /* 64-bit kABI, e.g. aarch64, ia64, mips64*, powerpc64*, s390x, > riscv64, and x86_64. */ > r = INLINE_SYSCALL_CALL (newfstatat, fd, file, &kst, flag); > #if defined __NR_fstatat64 > /* 64-bit kABI outlier, e.g. alpha and sparc64. */ > r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &kst, flag) > #else > struct statx tmp; > int r = INLINE_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | > flag, STATX_BASIC_STATS, &tmp); > if (r == 0 || errno != ENOSYS) > { > if (r == 0) > __cp_stat64_statx (st, &tmp); > return r; > } > /* Maybe I need to handle mips64-n32 here... */ > r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &kst, flag); > #endif > return r ?: __kstat_to_stat64 (&kst, st); > } > -- > > I am currently working on the 4. item on the list above and this is > just the part to consolidate the implementations and make it sane. > The y2038 part should be way simpler than trying to work with current > implementation: the __fxstatat64 would be the same, but for the statx > fallback to use fstatat64 we will need an extra convention to check > for overflow. It would be great to have the "syscall decision logic" in one place. Some of my "hackish" attempts to convert *stat* [1][2][3] to support 64 bit time were using statx as it is already available and supported in glibc. The only issue was to convert/emulate data for stat output. Links: [1] - https://github.com/lmajewski/y2038_glibc/commit/57841b674d3fc007e31e7551a66963834cf096ac [2] - https://github.com/lmajewski/y2038_glibc/commit/d79192c729562f5734a4fbc058e933c2a715d893 [3] - https://github.com/lmajewski/y2038_glibc/commit/c74c19ab74eef4dcba8bd48d5306d4b38c55648e Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Adhemerval, > On 17/07/2020 04:27, Lukasz Majewski wrote: > >> > >> Yes, it is on my list. I am trying to handle the mess of stat > >> functions > > > > Regarding stat - it is indeed a mess - many versions of the same > > functions for generic, LFS, and architectures (like ARM). > > Conversion to 64 bit time support is challenging there as well. > > > > But as fair as I can tell - more than "code base" cleanup - the > > raise of glibc minimal kernel supported version would help much > > more in this situation as code for many "use cases" would be just > > removed. > > > > A side remark - now the oldest LTS kernel supported is 4.4 and > > oldest supported kernel for glibc is 3.2. > > Maybe it is a good time to bump the minimal supported kernel for > > glibc? > >> before starting on the lowlevellock-futex.h. > > > > If you don't have time to do the conversion in next two weeks then > > - I can in the meantime do a conversion for the above code snippet - > > futex_reltimed_wait_time64() (and up to > > pthread_mutex_{timed,clock}). This work shall be orthogonal to > > lowlevellock-futex.h cleanup. > > > > What do you think? > > Sure, I am not sure if I will be able to start the lowlevellock work. > Ok, Then I will prepare proper patch. Thanks. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Fri, 17 Jul 2020, Adhemerval Zanella via Libc-alpha wrote: > 3. Move the {f}xstat{at}{64} to compat symbols and add {f}stat{at}64 > symbol for 2.33. My plan is also do decouple the struct stat > definition from the kernel and define a glibc generic one for > *all* architectures (we can do it because there are new symbols). That's the sort of change that, although it wouldn't break the ABI as seen by an existing set of binaries, would be problematic when any existing shared library has struct stat or struct stat64 in its ABI and people try to link to it with a new binary without recompiling the shared library, or recompile the shared library with new libc without recompiling all binaries linked against it. I don't think such a change to struct stat or struct stat64 would be a good idea - but a generic definition certainly makes sense for the case of 64-bit time on architectures that don't currently have 64-bit time.
Please find this early RFC for converting 'futex' syscall based code (pthreads/nptl/sem/gai) to support 64 bit time. When applicable the futex_time64 syscall is used. The main purpose of this RFC is to assess if taken approach for conversion is correct and acceptable by the glibc community. Quesitons/issues: 1. This whole patch set shall be squashed into a single patch, otherwise, the glibc will not build between separate commits. I've divided it to separate patches on the purpose - to facilitate review. 2. Question about rewritting lll_* macros in lowlevel-*.h - I'm wondering if there is maybe a better way to do it. Please pay attention to the *_4 suffix. 3. What would be the best point in the glibc release cycle to apply this patch set as it convets the core functionality of the library? Just after the stable release? Lukasz Majewski (10): doc: Fix wording and formatting in ./support/README y2038: Convert timespec_* from posix-timer.h to be Y2038 safe y2038: Convert __lll_futex* functions to use futex_time64 when available y2038: Replace struct timespec with __timespec64 in futex-internal.h y2038: Convert pthread_* functions to support 64 bit time y2038: Convert sem_{timed|clock}wait to support 64 bit timeout y2038: Convert __lll_clocklock_wait function to support 64 bit time y2038: Convert aio_suspend to support 64 bit timeout y2038: Convert gai_suspend to support 64 bit timeout y2038: x86: Fix __lll_clocklock_elision to support 64 bit time nptl/lll_timedlock_wait.c | 6 +- nptl/pthreadP.h | 53 +++++++++++++- nptl/pthread_clockjoin.c | 22 +++++- nptl/pthread_cond_wait.c | 46 ++++++++++-- nptl/pthread_join_common.c | 11 +-- nptl/pthread_mutex_timedlock.c | 39 +++++++--- nptl/pthread_rwlock_clockrdlock.c | 21 +++++- nptl/pthread_rwlock_clockwrlock.c | 21 +++++- nptl/pthread_rwlock_common.c | 4 +- nptl/pthread_rwlock_timedrdlock.c | 21 +++++- nptl/pthread_rwlock_timedwrlock.c | 21 +++++- nptl/pthread_timedjoin.c | 20 +++++- nptl/sem_clockwait.c | 21 +++++- nptl/sem_timedwait.c | 18 ++++- nptl/sem_waitcommon.c | 4 +- nptl/semaphoreP.h | 12 ++++ resolv/gai_misc.h | 7 ++ resolv/gai_suspend.c | 33 +++++++-- support/README | 4 +- sysdeps/nptl/aio_misc.h | 11 ++- sysdeps/nptl/futex-internal.h | 10 +-- sysdeps/nptl/lowlevellock-futex.h | 80 +++++++++++++++++++-- sysdeps/nptl/lowlevellock.h | 2 +- sysdeps/pthread/aio_suspend.c | 36 ++++++++-- sysdeps/pthread/posix-timer.h | 11 +-- sysdeps/unix/sysv/linux/x86/elision-timed.c | 2 +- sysdeps/unix/sysv/linux/x86/lowlevellock.h | 2 +- 27 files changed, 462 insertions(+), 76 deletions(-)