[PATCH v4] y2038: Convert aio_suspend to support 64 bit time

[Adhemerval Zanella]

On 27/11/2020 21:34, Lukasz Majewski wrote:
> On Fri, 27 Nov 2020 14:31:03 -0300
> Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
>> On 27/11/2020 11:17, Lukasz Majewski wrote:
>>> Hi Adhemerval,
>>>   
>>>> On 27/11/2020 09:56, Lukasz Majewski wrote:  
>>>>> The aio_suspend function has been converted to support 64 bit
>>>>> time.
>>>>>
>>>>> This change uses (in aio_misc.h):
>>>>> - __futex_abstimed_wait64 (instead of futex_reltimed_wait)
>>>>> - __futex_abstimed_wait_cancellable64
>>>>> 	(instead of futex_reltimed_wait_cancellable)
>>>>>     from ./sysdeps/nptl/futex-helpers.h
>>>>>
>>>>> The aio_suspend() accepts relative timeout, which then is
>>>>> converted to absolute one.
>>>>>
>>>>> The i686-gnu port (HURD) do not define DONT_NEED_AIO_MISC_COND and
>>>>> as it doesn't (yet) support 64 bit time it uses not converted
>>>>> pthread_cond_timedwait().
>>>>>
>>>>> The __aio_suspend() is supposed to be run on ports with __TIMESIZE
>>>>> !=64 and __WORDSIZE==32. It internally utilizes
>>>>> __aio_suspend_time64() and hence the conversion from 32 bit struct
>>>>> timespec to 64 bit one is required.
>>>>>
>>>>> For ports supporting 64 bit time the __aio_suspend_time64() will
>>>>> be used either via alias (to __aio_suspend when __TIMESIZE==64) or
>>>>> redirection (when -D_TIME_BITS=64 is passed).
>>>>>
>>>>> Build tests:
>>>>> ./src/scripts/build-many-glibcs.py glibcs
>>>>>
>>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>>>> ---
>>>>> Changes for v2:
>>>>> - Add missing -EOVERFLOW error handling for
>>>>> __futex_reltimed_wait64 and _futex_reltimed_wait_cancelable64
>>>>>
>>>>> Changes for v3:
>>>>> - Remove "__" prefix from futex_reltimed_wait64 and
>>>>>   futex_reltimed_wait_cancellable64
>>>>>
>>>>> - Remove some code, as HURD is not defining
>>>>> DONT_NEED_AIO_MISC_COND (i.e. Linux ports are defining it) and
>>>>> add in-code explanation why the code is NOT converted to support
>>>>> 64 bit time.
>>>>>
>>>>> - Rewrite the commit message
>>>>>
>>>>> Changes for v4:
>>>>> - Re-base on the newest master after refactor of the futex code -
>>>>> e.g. no need to provide futex_reltimed_wait_cancelable64()
>>>>>
>>>>> - Add missing libpthread_hidden_{proto|def}    
>>>>
>>>> LGTM with a small fix below.
>>>>
>>>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>>>  
>>>>> ---
>>>>>  include/aio.h                 |  8 +++++
>>>>>  nptl/Versions                 |  1 +
>>>>>  sysdeps/nptl/aio_misc.h       |  9 +++---
>>>>>  sysdeps/nptl/futex-internal.c |  2 ++
>>>>>  sysdeps/nptl/futex-internal.h |  6 ++--
>>>>>  sysdeps/pthread/aio_suspend.c | 60
>>>>> ++++++++++++++++++++--------------- 6 files changed, 55
>>>>> insertions(+), 31 deletions(-)
>>>>>
>>>>> diff --git a/include/aio.h b/include/aio.h
>>>>> index 90c74f9951..c7f4233310 100644
>>>>> --- a/include/aio.h
>>>>> +++ b/include/aio.h
>>>>> @@ -9,6 +9,14 @@ extern void __aio_init (const struct aioinit
>>>>> *__init); lio_listio and we do not issue events for each
>>>>> individual list element.  */
>>>>>  #define LIO_NO_INDIVIDUAL_EVENT	128
>>>>> +
>>>>> +# if __TIMESIZE == 64
>>>>> +#  define __aio_suspend_time64 __aio_suspend
>>>>> +# else
>>>>> +extern int __aio_suspend_time64 (const struct aiocb *const
>>>>> list[], int nent,
>>>>> +                                 const struct __timespec64
>>>>> *timeout); +librt_hidden_proto (__aio_suspend_time64)
>>>>> +# endif
>>>>>  #endif
>>>>>  
>>>>>  #endif
>>>>> diff --git a/nptl/Versions b/nptl/Versions
>>>>> index aed118e717..02650fe91c 100644
>>>>> --- a/nptl/Versions
>>>>> +++ b/nptl/Versions
>>>>> @@ -302,6 +302,7 @@ libpthread {
>>>>>      __pthread_clock_gettime; __pthread_clock_settime;
>>>>>      __pthread_unwind; __pthread_get_minstack;
>>>>>      __pthread_barrier_init; __pthread_barrier_wait;
>>>>> +    __futex_abstimed_wait64; __futex_abstimed_wait_cancelable64;
>>>>>      __shm_directory;
>>>>>      __libpthread_freeres;
>>>>>    }
>>>>> diff --git a/sysdeps/nptl/aio_misc.h b/sysdeps/nptl/aio_misc.h
>>>>> index 3f195f4794..dd8d99e5d6 100644
>>>>> --- a/sysdeps/nptl/aio_misc.h
>>>>> +++ b/sysdeps/nptl/aio_misc.h
>>>>> @@ -45,11 +45,12 @@
>>>>>  	do
>>>>> 	      \ {
>>>>> 		      \ if (cancel)
>>>>> 			      \
>>>>> -	      status = futex_reltimed_wait_cancelable (
>>>>> 		      \
>>>>> -		(unsigned int *) futexaddr, oldval, timeout,
>>>>> FUTEX_PRIVATE);  \
>>>>> +	      status = __futex_abstimed_wait_cancelable64 (
>>>>> 	      \
>>>>> +		(unsigned int *) futexaddr, oldval,
>>>>> CLOCK_MONOTONIC, timeout, \
>>>>> +		FUTEX_PRIVATE);
>>>>> 		      \ else
>>>>> 		      \
>>>>> -	      status = futex_reltimed_wait ((unsigned int *)
>>>>> futexaddr,	      \
>>>>> -		oldval, timeout, FUTEX_PRIVATE);
>>>>> 		      \
>>>>> +	      status = __futex_abstimed_wait64 ((unsigned int *)
>>>>> futexaddr,   \
>>>>> +		oldval, CLOCK_REALTIME, timeout, FUTEX_PRIVATE);
>>>>> 	      \ if (status != EAGAIN)
>>>>> 		      \ break;
>>>>> 			      \   
>>>>
>>>> It also need to check for EOVERFLOW along with the ETIMEOUT check
>>>> (to return EAGAIN).  
>>>
>>> But this was already implemented in the patch, which you posted
>>> today? Or shall I do something more?  
>>
>> I haven't changed the futex_reltimed_wait{_cancelable} because they
>> currently do not return EOVERFLOW.  The aio_misc.h code will possible
>> return with this change.
> 
> Ok, So I will add such check and repost the patch.

I think the check is quite straightforward (if is on the same if that
checks for ETIMEDOUT). You can't just add it and push it upstream,
thanks.