[RFC 00/10] y2038: nptl: futex: Provide support for futex_time64
Adhemerval Zanella
adhemerval.zanella@linaro.org
Tue Jul 14 15:10:29 GMT 2020
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
>
-------------- 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/20200714/4eb1f624/attachment-0001.sig>
More information about the Libc-alpha
mailing list