[RFC 00/10] y2038: nptl: futex: Provide support for futex_time64
Lukasz Majewski
lukma@denx.de
Wed Jul 15 08:47:38 GMT 2020
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20200715/659a171a/attachment-0001.sig>
More information about the Libc-alpha
mailing list