Hi Adhemerval, > On 19/09/2020 10:07, Lukasz Majewski wrote: > > This is the helper function, which uses struct __timespec64 > > to provide 64 bit absolute time to futex syscalls. > > > > The aim of this function is to move convoluted pre-processor > > macro code from sysdeps/nptl/lowlevellock-futex.h to C > > function in futex-internal.c > > > > The futex_abstimed_wait64 function has been put into a separate > > file on the purpose - to avoid issues apparent on the m68k > > architecture related to small number of available registers (there > > is not enough registers to put all necessary arguments in them if > > the above function would be added to futex-internal.h with > > __always_inline attribute). > > > > Additional precautions for m68k port have been taken - the > > futex-internal.c file will be compiled with -fno-inline flag. > > LGTM with a nit regarding style and a clarification about > '-fno-inline'. Please find the explanation below. > > > > > --- > > Changes for v2: > > - Handle the case when *abstime pointer is NULL > > --- > > sysdeps/nptl/futex-internal.c | 70 > > +++++++++++++++++++++++++++ sysdeps/nptl/futex-internal.h | > > 6 +++ sysdeps/unix/sysv/linux/m68k/Makefile | 2 + > > 3 files changed, 78 insertions(+) > > > > diff --git a/sysdeps/nptl/futex-internal.c > > b/sysdeps/nptl/futex-internal.c index 3366aac162..3211b4c94f 100644 > > --- a/sysdeps/nptl/futex-internal.c > > +++ b/sysdeps/nptl/futex-internal.c > > @@ -45,6 +45,29 @@ __futex_abstimed_wait_cancelable32 (unsigned > > int* futex_word, abstime != NULL ? &ts32 : NULL, > > NULL /* Unused. */, > > FUTEX_BITSET_MATCH_ANY); } > > + > > +static int > > +__futex_abstimed_wait32 (unsigned int* futex_word, > > + unsigned int expected, clockid_t clockid, > > + const struct __timespec64* abstime, > > + int private) > > +{ > > + struct timespec ts32; > > + > > + if (abstime != NULL && ! in_time_t_range (abstime->tv_sec)) > > + return -EOVERFLOW; > > + > > + unsigned int clockbit = (clockid == CLOCK_REALTIME) ? > > + FUTEX_CLOCK_REALTIME : 0; > > + int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, > > private); + > > + if (abstime != NULL) > > + ts32 = valid_timespec64_to_timespec (*abstime); > > + > > + return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected, > > + abstime != NULL ? &ts32 : NULL, > > + NULL /* Unused. */, > > FUTEX_BITSET_MATCH_ANY); +} > > #endif > > > > int > > Ok. > > > @@ -97,3 +120,50 @@ __futex_abstimed_wait_cancelable64 (unsigned > > int* futex_word, futex_fatal_error (); > > } > > } > > + > > +int > > +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int > > expected, > > + clockid_t clockid, > > + const struct __timespec64* abstime, int > > private) +{ > > + unsigned int clockbit; > > + int err; > > + > > + /* Work around the fact that the kernel rejects negative timeout > > values > > + despite them being valid. */ > > + if (__glibc_unlikely ((abstime != NULL) && (abstime->tv_sec < > > 0))) > > + return ETIMEDOUT; > > + > > + if (! lll_futex_supported_clockid(clockid)) > > + return EINVAL; > > Space before parentheses. Ok. Fixed. > > > + > > + clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : > > 0; > > + int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, > > private); + > > + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, > > expected, > > + abstime, NULL /* Unused. */, > > + FUTEX_BITSET_MATCH_ANY); > > +#ifndef __ASSUME_TIME64_SYSCALLS > > + if (err == -ENOSYS) > > + err = __futex_abstimed_wait32 (futex_word, expected, > > + clockid, abstime, private); > > +#endif > > + switch (err) > > + { > > + case 0: > > + case -EAGAIN: > > + case -EINTR: > > + case -ETIMEDOUT: > > + return -err; > > + > > + case -EFAULT: /* Must have been caused by a glibc or > > application bug. */ > > + case -EINVAL: /* Either due to wrong alignment, unsupported > > + clockid 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 (); > > + } > > +} > > Ok. > > > diff --git a/sysdeps/nptl/futex-internal.h > > b/sysdeps/nptl/futex-internal.h index 7f3910ad98..1ba0d61938 100644 > > --- a/sysdeps/nptl/futex-internal.h > > +++ b/sysdeps/nptl/futex-internal.h > > @@ -529,4 +529,10 @@ __futex_abstimed_wait_cancelable64 (unsigned > > int* futex_word, const struct __timespec64* abstime, > > int private) attribute_hidden; > > > > +int > > +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int > > expected, > > + clockid_t clockid, > > + const struct __timespec64* abstime, > > + int private) attribute_hidden; > > + > > #endif /* futex-internal.h */ > > Ok. > > > diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile > > b/sysdeps/unix/sysv/linux/m68k/Makefile index > > be40fae68a..65164c5752 100644 --- > > a/sysdeps/unix/sysv/linux/m68k/Makefile +++ > > b/sysdeps/unix/sysv/linux/m68k/Makefile @@ -21,3 +21,5 @@ > > sysdep-dl-routines += dl-static sysdep-others += lddlibc4 > > install-bin += lddlibc4 > > endif > > + > > +CFLAGS-futex-internal.c += -fno-inline > > > > Do we still need it? Unfortunately, yes. The m68k architecture has the issue with properly compiling this code (in futex-internal.c) as it has very limited number of registers (8 x 32 bit IIRC). I did some investigation and it looks like static inline in_time_t_range() function affects the compiler capabilities to generate correct code. It can be easily inlined on any architecture but m68k. As m68k has issues with this code compilation - it is IMHO better to disable inlining (-fno-inline) on this particular port. As a result we would have slower execution only on relevant functions, but 64 bit time support would be provided. 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