Hi Adhemerval, > On 30/09/2020 09:47, Lukasz Majewski wrote: > > 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. > > I recall this was need on first patch for the cancellable 64-bit > futex_abstimed_wait where it embedded the 32-bit fallback in the > __futex_abstimed_wait_cancelable64. And my suggestion to move it to > an external function seems to avoid the extra compiler flag. > > This patchset uses the same strategy and I checked both patches > and it seems that -fno-inline is not required to build m68k. As fair as I can tell (at the time I tested it) - it was necessary to have -fno-inline when this patch was applied on top of the one which is now in the -master branch (in futex-internal.c). I will double check it now with: glibc/glibc-many-build --keep failed glibcs 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