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 B9D30385ED4D for ; Wed, 15 Jul 2020 08:47:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B9D30385ED4D 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 4B69xw2Mzkz1rwvQ; Wed, 15 Jul 2020 10:47:48 +0200 (CEST) Received: from localhost (dynscan1.mnet-online.de [192.168.6.70]) by mail.m-online.net (Postfix) with ESMTP id 4B69xw0zGtz1r56b; Wed, 15 Jul 2020 10:47:48 +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 YwXCcqp-PUh5; Wed, 15 Jul 2020 10:47:45 +0200 (CEST) X-Auth-Info: KcXNclyufKKOckmDTPLRcLhMQX6NhGEZ61N4wISoLuU= 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, 15 Jul 2020 10:47:45 +0200 (CEST) Date: Wed, 15 Jul 2020 10:47:38 +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 Subject: Re: [RFC 00/10] y2038: nptl: futex: Provide support for futex_time64 Message-ID: <20200715104738.245adf67@jawa> In-Reply-To: References: <20200707150827.20899-1-lukma@denx.de> <8e50e48c-f3e9-7c56-bff7-8da0d80a4bf2@linaro.org> <20200714095617.320eb56d@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_/Y/5z.j/mKBhCaDxoTOU1z1y"; protocol="application/pgp-signature" X-Spam-Status: No, score=-1.3 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, KAM_NUMSUBJECT, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=no 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, 15 Jul 2020 08:47:54 -0000 --Sig_/Y/5z.j/mKBhCaDxoTOU1z1y Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Hi Adhemerval, > On 14/07/2020 04:56, Lukasz Majewski wrote: > > Hi Adhemerval, > > =20 > >> On 07/07/2020 12:08, Lukasz Majewski wrote: =20 > >>> Please find this early RFC for converting 'futex' syscall based > >>> code (pthreads/nptl/sem/gai) to support 64 bit time. > >>> When applicable the futex_time64 syscall is used. > >>> > >>> The main purpose of this RFC is to assess if taken approach for > >>> conversion is correct and acceptable by the glibc community. > >>> > >>> Quesitons/issues: > >>> > >>> 1. This whole patch set shall be squashed into a single patch, > >>> otherwise, the glibc will not build between separate commits. I've > >>> divided it to separate patches on the purpose - to facilitate > >>> review. =20 > >> > >> Another possibility could to work by adjusting each symbol and the > >> required futex_* / lll_lock machinery instead. For instance, add > >> 64-bit time_t support pthread_mutex_{timed,clock}lock, which in > >> turn required to adjust > >> futex_lock_pi/lll_futex_timed_wait/lll_futex_clock_wait_bitset. =20 > >=20 > > I've looked for such possibility, but the real issue IMHO is the > > need to convert struct timespec to struct __timespec64 in functions > > definitions/declarations. > >=20 > > In the end you would need to convert lll_futex_syscall() which need > > to support __ASSUME_TIME64_SYSCALLS and futex_time64. > > After this you are forced to convert all other pthread syscalls, > > which use timeout argument. =20 >=20 > My idea would be to implement the internal > pthread_mutex_{timed,clock}_time64 and make > pthread_mutex_{timed,clock} call them.=20 Let's consider __pthread_mutex_timedlock() from ./nptl/pthread_mutex_timedlock.c Its conversion to 64 bit time has been proposed in RFC: https://patchwork.ozlabs.org/project/glibc/patch/20200707150827.20899-6-luk= ma@denx.de/ > For the internal time64 > variant the idea would to add new time64 helper functions, in this > case futex_lock_pi_time64, lll_futex_timed_wait_time64, and > lll_futex_clock_wait_bitset_time64. I've written some comments down below regarding this issue. >=20 > What I mean is we would incrementally add new functions without > touching to old lll/futex definitions. Once we move all the internal > implementation to time64 we can then remove them altogether.\ My concern is that we will introduce "helper" functions (with 64 or time64 suffix) for internal functions - a lot of new code in the situation where we shall use __timespe64 anyway as an internal glibc representation of time. >=20 > > =20 > >> > >> In this way we can tests the change better since they are > >> incremental. =20 > >=20 > > Please see above comment - lowlevellock-futex.h written with C > > preprocessor macros is the real issue here. > > =20 > >> =20 > >>> > >>> 2. Question about rewritting lll_* macros in lowlevel-*.h - I'm > >>> wondering if there is maybe a better way to do it. Please pay > >>> attention to the *_4 suffix. =20 > >> > >> For lll_* I really think we should convert them to proper inline > >> function instead, the required code change to adjust the macro is > >> becoming really convoluted. =20 > >=20 > > I fully agree here - the code as proposed[1] - is rather not clean > > and safe. > > =20 > >> I can help you on refactoring to code so > >> the time64 changes should be simpler. =20 > >=20 > > Ok. Thanks. > >=20 > > Now for instance we do have: > >=20 > > __pthread_mutex_clocklock_common (funcion) > > (pthread_mutex_timedlock.c) | > > \|/ > > lll_timedwait (macro) __lll_clocklock > > | | > > \|/ \|/ > > __lll_clocklock_wait -> eligible for "conversion" Y2038 > > (function!) lll_futex_timed_wait -> (macro) > > lll_futex_syscall -> here is the call to futex syscall (macro) > >=20 > > The code itself has many levels with inline functions convoluted > > with macros. =20 >=20 > Yeah, this is messy indeed. In fact now that we don't have a NaCL > port anymore I don't seem much point in the extra lll_* indirections. Just wondering - what was the rationale for extra lll_* indirection for the NaCL port? >=20 > For instance, on pthread_mutex_{timed,clock} I think we can move and > rename both time64 lll_futex_timed_wait and > lll_futex_clock_wait_bitset to sysdeps/nptl/futex-internal.h and call > INTERNAL_SYSCALL_* directly. >=20 > Something like: >=20 > static inline int > futex_private_flag (int fl, int priv) > { > #if IS_IN (libc) || IS_IN (rtld) > return fl | FUTEX_PRIVATE_FLAG; > #else > return (fl | FUTEX_PRIVATE_FLAG) ^ priv;=20 > #endif > } >=20 > static __always_inline int > futex_reltimed_wait_time64 (unsigned int* futex_word, unsigned int > expected, const struct __timespec64* reltime, int priv) > { > #ifndef __NR_futex_time64 > # define __NR_futex_time64 __NR_futex > #endif > int r =3D INTERNAL_SYSCALL_CALL (futex, futex_word, > futex_private_flag (FUTEX_WAIT, > priv), expected, reltime); > #ifndef __ASSUME_TIME64_SYSCALLS > if (r =3D=3D -ENOSYS) > { > struct timespec ts32, *pts32 =3D NULL; > if (timeout !=3D NULL) > { > if (! in_time_t_range (timeout->tv_sec)) > return -EINVAL; > ts32 =3D valid_timespec64_to_timespec (ts64); > pts32 =3D &ts32; > } >=20 > r =3D INTERNAL_SYSCALL_CALL (futex, futex_word, > futex_private_flag (FUTEX_WAIT, > priv), expected, pts32); > } > #endif >=20 > switch (r) > { > case 0: > case -EAGAIN: > case -EINTR: > case -ETIMEDOUT: > return -r; >=20 > case -EFAULT: /* Must have been caused by a glibc or > application bug. */ case -EINVAL: /* Either due to wrong alignment > 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 > } If having the invocations to futex and futex_time64 syscalls is not the problem in many places - like futex-internal.h and lowlevel-futex.h and also if removing the lll_* indirection is welcome, then I'm fine with it. With the above patch we can also rename struct timespec to __timespec64 for eligible functions - like futex_lock_pi64. Will you have time to prepare the cleanup patch for lowlevellock-futex.h in the near future? Or maybe to prepare the patch with the code you wrote above? >=20 > > =20 > >> > >> Also, futex is a syscall used extensively and I think we should > >> optimize the fallback code to avoid issue the 64-bit time one if > >> the kernel does not support it (as we do for clock_gettime). =20 > >=20 > > I think that this is not the case for most systems. > >=20 > > The 64 bit call for futex_time64 (and other 64 bit syscalls) were > > introduced in Linux 5.1 - which is now more than a year ago. > >=20 > > The newest LTS Linux (5.4) now supports it - so it is likely that > > new BSPs will use it. Especially, yocto LTS - dunfell (3.1) > > supports by default 5.4 kernel. =20 >=20 > It really depends of the kind of deployment and time32 applications > will stick for a while, so IMHO we need to not penalize them too much > with the move to use the time64 syscalls as default. I'm wondering what are the numbers - i.e. what is the exact penalty for this? >=20 > For riscv32 and other ABI wuth time64 as default ABI it should not be > a problem, Yes. I do agree. It is also not a problem for new ports for ARM. > but at least for i686 I foresee it might indeed impact > 32-bit application that relies heavily on libpthread (specially on > patterns where locks are used for highly granulated contention). And > this might happen most with virtualized environments where the host > kernel is not updated as the client one (I think last conversation > with Florian he say it is a common scenario for RHEL). Could you elaborate on this?=20 Please correct me if I'm wrong, but if the client is updated to kernel 5.1+ and recent glibc, then it shall support 64 bit time no matter how old is the host VM system. Problem with performance - with missing syscalls - would start when the old kernel is left in place and only glibc with rootfs is updated. >=20 > In any case I think we can incrementally add such optimizations. Yes, if it solves the real issue. >=20 > > =20 > >> > >> I have send a patchset with some y2038 fixes and I added a generic > >> support to simplify it [1]. We will probably need some adjustments > >> to make it work on libpthread. > >> =20 > >=20 > > I will share my comments on it in the patch itself. > > =20 > >> [1] > >> https://sourceware.org/pipermail/libc-alpha/2020-July/116259.html =20 >=20 > Thanks. >=20 > >> =20 > >>> > >>> 3. What would be the best point in the glibc release cycle to > >>> apply this patch set as it convets the core functionality of the > >>> library? > >>> > >>> Just after the stable release? =20 > >> > >> I think it is late for 2.32, we should postpone it to 2.33. =20 > >=20 > > I'm fine with postponing it as long as some work will be done in > > parallel - the futex system call is crucial in many parts of glibc > > library. Sooner we convert it and pull patches into glibc master, > > then sooner it matures. > >=20 > >=20 > > Links: > > [1] > > -https://patchwork.ozlabs.org/project/glibc/patch/20200707150827.20899-= 4-lukma@denx.de/ > >=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=20 >=20 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_/Y/5z.j/mKBhCaDxoTOU1z1y Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQEzBAEBCgAdFiEEgAyFJ+N6uu6+XupJAR8vZIA0zr0FAl8OwqoACgkQAR8vZIA0 zr3s3wf+KDVInebThnUDqc1VRU16nPSk7a+qxXyiWVEZ9Ybv+250QmAlewo3+t6K OlY9POtN7Ya61gMl+VX4C5r/nlw0mq5UVJc3P8x3Ifi6Mw5qdy1y34nNuiY4X+v6 1qyTEeY/mUYs2cNxy5xv91SfjNolD/zZB+owVCXqBwxQYCjrhtf8NyJU1yJZ87Rk FWS1cdI2wixlvoo6QZ2ffyd8jsr0dB/z+PycWcWyREVasNfMvlUrV5C0vr6vnINE 2LW+RPG1UfVWZ5HUKe5SW8vxJHt3n4SIGmmyJYNPdjXkqpZtqmXWG5Ryqv0p1SJ6 liYLSNHW3xavIzXqdc3ohTyW7c/e1A== =EskG -----END PGP SIGNATURE----- --Sig_/Y/5z.j/mKBhCaDxoTOU1z1y--