[RFC 00/10] y2038: nptl: futex: Provide support for futex_time64
Adhemerval Zanella
adhemerval.zanella@linaro.org
Thu Jul 16 19:02:23 GMT 2020
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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20200716/d06abee1/attachment.sig>
More information about the Libc-alpha
mailing list