On Wed, 30 Sep 2020 15:39:53 +0200 Lukasz Majewski wrote: > 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 Indeed with current -master the issue is gone: ../src/scripts/build-many-glibcs.py /home/lukma/work/glibc/glibc-many-build --keep failed glibcs m68k-linux-gnu m68k-linux-gnu-coldfire m68k-linux-gnu-coldfire-soft x86_64-linux-gnu Is all OK with the -fno-inline removed.... Strange. > > > 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