From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-out.m-online.net (mail-out.m-online.net [212.18.0.10]) by sourceware.org (Postfix) with ESMTPS id 86E80398793F for ; Wed, 30 Sep 2020 14:06:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 86E80398793F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: sourceware.org; spf=none smtp.mailfrom=lukma@denx.de Received: from frontend01.mail.m-online.net (unknown [192.168.8.182]) by mail-out.m-online.net (Postfix) with ESMTP id 4C1dN93X0Qz1rxwG; Wed, 30 Sep 2020 16:06:33 +0200 (CEST) Received: from localhost (dynscan1.mnet-online.de [192.168.6.70]) by mail.m-online.net (Postfix) with ESMTP id 4C1dN92DBHz1sM91; Wed, 30 Sep 2020 16:06:33 +0200 (CEST) X-Virus-Scanned: amavisd-new at mnet-online.de Received: from mail.mnet-online.de ([192.168.8.182]) by localhost (dynscan1.mail.m-online.net [192.168.6.70]) (amavisd-new, port 10024) with ESMTP id Nm5ek3UoOS8W; Wed, 30 Sep 2020 16:06:31 +0200 (CEST) X-Auth-Info: uRZZf16hhHt0KafdI7wwUSLdom2m3Dkm++0+UT6srOA= Received: from jawa (85-222-111-42.dynamic.chello.pl [85.222.111.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.mnet-online.de (Postfix) with ESMTPSA; Wed, 30 Sep 2020 16:06:31 +0200 (CEST) Date: Wed, 30 Sep 2020 16:06:25 +0200 From: Lukasz Majewski To: Adhemerval Zanella Cc: Joseph Myers , Paul Eggert , Alistair Francis , Arnd Bergmann , Alistair Francis , GNU C Library , Florian Weimer , Carlos O'Donell , Stepan Golosunov , Andreas Schwab , Zack Weinberg , Jeff Law Subject: Re: [PATCH v2 2/3] Y2038: nptl: Provide futex_abstimed_wait64 supporting 64 bit time Message-ID: <20200930160625.00f3ed4d@jawa> In-Reply-To: <20200930153953.424a46c2@jawa> References: <20200919130759.31916-1-lukma@denx.de> <20200919130759.31916-2-lukma@denx.de> <20200930144710.662d9d99@jawa> <20200930153953.424a46c2@jawa> Organization: denx.de X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; boundary="Sig_/BEu7mDQ8=/HWh1J5kFQREbQ"; protocol="application/pgp-signature" X-Spam-Status: No, score=-16.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 Sep 2020 14:06:36 -0000 --Sig_/BEu7mDQ8=/HWh1J5kFQREbQ Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Wed, 30 Sep 2020 15:39:53 +0200 Lukasz Majewski wrote: > Hi Adhemerval, >=20 > > On 30/09/2020 09:47, Lukasz Majewski wrote: =20 > > > Hi Adhemerval, > > > =20 > > >> On 19/09/2020 10:07, Lukasz Majewski wrote: =20 > > >>> 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. > > >>> =20 > > >> > > >> LGTM with a nit regarding style and a clarification about=20 > > >> '-fno-inline'. =20 > > >=20 > > > Please find the explanation below. > > > =20 > > >> =20 > > >>> > > >>> --- > > >>> 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 !=3D 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 !=3D NULL && ! in_time_t_range (abstime->tv_sec)) > > >>> + return -EOVERFLOW; > > >>> + > > >>> + unsigned int clockbit =3D (clockid =3D=3D CLOCK_REALTIME) ? > > >>> + FUTEX_CLOCK_REALTIME : 0; > > >>> + int op =3D __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, > > >>> private); + > > >>> + if (abstime !=3D NULL) > > >>> + ts32 =3D valid_timespec64_to_timespec (*abstime); > > >>> + > > >>> + return INTERNAL_SYSCALL_CALL (futex, futex_word, op, > > >>> expected, > > >>> + abstime !=3D NULL ? &ts32 : NULL, > > >>> + NULL /* Unused. */, > > >>> FUTEX_BITSET_MATCH_ANY); +} > > >>> #endif > > >>> =20 > > >>> int =20 > > >> > > >> Ok. > > >> =20 > > >>> @@ -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 !=3D NULL) && (abstime->tv_sec < > > >>> 0))) > > >>> + return ETIMEDOUT; > > >>> + > > >>> + if (! lll_futex_supported_clockid(clockid)) > > >>> + return EINVAL; =20 > > >> > > >> Space before parentheses. =20 > > >=20 > > > Ok. Fixed. > > > =20 > > >> =20 > > >>> + > > >>> + clockbit =3D (clockid =3D=3D CLOCK_REALTIME) ? > > >>> FUTEX_CLOCK_REALTIME : 0; > > >>> + int op =3D __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, > > >>> private); + > > >>> + err =3D INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, > > >>> expected, > > >>> + abstime, NULL /* Unused. */, > > >>> + FUTEX_BITSET_MATCH_ANY); > > >>> +#ifndef __ASSUME_TIME64_SYSCALLS > > >>> + if (err =3D=3D -ENOSYS) > > >>> + err =3D __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 (); > > >>> + } > > >>> +} =20 > > >> > > >> Ok. > > >> =20 > > >>> 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;=20 > > >>> +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 */ =20 > > >> > > >> Ok. > > >> =20 > > >>> 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 +=3D dl-static sysdep-others +=3D lddlibc4 > > >>> install-bin +=3D lddlibc4 > > >>> endif > > >>> + > > >>> +CFLAGS-futex-internal.c +=3D -fno-inline > > >>> =20 > > >> > > >> Do we still need it? =20 > > >=20 > > > 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). > > >=20 > > > I did some investigation and it looks like static inline > > > in_time_t_range() function affects the compiler capabilities to > > > generate correct code. > > >=20 > > > It can be easily inlined on any architecture but m68k. > > >=20 > > > As m68k has issues with this code compilation - it is IMHO better > > > to disable inlining (-fno-inline) on this particular port. > > >=20 > > > As a result we would have slower execution only on relevant > > > functions, but 64 bit time support would be provided. =20 > >=20 > > 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=20 > > __futex_abstimed_wait_cancelable64. And my suggestion to move it to=20 > > an external function seems to avoid the extra compiler flag. > >=20 > > This patchset uses the same strategy and I checked both patches > > and it seems that -fno-inline is not required to build m68k. =20 >=20 > 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). >=20 > I will double check it now with: >=20 > 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. >=20 >=20 > Best regards, >=20 > Lukasz Majewski >=20 > -- >=20 > 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 --Sig_/BEu7mDQ8=/HWh1J5kFQREbQ Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQEzBAEBCgAdFiEEgAyFJ+N6uu6+XupJAR8vZIA0zr0FAl90kOEACgkQAR8vZIA0 zr0hFwgAx9PkW5xSF4J51ZvtfmBqMSKCqts6t4SFxHVOYtgiy1ZoJNSl0IloewTf 2sk5TF71+o/cgQ51RViCv6EtGGWeLc9dcsgkvUYoubyDaVlqJg59J80EoIlkT6F7 g6hH6P/WE8LwXw730BDUQnUSH2EZkRmu/gQQvKl6LSPTmk6+e2aMyrUBEcemqmnX H+uq14n9hgPWHkDXMcirCJfsAqQxRg1IlfndD8aSZwsEpmqsSkIwkEs1/bzCU6HX RlcGoTIyqMlg8tBlY3+OVzVd560cQSRQkbKVhSmyVmJ1NrNBfIPfk/+dwyVgavrP UrIhiiHQLsUoibA5WBj42WKeqb8j7g== =JWHd -----END PGP SIGNATURE----- --Sig_/BEu7mDQ8=/HWh1J5kFQREbQ--