Message ID | 20200919130759.31916-3-lukma@denx.de |
---|---|
State | New |
Headers | show |
Series | [v2,1/3] nptl: futex: Move __NR_futex_time64 alias to beginning of futex-internal.h | expand |
Dear Community, > The pthread_rwlock_clockrdlock, pthread_rwlock_clockwrlock, > pthread_rwlock_timedrdlock and pthread_rwlock_timedwrlock have been > converted to support 64 bit time. > > This change uses new futex_abstimed_wait64 function in > ./sysdeps/nptl/futex-helpers.c, which uses futex_time64 where > possible. > > The pthread_rwlock_{clock|timed}{rd|wr}lock only accepts absolute > time. Moreover, there is no need to check for NULL passed as *abstime > pointer to the syscalls as those calls have exported symbols marked > with __nonull attribute for abstime. > > For systems with __TIMESIZE != 64 && __WORDSIZE == 32: > - Conversions between 64 bit time to 32 bit are necessary > - Redirection to pthread_rwlock_{clock|timed}{rd|wr}lock will provide > support for 64 bit time Do we have more comments regarding this patch? Or shall I pull it to -master? > > Build tests: > ./src/scripts/build-many-glibcs.py glibcs > > Run-time tests: > - Run specific tests on ARM/x86 32bit systems (qemu): > https://github.com/lmajewski/meta-y2038 and run tests: > https://github.com/lmajewski/y2038-tests/commits/master > > Above tests were performed with Y2038 redirection applied as well as > without to test the proper usage of both > __pthread_rwlock_{clock|timed}{rd|wr}lock64 and > __pthread_rwlock_{clock|timed}{rd|wr}lock. > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > --- > Changes for v2: > - None > --- > nptl/pthreadP.h | 18 +++++++++++++++++ > nptl/pthread_rwlock_clockrdlock.c | 19 +++++++++++++++--- > nptl/pthread_rwlock_clockwrlock.c | 19 +++++++++++++++--- > nptl/pthread_rwlock_common.c | 33 > +++++++++++++++---------------- nptl/pthread_rwlock_rdlock.c | > 2 +- nptl/pthread_rwlock_timedrdlock.c | 19 +++++++++++++++--- > nptl/pthread_rwlock_timedwrlock.c | 19 +++++++++++++++--- > nptl/pthread_rwlock_wrlock.c | 2 +- > 8 files changed, 100 insertions(+), 31 deletions(-) > > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h > index 9bb44c8535..5bcc8a2db5 100644 > --- a/nptl/pthreadP.h > +++ b/nptl/pthreadP.h > @@ -464,6 +464,10 @@ extern int __pthread_cond_wait (pthread_cond_t > *cond, pthread_mutex_t *mutex); # define __pthread_timedjoin_np64 > __pthread_timedjoin_np # define __pthread_cond_timedwait64 > __pthread_cond_timedwait # define __pthread_cond_clockwait64 > __pthread_cond_clockwait +# define __pthread_rwlock_clockrdlock64 > __pthread_rwlock_clockrdlock +# define __pthread_rwlock_clockwrlock64 > __pthread_rwlock_clockwrlock +# define __pthread_rwlock_timedrdlock64 > __pthread_rwlock_timedrdlock +# define __pthread_rwlock_timedwrlock64 > __pthread_rwlock_timedwrlock #else > extern int __pthread_clockjoin_np64 (pthread_t threadid, void > **thread_return, clockid_t clockid, > @@ -481,6 +485,20 @@ extern int __pthread_cond_clockwait64 > (pthread_cond_t *cond, clockid_t clockid, > const struct __timespec64 > *abstime); libpthread_hidden_proto (__pthread_cond_clockwait64) > +extern int __pthread_rwlock_clockrdlock64 (pthread_rwlock_t *rwlock, > + clockid_t clockid, > + const struct __timespec64 > *abstime); +libpthread_hidden_proto (__pthread_rwlock_clockrdlock64) > +extern int __pthread_rwlock_clockwrlock64 (pthread_rwlock_t *rwlock, > + clockid_t clockid, > + const struct __timespec64 > *abstime); +libpthread_hidden_proto (__pthread_rwlock_clockwrlock64) > +extern int __pthread_rwlock_timedrdlock64 (pthread_rwlock_t *rwlock, > + const struct __timespec64 > *abstime); +libpthread_hidden_proto (__pthread_rwlock_timedrdlock64) > +extern int __pthread_rwlock_timedwrlock64 (pthread_rwlock_t *rwlock, > + const struct __timespec64 > *abstime); +libpthread_hidden_proto (__pthread_rwlock_timedwrlock64) > #endif > > extern int __pthread_cond_timedwait (pthread_cond_t *cond, > diff --git a/nptl/pthread_rwlock_clockrdlock.c > b/nptl/pthread_rwlock_clockrdlock.c index 4cedfd1dcd..d93b133c9e > 100644 --- a/nptl/pthread_rwlock_clockrdlock.c > +++ b/nptl/pthread_rwlock_clockrdlock.c > @@ -21,8 +21,21 @@ > > /* See pthread_rwlock_common.c. */ > int > -pthread_rwlock_clockrdlock (pthread_rwlock_t *rwlock, clockid_t > clockid, > - const struct timespec *abstime) > +__pthread_rwlock_clockrdlock64 (pthread_rwlock_t *rwlock, clockid_t > clockid, > + const struct __timespec64 *abstime) > { > - return __pthread_rwlock_rdlock_full (rwlock, clockid, abstime); > + return __pthread_rwlock_rdlock_full64 (rwlock, clockid, abstime); > } > + > +#if __TIMESIZE != 64 > +libpthread_hidden_def (__pthread_rwlock_clockrdlock64) > +int > +__pthread_rwlock_clockrdlock (pthread_rwlock_t *rwlock, clockid_t > clockid, > + const struct timespec *abstime) > +{ > + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); > + > + return __pthread_rwlock_clockrdlock64 (rwlock, clockid, &ts64); > +} > +#endif > +weak_alias (__pthread_rwlock_clockrdlock, pthread_rwlock_clockrdlock) > diff --git a/nptl/pthread_rwlock_clockwrlock.c > b/nptl/pthread_rwlock_clockwrlock.c index 7a954cf529..10a314c76d > 100644 --- a/nptl/pthread_rwlock_clockwrlock.c > +++ b/nptl/pthread_rwlock_clockwrlock.c > @@ -21,8 +21,21 @@ > > /* See pthread_rwlock_common.c. */ > int > -pthread_rwlock_clockwrlock (pthread_rwlock_t *rwlock, clockid_t > clockid, > - const struct timespec *abstime) > +__pthread_rwlock_clockwrlock64 (pthread_rwlock_t *rwlock, clockid_t > clockid, > + const struct __timespec64 *abstime) > { > - return __pthread_rwlock_wrlock_full (rwlock, clockid, abstime); > + return __pthread_rwlock_wrlock_full64 (rwlock, clockid, abstime); > } > + > +#if __TIMESIZE != 64 > +libpthread_hidden_def (__pthread_rwlock_clockwrlock64) > +int > +__pthread_rwlock_clockwrlock (pthread_rwlock_t *rwlock, clockid_t > clockid, > + const struct timespec *abstime) > +{ > + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); > + > + return __pthread_rwlock_clockwrlock64 (rwlock, clockid, &ts64); > +} > +#endif > +weak_alias (__pthread_rwlock_clockwrlock, pthread_rwlock_clockwrlock) > diff --git a/nptl/pthread_rwlock_common.c > b/nptl/pthread_rwlock_common.c index 3fbc66ded2..4c9f582d3d 100644 > --- a/nptl/pthread_rwlock_common.c > +++ b/nptl/pthread_rwlock_common.c > @@ -278,9 +278,8 @@ __pthread_rwlock_rdunlock (pthread_rwlock_t > *rwlock) > > static __always_inline int > -__pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, > - clockid_t clockid, > - const struct timespec *abstime) > +__pthread_rwlock_rdlock_full64 (pthread_rwlock_t *rwlock, clockid_t > clockid, > + const struct __timespec64 *abstime) > { > unsigned int r; > > @@ -330,8 +329,9 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t > *rwlock, & PTHREAD_RWLOCK_RWAITING) != 0) > { > int private = __pthread_rwlock_get_private > (rwlock); > - int err = futex_abstimed_wait > (&rwlock->__data.__readers, > - r, clockid, > abstime, private); > + int err = __futex_abstimed_wait64 > (&rwlock->__data.__readers, > + r, clockid, > abstime, > + private); > /* We ignore EAGAIN and EINTR. On time-outs, we > can just return because we don't need to clean up anything. */ > if (err == ETIMEDOUT) > @@ -457,9 +457,9 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t > *rwlock, (&rwlock->__data.__wrphase_futex, > &wpf, wpf | PTHREAD_RWLOCK_FUTEX_USED))) > continue; > - int err = futex_abstimed_wait > (&rwlock->__data.__wrphase_futex, > - 1 | > PTHREAD_RWLOCK_FUTEX_USED, > - clockid, abstime, private); > + int err = __futex_abstimed_wait64 > (&rwlock->__data.__wrphase_futex, > + 1 | > PTHREAD_RWLOCK_FUTEX_USED, > + clockid, abstime, > private); if (err == ETIMEDOUT) > { > /* If we timed out, we need to unregister. If no read > phase @@ -585,9 +585,8 @@ __pthread_rwlock_wrunlock (pthread_rwlock_t > *rwlock) > > static __always_inline int > -__pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, > - clockid_t clockid, > - const struct timespec *abstime) > +__pthread_rwlock_wrlock_full64 (pthread_rwlock_t *rwlock, clockid_t > clockid, > + const struct __timespec64 *abstime) > { > /* Make sure any passed in clockid and timeout value are valid. > Note that the previous implementation assumed that this check *must* > not be @@ -728,9 +727,9 @@ __pthread_rwlock_wrlock_full > (pthread_rwlock_t *rwlock, share the flag, and another writer will > wake one of the writers in this group. */ > may_share_futex_used_flag = true; > - int err = futex_abstimed_wait > (&rwlock->__data.__writers_futex, > - 1 | > PTHREAD_RWLOCK_FUTEX_USED, > - clockid, abstime, private); > + int err = __futex_abstimed_wait64 > (&rwlock->__data.__writers_futex, > + 1 | > PTHREAD_RWLOCK_FUTEX_USED, > + clockid, abstime, > private); if (err == ETIMEDOUT) > { > if (prefer_writer) > @@ -827,9 +826,9 @@ __pthread_rwlock_wrlock_full (pthread_rwlock_t > *rwlock, (&rwlock->__data.__wrphase_futex, &wpf, > PTHREAD_RWLOCK_FUTEX_USED))) > continue; > - int err = futex_abstimed_wait > (&rwlock->__data.__wrphase_futex, > - PTHREAD_RWLOCK_FUTEX_USED, > - clockid, abstime, private); > + int err = __futex_abstimed_wait64 > (&rwlock->__data.__wrphase_futex, > + > PTHREAD_RWLOCK_FUTEX_USED, > + clockid, abstime, > private); if (err == ETIMEDOUT) > { > if (rwlock->__data.__flags != > PTHREAD_RWLOCK_PREFER_READER_NP) diff --git > a/nptl/pthread_rwlock_rdlock.c b/nptl/pthread_rwlock_rdlock.c index > 04cecad395..2b8509a49c 100644 --- a/nptl/pthread_rwlock_rdlock.c > +++ b/nptl/pthread_rwlock_rdlock.c > @@ -24,7 +24,7 @@ __pthread_rwlock_rdlock (pthread_rwlock_t *rwlock) > { > LIBC_PROBE (rdlock_entry, 1, rwlock); > > - int result = __pthread_rwlock_rdlock_full (rwlock, CLOCK_REALTIME, > NULL); > + int result = __pthread_rwlock_rdlock_full64 (rwlock, > CLOCK_REALTIME, NULL); LIBC_PROBE (rdlock_acquire_read, 1, rwlock); > return result; > } > diff --git a/nptl/pthread_rwlock_timedrdlock.c > b/nptl/pthread_rwlock_timedrdlock.c index c5d8aee909..d5999a1419 > 100644 --- a/nptl/pthread_rwlock_timedrdlock.c > +++ b/nptl/pthread_rwlock_timedrdlock.c > @@ -20,8 +20,21 @@ > > /* See pthread_rwlock_common.c. */ > int > -pthread_rwlock_timedrdlock (pthread_rwlock_t *rwlock, > - const struct timespec *abstime) > +__pthread_rwlock_timedrdlock64 (pthread_rwlock_t *rwlock, > + const struct __timespec64 *abstime) > { > - return __pthread_rwlock_rdlock_full (rwlock, CLOCK_REALTIME, > abstime); > + return __pthread_rwlock_rdlock_full64 (rwlock, CLOCK_REALTIME, > abstime); } > + > +#if __TIMESIZE != 64 > +libpthread_hidden_def (__pthread_rwlock_timedrdlock64) > +int > +__pthread_rwlock_timedrdlock (pthread_rwlock_t *rwlock, > + const struct timespec *abstime) > +{ > + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); > + > + return __pthread_rwlock_timedrdlock64 (rwlock, &ts64); > +} > +#endif > +weak_alias (__pthread_rwlock_timedrdlock, pthread_rwlock_timedrdlock) > diff --git a/nptl/pthread_rwlock_timedwrlock.c > b/nptl/pthread_rwlock_timedwrlock.c index ccee8b77d9..6c5284a172 > 100644 --- a/nptl/pthread_rwlock_timedwrlock.c > +++ b/nptl/pthread_rwlock_timedwrlock.c > @@ -20,8 +20,21 @@ > > /* See pthread_rwlock_common.c. */ > int > -pthread_rwlock_timedwrlock (pthread_rwlock_t *rwlock, > - const struct timespec *abstime) > +__pthread_rwlock_timedwrlock64 (pthread_rwlock_t *rwlock, > + const struct __timespec64 *abstime) > { > - return __pthread_rwlock_wrlock_full (rwlock, CLOCK_REALTIME, > abstime); > + return __pthread_rwlock_wrlock_full64 (rwlock, CLOCK_REALTIME, > abstime); } > + > +#if __TIMESIZE != 64 > +libpthread_hidden_def (__pthread_rwlock_timedwrlock64) > +int > +__pthread_rwlock_timedwrlock (pthread_rwlock_t *rwlock, > + const struct timespec *abstime) > +{ > + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); > + > + return __pthread_rwlock_timedwrlock64 (rwlock, &ts64); > +} > +#endif > +weak_alias (__pthread_rwlock_timedwrlock, pthread_rwlock_timedwrlock) > diff --git a/nptl/pthread_rwlock_wrlock.c > b/nptl/pthread_rwlock_wrlock.c index a42aa626f0..210e6cffdc 100644 > --- a/nptl/pthread_rwlock_wrlock.c > +++ b/nptl/pthread_rwlock_wrlock.c > @@ -24,7 +24,7 @@ __pthread_rwlock_wrlock (pthread_rwlock_t *rwlock) > { > LIBC_PROBE (wrlock_entry, 1, rwlock); > > - int result = __pthread_rwlock_wrlock_full (rwlock, CLOCK_REALTIME, > NULL); > + int result = __pthread_rwlock_wrlock_full64 (rwlock, > CLOCK_REALTIME, NULL); LIBC_PROBE (wrlock_acquire_write, 1, rwlock); > return result; > } 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
On 19/09/2020 10:07, Lukasz Majewski wrote: > The pthread_rwlock_clockrdlock, pthread_rwlock_clockwrlock, > pthread_rwlock_timedrdlock and pthread_rwlock_timedwrlock have been converted > to support 64 bit time. > > This change uses new futex_abstimed_wait64 function in > ./sysdeps/nptl/futex-helpers.c, which uses futex_time64 where possible. > > The pthread_rwlock_{clock|timed}{rd|wr}lock only accepts absolute time. > Moreover, there is no need to check for NULL passed as *abstime pointer to the > syscalls as those calls have exported symbols marked with __nonull attribute > for abstime. > > For systems with __TIMESIZE != 64 && __WORDSIZE == 32: > - Conversions between 64 bit time to 32 bit are necessary > - Redirection to pthread_rwlock_{clock|timed}{rd|wr}lock will provide support > for 64 bit time > > Build tests: > ./src/scripts/build-many-glibcs.py glibcs > > Run-time tests: > - Run specific tests on ARM/x86 32bit systems (qemu): > https://github.com/lmajewski/meta-y2038 and run tests: > https://github.com/lmajewski/y2038-tests/commits/master > > Above tests were performed with Y2038 redirection applied as well as without > to test the proper usage of both __pthread_rwlock_{clock|timed}{rd|wr}lock64 > and __pthread_rwlock_{clock|timed}{rd|wr}lock. > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Looks good, there is only a issue regarding LIBC_PROBE (below). > > --- > Changes for v2: > - None > --- > nptl/pthreadP.h | 18 +++++++++++++++++ > nptl/pthread_rwlock_clockrdlock.c | 19 +++++++++++++++--- > nptl/pthread_rwlock_clockwrlock.c | 19 +++++++++++++++--- > nptl/pthread_rwlock_common.c | 33 +++++++++++++++---------------- > nptl/pthread_rwlock_rdlock.c | 2 +- > nptl/pthread_rwlock_timedrdlock.c | 19 +++++++++++++++--- > nptl/pthread_rwlock_timedwrlock.c | 19 +++++++++++++++--- > nptl/pthread_rwlock_wrlock.c | 2 +- > 8 files changed, 100 insertions(+), 31 deletions(-) > > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h > index 9bb44c8535..5bcc8a2db5 100644 > --- a/nptl/pthreadP.h > +++ b/nptl/pthreadP.h > @@ -464,6 +464,10 @@ extern int __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex); > # define __pthread_timedjoin_np64 __pthread_timedjoin_np > # define __pthread_cond_timedwait64 __pthread_cond_timedwait > # define __pthread_cond_clockwait64 __pthread_cond_clockwait > +# define __pthread_rwlock_clockrdlock64 __pthread_rwlock_clockrdlock > +# define __pthread_rwlock_clockwrlock64 __pthread_rwlock_clockwrlock > +# define __pthread_rwlock_timedrdlock64 __pthread_rwlock_timedrdlock > +# define __pthread_rwlock_timedwrlock64 __pthread_rwlock_timedwrlock > #else > extern int __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return, > clockid_t clockid, > @@ -481,6 +485,20 @@ extern int __pthread_cond_clockwait64 (pthread_cond_t *cond, > clockid_t clockid, > const struct __timespec64 *abstime); > libpthread_hidden_proto (__pthread_cond_clockwait64) > +extern int __pthread_rwlock_clockrdlock64 (pthread_rwlock_t *rwlock, > + clockid_t clockid, > + const struct __timespec64 *abstime); > +libpthread_hidden_proto (__pthread_rwlock_clockrdlock64) > +extern int __pthread_rwlock_clockwrlock64 (pthread_rwlock_t *rwlock, > + clockid_t clockid, > + const struct __timespec64 *abstime); > +libpthread_hidden_proto (__pthread_rwlock_clockwrlock64) > +extern int __pthread_rwlock_timedrdlock64 (pthread_rwlock_t *rwlock, > + const struct __timespec64 *abstime); > +libpthread_hidden_proto (__pthread_rwlock_timedrdlock64) > +extern int __pthread_rwlock_timedwrlock64 (pthread_rwlock_t *rwlock, > + const struct __timespec64 *abstime); > +libpthread_hidden_proto (__pthread_rwlock_timedwrlock64) > #endif > > extern int __pthread_cond_timedwait (pthread_cond_t *cond, Ok. > diff --git a/nptl/pthread_rwlock_clockrdlock.c b/nptl/pthread_rwlock_clockrdlock.c > index 4cedfd1dcd..d93b133c9e 100644 > --- a/nptl/pthread_rwlock_clockrdlock.c > +++ b/nptl/pthread_rwlock_clockrdlock.c > @@ -21,8 +21,21 @@ > > /* See pthread_rwlock_common.c. */ > int > -pthread_rwlock_clockrdlock (pthread_rwlock_t *rwlock, clockid_t clockid, > - const struct timespec *abstime) > +__pthread_rwlock_clockrdlock64 (pthread_rwlock_t *rwlock, clockid_t clockid, > + const struct __timespec64 *abstime) > { > - return __pthread_rwlock_rdlock_full (rwlock, clockid, abstime); > + return __pthread_rwlock_rdlock_full64 (rwlock, clockid, abstime); > } > + > +#if __TIMESIZE != 64 > +libpthread_hidden_def (__pthread_rwlock_clockrdlock64) Maybe an extra space here? > +int > +__pthread_rwlock_clockrdlock (pthread_rwlock_t *rwlock, clockid_t clockid, > + const struct timespec *abstime) > +{ > + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); > + > + return __pthread_rwlock_clockrdlock64 (rwlock, clockid, &ts64); > +} > +#endif > +weak_alias (__pthread_rwlock_clockrdlock, pthread_rwlock_clockrdlock) Ok. > diff --git a/nptl/pthread_rwlock_clockwrlock.c b/nptl/pthread_rwlock_clockwrlock.c > index 7a954cf529..10a314c76d 100644 > --- a/nptl/pthread_rwlock_clockwrlock.c > +++ b/nptl/pthread_rwlock_clockwrlock.c > @@ -21,8 +21,21 @@ > > /* See pthread_rwlock_common.c. */ > int > -pthread_rwlock_clockwrlock (pthread_rwlock_t *rwlock, clockid_t clockid, > - const struct timespec *abstime) > +__pthread_rwlock_clockwrlock64 (pthread_rwlock_t *rwlock, clockid_t clockid, > + const struct __timespec64 *abstime) > { > - return __pthread_rwlock_wrlock_full (rwlock, clockid, abstime); > + return __pthread_rwlock_wrlock_full64 (rwlock, clockid, abstime); > } > + Ok. > +#if __TIMESIZE != 64 > +libpthread_hidden_def (__pthread_rwlock_clockwrlock64) Maybe an extra space here? > +int > +__pthread_rwlock_clockwrlock (pthread_rwlock_t *rwlock, clockid_t clockid, > + const struct timespec *abstime) > +{ > + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); > + > + return __pthread_rwlock_clockwrlock64 (rwlock, clockid, &ts64); > +} > +#endif > +weak_alias (__pthread_rwlock_clockwrlock, pthread_rwlock_clockwrlock) Ok. > diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c > index 3fbc66ded2..4c9f582d3d 100644 > --- a/nptl/pthread_rwlock_common.c > +++ b/nptl/pthread_rwlock_common.c > @@ -278,9 +278,8 @@ __pthread_rwlock_rdunlock (pthread_rwlock_t *rwlock) > > > static __always_inline int > -__pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, > - clockid_t clockid, > - const struct timespec *abstime) > +__pthread_rwlock_rdlock_full64 (pthread_rwlock_t *rwlock, clockid_t clockid, > + const struct __timespec64 *abstime) > { > unsigned int r; > Ok. > @@ -330,8 +329,9 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, > & PTHREAD_RWLOCK_RWAITING) != 0) > { > int private = __pthread_rwlock_get_private (rwlock); > - int err = futex_abstimed_wait (&rwlock->__data.__readers, > - r, clockid, abstime, private); > + int err = __futex_abstimed_wait64 (&rwlock->__data.__readers, > + r, clockid, abstime, > + private); > /* We ignore EAGAIN and EINTR. On time-outs, we can just > return because we don't need to clean up anything. */ > if (err == ETIMEDOUT) Ok. > @@ -457,9 +457,9 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, > (&rwlock->__data.__wrphase_futex, > &wpf, wpf | PTHREAD_RWLOCK_FUTEX_USED))) > continue; > - int err = futex_abstimed_wait (&rwlock->__data.__wrphase_futex, > - 1 | PTHREAD_RWLOCK_FUTEX_USED, > - clockid, abstime, private); > + int err = __futex_abstimed_wait64 (&rwlock->__data.__wrphase_futex, > + 1 | PTHREAD_RWLOCK_FUTEX_USED, > + clockid, abstime, private); > if (err == ETIMEDOUT) > { > /* If we timed out, we need to unregister. If no read phase Ok. > @@ -585,9 +585,8 @@ __pthread_rwlock_wrunlock (pthread_rwlock_t *rwlock) > > > static __always_inline int > -__pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, > - clockid_t clockid, > - const struct timespec *abstime) > +__pthread_rwlock_wrlock_full64 (pthread_rwlock_t *rwlock, clockid_t clockid, > + const struct __timespec64 *abstime) > { > /* Make sure any passed in clockid and timeout value are valid. Note that > the previous implementation assumed that this check *must* not be > @@ -728,9 +727,9 @@ __pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, > share the flag, and another writer will wake one of the writers > in this group. */ > may_share_futex_used_flag = true; > - int err = futex_abstimed_wait (&rwlock->__data.__writers_futex, > - 1 | PTHREAD_RWLOCK_FUTEX_USED, > - clockid, abstime, private); > + int err = __futex_abstimed_wait64 (&rwlock->__data.__writers_futex, > + 1 | PTHREAD_RWLOCK_FUTEX_USED, > + clockid, abstime, private); > if (err == ETIMEDOUT) > { > if (prefer_writer) Ok. > @@ -827,9 +826,9 @@ __pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, > (&rwlock->__data.__wrphase_futex, &wpf, > PTHREAD_RWLOCK_FUTEX_USED))) > continue; > - int err = futex_abstimed_wait (&rwlock->__data.__wrphase_futex, > - PTHREAD_RWLOCK_FUTEX_USED, > - clockid, abstime, private); > + int err = __futex_abstimed_wait64 (&rwlock->__data.__wrphase_futex, > + PTHREAD_RWLOCK_FUTEX_USED, > + clockid, abstime, private); > if (err == ETIMEDOUT) > { > if (rwlock->__data.__flags != PTHREAD_RWLOCK_PREFER_READER_NP) Ok. > diff --git a/nptl/pthread_rwlock_rdlock.c b/nptl/pthread_rwlock_rdlock.c > index 04cecad395..2b8509a49c 100644 > --- a/nptl/pthread_rwlock_rdlock.c > +++ b/nptl/pthread_rwlock_rdlock.c > @@ -24,7 +24,7 @@ __pthread_rwlock_rdlock (pthread_rwlock_t *rwlock) > { > LIBC_PROBE (rdlock_entry, 1, rwlock); We need to move the systemtap probe to the 64-bit variant, so the 64-bit time will still trigger it. > > - int result = __pthread_rwlock_rdlock_full (rwlock, CLOCK_REALTIME, NULL); > + int result = __pthread_rwlock_rdlock_full64 (rwlock, CLOCK_REALTIME, NULL); > LIBC_PROBE (rdlock_acquire_read, 1, rwlock); > return result; > } Ok. > diff --git a/nptl/pthread_rwlock_timedrdlock.c b/nptl/pthread_rwlock_timedrdlock.c > index c5d8aee909..d5999a1419 100644 > --- a/nptl/pthread_rwlock_timedrdlock.c > +++ b/nptl/pthread_rwlock_timedrdlock.c > @@ -20,8 +20,21 @@ > > /* See pthread_rwlock_common.c. */ > int > -pthread_rwlock_timedrdlock (pthread_rwlock_t *rwlock, > - const struct timespec *abstime) > +__pthread_rwlock_timedrdlock64 (pthread_rwlock_t *rwlock, > + const struct __timespec64 *abstime) > { > - return __pthread_rwlock_rdlock_full (rwlock, CLOCK_REALTIME, abstime); > + return __pthread_rwlock_rdlock_full64 (rwlock, CLOCK_REALTIME, abstime); > } > + Ok. > +#if __TIMESIZE != 64 > +libpthread_hidden_def (__pthread_rwlock_timedrdlock64) Maybe an extra space here? > +int > +__pthread_rwlock_timedrdlock (pthread_rwlock_t *rwlock, > + const struct timespec *abstime) > +{ > + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); > + > + return __pthread_rwlock_timedrdlock64 (rwlock, &ts64); > +} > +#endif > +weak_alias (__pthread_rwlock_timedrdlock, pthread_rwlock_timedrdlock) Ok. > diff --git a/nptl/pthread_rwlock_timedwrlock.c b/nptl/pthread_rwlock_timedwrlock.c > index ccee8b77d9..6c5284a172 100644 > --- a/nptl/pthread_rwlock_timedwrlock.c > +++ b/nptl/pthread_rwlock_timedwrlock.c > @@ -20,8 +20,21 @@ > > /* See pthread_rwlock_common.c. */ > int > -pthread_rwlock_timedwrlock (pthread_rwlock_t *rwlock, > - const struct timespec *abstime) > +__pthread_rwlock_timedwrlock64 (pthread_rwlock_t *rwlock, > + const struct __timespec64 *abstime) > { > - return __pthread_rwlock_wrlock_full (rwlock, CLOCK_REALTIME, abstime); > + return __pthread_rwlock_wrlock_full64 (rwlock, CLOCK_REALTIME, abstime); > } > + > +#if __TIMESIZE != 64 > +libpthread_hidden_def (__pthread_rwlock_timedwrlock64) Maybe an extra space here? > +int > +__pthread_rwlock_timedwrlock (pthread_rwlock_t *rwlock, > + const struct timespec *abstime) > +{ > + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); > + > + return __pthread_rwlock_timedwrlock64 (rwlock, &ts64); > +} > +#endif > +weak_alias (__pthread_rwlock_timedwrlock, pthread_rwlock_timedwrlock) Ok. > diff --git a/nptl/pthread_rwlock_wrlock.c b/nptl/pthread_rwlock_wrlock.c > index a42aa626f0..210e6cffdc 100644 > --- a/nptl/pthread_rwlock_wrlock.c > +++ b/nptl/pthread_rwlock_wrlock.c > @@ -24,7 +24,7 @@ __pthread_rwlock_wrlock (pthread_rwlock_t *rwlock) > { > LIBC_PROBE (wrlock_entry, 1, rwlock); We need to move the systemtap probe to the 64-bit variant, so the 64-bit time will still trigger it. > > - int result = __pthread_rwlock_wrlock_full (rwlock, CLOCK_REALTIME, NULL); > + int result = __pthread_rwlock_wrlock_full64 (rwlock, CLOCK_REALTIME, NULL); > LIBC_PROBE (wrlock_acquire_write, 1, rwlock); > return result; > } > Ok.
Hi Adhemerval, > On 19/09/2020 10:07, Lukasz Majewski wrote: > > The pthread_rwlock_clockrdlock, pthread_rwlock_clockwrlock, > > pthread_rwlock_timedrdlock and pthread_rwlock_timedwrlock have been > > converted to support 64 bit time. > > > > This change uses new futex_abstimed_wait64 function in > > ./sysdeps/nptl/futex-helpers.c, which uses futex_time64 where > > possible. > > > > The pthread_rwlock_{clock|timed}{rd|wr}lock only accepts absolute > > time. Moreover, there is no need to check for NULL passed as > > *abstime pointer to the syscalls as those calls have exported > > symbols marked with __nonull attribute for abstime. > > > > For systems with __TIMESIZE != 64 && __WORDSIZE == 32: > > - Conversions between 64 bit time to 32 bit are necessary > > - Redirection to pthread_rwlock_{clock|timed}{rd|wr}lock will > > provide support for 64 bit time > > > > Build tests: > > ./src/scripts/build-many-glibcs.py glibcs > > > > Run-time tests: > > - Run specific tests on ARM/x86 32bit systems (qemu): > > https://github.com/lmajewski/meta-y2038 and run tests: > > https://github.com/lmajewski/y2038-tests/commits/master > > > > Above tests were performed with Y2038 redirection applied as well > > as without to test the proper usage of both > > __pthread_rwlock_{clock|timed}{rd|wr}lock64 and > > __pthread_rwlock_{clock|timed}{rd|wr}lock. > > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > Looks good, there is only a issue regarding LIBC_PROBE (below). > > > > > > --- > > Changes for v2: > > - None > > --- > > nptl/pthreadP.h | 18 +++++++++++++++++ > > nptl/pthread_rwlock_clockrdlock.c | 19 +++++++++++++++--- > > nptl/pthread_rwlock_clockwrlock.c | 19 +++++++++++++++--- > > nptl/pthread_rwlock_common.c | 33 > > +++++++++++++++---------------- nptl/pthread_rwlock_rdlock.c | > > 2 +- nptl/pthread_rwlock_timedrdlock.c | 19 +++++++++++++++--- > > nptl/pthread_rwlock_timedwrlock.c | 19 +++++++++++++++--- > > nptl/pthread_rwlock_wrlock.c | 2 +- > > 8 files changed, 100 insertions(+), 31 deletions(-) > > > > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h > > index 9bb44c8535..5bcc8a2db5 100644 > > --- a/nptl/pthreadP.h > > +++ b/nptl/pthreadP.h > > @@ -464,6 +464,10 @@ extern int __pthread_cond_wait (pthread_cond_t > > *cond, pthread_mutex_t *mutex); # define __pthread_timedjoin_np64 > > __pthread_timedjoin_np # define __pthread_cond_timedwait64 > > __pthread_cond_timedwait # define __pthread_cond_clockwait64 > > __pthread_cond_clockwait +# define __pthread_rwlock_clockrdlock64 > > __pthread_rwlock_clockrdlock +# define > > __pthread_rwlock_clockwrlock64 __pthread_rwlock_clockwrlock +# > > define __pthread_rwlock_timedrdlock64 __pthread_rwlock_timedrdlock > > +# define __pthread_rwlock_timedwrlock64 > > __pthread_rwlock_timedwrlock #else extern int > > __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return, > > clockid_t clockid, @@ -481,6 +485,20 @@ extern int > > __pthread_cond_clockwait64 (pthread_cond_t *cond, clockid_t clockid, > > const struct __timespec64 > > *abstime); libpthread_hidden_proto (__pthread_cond_clockwait64) > > +extern int __pthread_rwlock_clockrdlock64 (pthread_rwlock_t > > *rwlock, > > + clockid_t clockid, > > + const struct > > __timespec64 *abstime); +libpthread_hidden_proto > > (__pthread_rwlock_clockrdlock64) +extern int > > __pthread_rwlock_clockwrlock64 (pthread_rwlock_t *rwlock, > > + clockid_t clockid, > > + const struct > > __timespec64 *abstime); +libpthread_hidden_proto > > (__pthread_rwlock_clockwrlock64) +extern int > > __pthread_rwlock_timedrdlock64 (pthread_rwlock_t *rwlock, > > + const struct > > __timespec64 *abstime); +libpthread_hidden_proto > > (__pthread_rwlock_timedrdlock64) +extern int > > __pthread_rwlock_timedwrlock64 (pthread_rwlock_t *rwlock, > > + const struct > > __timespec64 *abstime); +libpthread_hidden_proto > > (__pthread_rwlock_timedwrlock64) #endif > > > > extern int __pthread_cond_timedwait (pthread_cond_t *cond, > > Ok. > > > diff --git a/nptl/pthread_rwlock_clockrdlock.c > > b/nptl/pthread_rwlock_clockrdlock.c index 4cedfd1dcd..d93b133c9e > > 100644 --- a/nptl/pthread_rwlock_clockrdlock.c > > +++ b/nptl/pthread_rwlock_clockrdlock.c > > @@ -21,8 +21,21 @@ > > > > /* See pthread_rwlock_common.c. */ > > int > > -pthread_rwlock_clockrdlock (pthread_rwlock_t *rwlock, clockid_t > > clockid, > > - const struct timespec *abstime) > > +__pthread_rwlock_clockrdlock64 (pthread_rwlock_t *rwlock, > > clockid_t clockid, > > + const struct __timespec64 *abstime) > > { > > - return __pthread_rwlock_rdlock_full (rwlock, clockid, abstime); > > + return __pthread_rwlock_rdlock_full64 (rwlock, clockid, abstime); > > } > > + > > +#if __TIMESIZE != 64 > > +libpthread_hidden_def (__pthread_rwlock_clockrdlock64) > > Maybe an extra space here? Ok. Fixed. > > > +int > > +__pthread_rwlock_clockrdlock (pthread_rwlock_t *rwlock, clockid_t > > clockid, > > + const struct timespec *abstime) > > +{ > > + struct __timespec64 ts64 = valid_timespec_to_timespec64 > > (*abstime); + > > + return __pthread_rwlock_clockrdlock64 (rwlock, clockid, &ts64); > > +} > > +#endif > > +weak_alias (__pthread_rwlock_clockrdlock, > > pthread_rwlock_clockrdlock) > > Ok. > > > diff --git a/nptl/pthread_rwlock_clockwrlock.c > > b/nptl/pthread_rwlock_clockwrlock.c index 7a954cf529..10a314c76d > > 100644 --- a/nptl/pthread_rwlock_clockwrlock.c > > +++ b/nptl/pthread_rwlock_clockwrlock.c > > @@ -21,8 +21,21 @@ > > > > /* See pthread_rwlock_common.c. */ > > int > > -pthread_rwlock_clockwrlock (pthread_rwlock_t *rwlock, clockid_t > > clockid, > > - const struct timespec *abstime) > > +__pthread_rwlock_clockwrlock64 (pthread_rwlock_t *rwlock, > > clockid_t clockid, > > + const struct __timespec64 *abstime) > > { > > - return __pthread_rwlock_wrlock_full (rwlock, clockid, abstime); > > + return __pthread_rwlock_wrlock_full64 (rwlock, clockid, abstime); > > } > > + > > Ok. > > > +#if __TIMESIZE != 64 > > +libpthread_hidden_def (__pthread_rwlock_clockwrlock64) > > Maybe an extra space here? Ok. Fixed. > > > +int > > +__pthread_rwlock_clockwrlock (pthread_rwlock_t *rwlock, clockid_t > > clockid, > > + const struct timespec *abstime) > > +{ > > + struct __timespec64 ts64 = valid_timespec_to_timespec64 > > (*abstime); + > > + return __pthread_rwlock_clockwrlock64 (rwlock, clockid, &ts64); > > +} > > +#endif > > +weak_alias (__pthread_rwlock_clockwrlock, > > pthread_rwlock_clockwrlock) > > Ok. > > > diff --git a/nptl/pthread_rwlock_common.c > > b/nptl/pthread_rwlock_common.c index 3fbc66ded2..4c9f582d3d 100644 > > --- a/nptl/pthread_rwlock_common.c > > +++ b/nptl/pthread_rwlock_common.c > > @@ -278,9 +278,8 @@ __pthread_rwlock_rdunlock (pthread_rwlock_t > > *rwlock) > > > > static __always_inline int > > -__pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, > > - clockid_t clockid, > > - const struct timespec *abstime) > > +__pthread_rwlock_rdlock_full64 (pthread_rwlock_t *rwlock, > > clockid_t clockid, > > + const struct __timespec64 *abstime) > > { > > unsigned int r; > > > > Ok. > > > @@ -330,8 +329,9 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t > > *rwlock, & PTHREAD_RWLOCK_RWAITING) != 0) > > { > > int private = __pthread_rwlock_get_private > > (rwlock); > > - int err = futex_abstimed_wait > > (&rwlock->__data.__readers, > > - r, clockid, > > abstime, private); > > + int err = __futex_abstimed_wait64 > > (&rwlock->__data.__readers, > > + r, clockid, > > abstime, > > + private); > > /* We ignore EAGAIN and EINTR. On time-outs, we > > can just return because we don't need to clean up anything. */ > > if (err == ETIMEDOUT) > > Ok. > > > @@ -457,9 +457,9 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t > > *rwlock, (&rwlock->__data.__wrphase_futex, > > &wpf, wpf | PTHREAD_RWLOCK_FUTEX_USED))) > > continue; > > - int err = futex_abstimed_wait > > (&rwlock->__data.__wrphase_futex, > > - 1 | > > PTHREAD_RWLOCK_FUTEX_USED, > > - clockid, abstime, > > private); > > + int err = __futex_abstimed_wait64 > > (&rwlock->__data.__wrphase_futex, > > + 1 | > > PTHREAD_RWLOCK_FUTEX_USED, > > + clockid, abstime, > > private); if (err == ETIMEDOUT) > > { > > /* If we timed out, we need to unregister. If no > > read phase > > Ok. > > > @@ -585,9 +585,8 @@ __pthread_rwlock_wrunlock (pthread_rwlock_t > > *rwlock) > > > > static __always_inline int > > -__pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, > > - clockid_t clockid, > > - const struct timespec *abstime) > > +__pthread_rwlock_wrlock_full64 (pthread_rwlock_t *rwlock, > > clockid_t clockid, > > + const struct __timespec64 *abstime) > > { > > /* Make sure any passed in clockid and timeout value are valid. > > Note that the previous implementation assumed that this check > > *must* not be @@ -728,9 +727,9 @@ __pthread_rwlock_wrlock_full > > (pthread_rwlock_t *rwlock, share the flag, and another writer will > > wake one of the writers in this group. */ > > may_share_futex_used_flag = true; > > - int err = futex_abstimed_wait > > (&rwlock->__data.__writers_futex, > > - 1 | > > PTHREAD_RWLOCK_FUTEX_USED, > > - clockid, abstime, > > private); > > + int err = __futex_abstimed_wait64 > > (&rwlock->__data.__writers_futex, > > + 1 | > > PTHREAD_RWLOCK_FUTEX_USED, > > + clockid, abstime, > > private); if (err == ETIMEDOUT) > > { > > if (prefer_writer) > > Ok. > > > @@ -827,9 +826,9 @@ __pthread_rwlock_wrlock_full (pthread_rwlock_t > > *rwlock, (&rwlock->__data.__wrphase_futex, &wpf, > > PTHREAD_RWLOCK_FUTEX_USED))) > > continue; > > - int err = futex_abstimed_wait > > (&rwlock->__data.__wrphase_futex, > > - PTHREAD_RWLOCK_FUTEX_USED, > > - clockid, abstime, > > private); > > + int err = __futex_abstimed_wait64 > > (&rwlock->__data.__wrphase_futex, > > + > > PTHREAD_RWLOCK_FUTEX_USED, > > + clockid, abstime, > > private); if (err == ETIMEDOUT) > > { > > if (rwlock->__data.__flags != > > PTHREAD_RWLOCK_PREFER_READER_NP) > > Ok. > > > diff --git a/nptl/pthread_rwlock_rdlock.c > > b/nptl/pthread_rwlock_rdlock.c index 04cecad395..2b8509a49c 100644 > > --- a/nptl/pthread_rwlock_rdlock.c > > +++ b/nptl/pthread_rwlock_rdlock.c > > @@ -24,7 +24,7 @@ __pthread_rwlock_rdlock (pthread_rwlock_t *rwlock) > > { > > LIBC_PROBE (rdlock_entry, 1, rwlock); > > We need to move the systemtap probe to the 64-bit variant, so the > 64-bit time will still trigger it. Those are syscalls, which don't require timespec time. The LIBC_PROBE() macros were there before, so I left them as is and only changed __pthread_rwlock_rdlock_full() to __pthread_rwlock_rdlock_full64(). On the other hand - for example: __pthread_rwlock_clockwrlock() and __pthread_rwlock_clockwrlock64() are only calling __pthread_rwlock_wrlock_full64(), and there were no LIBC_PROBE() macros before. Do I understand correctly that you propose to add LIBC_PROBE() macros to e.g. __pthread_rwlock_clockwrlock64() ? > > > > > - int result = __pthread_rwlock_rdlock_full (rwlock, > > CLOCK_REALTIME, NULL); > > + int result = __pthread_rwlock_rdlock_full64 (rwlock, > > CLOCK_REALTIME, NULL); LIBC_PROBE (rdlock_acquire_read, 1, rwlock); > > return result; > > } > > Ok. > > > diff --git a/nptl/pthread_rwlock_timedrdlock.c > > b/nptl/pthread_rwlock_timedrdlock.c index c5d8aee909..d5999a1419 > > 100644 --- a/nptl/pthread_rwlock_timedrdlock.c > > +++ b/nptl/pthread_rwlock_timedrdlock.c > > @@ -20,8 +20,21 @@ > > > > /* See pthread_rwlock_common.c. */ > > int > > -pthread_rwlock_timedrdlock (pthread_rwlock_t *rwlock, > > - const struct timespec *abstime) > > +__pthread_rwlock_timedrdlock64 (pthread_rwlock_t *rwlock, > > + const struct __timespec64 *abstime) > > { > > - return __pthread_rwlock_rdlock_full (rwlock, CLOCK_REALTIME, > > abstime); > > + return __pthread_rwlock_rdlock_full64 (rwlock, CLOCK_REALTIME, > > abstime); } > > + > > Ok. > > > +#if __TIMESIZE != 64 > > +libpthread_hidden_def (__pthread_rwlock_timedrdlock64) > > Maybe an extra space here? Ok. Fixed. > > > +int > > +__pthread_rwlock_timedrdlock (pthread_rwlock_t *rwlock, > > + const struct timespec *abstime) > > +{ > > + struct __timespec64 ts64 = valid_timespec_to_timespec64 > > (*abstime); + > > + return __pthread_rwlock_timedrdlock64 (rwlock, &ts64); > > +} > > +#endif > > +weak_alias (__pthread_rwlock_timedrdlock, > > pthread_rwlock_timedrdlock) > > Ok. > > > diff --git a/nptl/pthread_rwlock_timedwrlock.c > > b/nptl/pthread_rwlock_timedwrlock.c index ccee8b77d9..6c5284a172 > > 100644 --- a/nptl/pthread_rwlock_timedwrlock.c > > +++ b/nptl/pthread_rwlock_timedwrlock.c > > @@ -20,8 +20,21 @@ > > > > /* See pthread_rwlock_common.c. */ > > int > > -pthread_rwlock_timedwrlock (pthread_rwlock_t *rwlock, > > - const struct timespec *abstime) > > +__pthread_rwlock_timedwrlock64 (pthread_rwlock_t *rwlock, > > + const struct __timespec64 *abstime) > > { > > - return __pthread_rwlock_wrlock_full (rwlock, CLOCK_REALTIME, > > abstime); > > + return __pthread_rwlock_wrlock_full64 (rwlock, CLOCK_REALTIME, > > abstime); } > > + > > +#if __TIMESIZE != 64 > > +libpthread_hidden_def (__pthread_rwlock_timedwrlock64) > > > Maybe an extra space here? Ok. Fixed. > > > +int > > +__pthread_rwlock_timedwrlock (pthread_rwlock_t *rwlock, > > + const struct timespec *abstime) > > +{ > > + struct __timespec64 ts64 = valid_timespec_to_timespec64 > > (*abstime); + > > + return __pthread_rwlock_timedwrlock64 (rwlock, &ts64); > > +} > > +#endif > > +weak_alias (__pthread_rwlock_timedwrlock, > > pthread_rwlock_timedwrlock) > > Ok. > > > diff --git a/nptl/pthread_rwlock_wrlock.c > > b/nptl/pthread_rwlock_wrlock.c index a42aa626f0..210e6cffdc 100644 > > --- a/nptl/pthread_rwlock_wrlock.c > > +++ b/nptl/pthread_rwlock_wrlock.c > > @@ -24,7 +24,7 @@ __pthread_rwlock_wrlock (pthread_rwlock_t *rwlock) > > { > > LIBC_PROBE (wrlock_entry, 1, rwlock); > > We need to move the systemtap probe to the 64-bit variant, so the > 64-bit time will still trigger it. The same comment as above apply here. > > > > > - int result = __pthread_rwlock_wrlock_full (rwlock, > > CLOCK_REALTIME, NULL); > > + int result = __pthread_rwlock_wrlock_full64 (rwlock, > > CLOCK_REALTIME, NULL); LIBC_PROBE (wrlock_acquire_write, 1, rwlock); > > return result; > > } > > > > Ok. 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
On 30/09/2020 10:12, Lukasz Majewski wrote: >>> diff --git a/nptl/pthread_rwlock_rdlock.c >>> b/nptl/pthread_rwlock_rdlock.c index 04cecad395..2b8509a49c 100644 >>> --- a/nptl/pthread_rwlock_rdlock.c >>> +++ b/nptl/pthread_rwlock_rdlock.c >>> @@ -24,7 +24,7 @@ __pthread_rwlock_rdlock (pthread_rwlock_t *rwlock) >>> { >>> LIBC_PROBE (rdlock_entry, 1, rwlock); >> >> We need to move the systemtap probe to the 64-bit variant, so the >> 64-bit time will still trigger it. > > Those are syscalls, which don't require timespec time. These are in fact systemtap markers [1] and they are not related to syscalls. > > The LIBC_PROBE() macros were there before, so I left them as is and > only changed __pthread_rwlock_rdlock_full() to > __pthread_rwlock_rdlock_full64(). > > > On the other hand - for example: > __pthread_rwlock_clockwrlock() and __pthread_rwlock_clockwrlock64() > are only calling __pthread_rwlock_wrlock_full64(), and there were no > LIBC_PROBE() macros before. > > Do I understand correctly that you propose to add LIBC_PROBE() macros > to e.g. __pthread_rwlock_clockwrlock64() ? The idea of the LIBC_PROBE is to trigger an systemtap even when the function is called so systemtap can act accordingly (and the markers are enable only if --enable-systemtap is used on configure). Without moving the LIBC_PROBE, 64-bit time architectures won't see the probes anymore since they will call the 64-bit time version instead. [1] https://sourceware.org/systemtap/wiki/glibcMarkers
Hi Adhemerval, > On 30/09/2020 10:12, Lukasz Majewski wrote: > >>> diff --git a/nptl/pthread_rwlock_rdlock.c > >>> b/nptl/pthread_rwlock_rdlock.c index 04cecad395..2b8509a49c 100644 > >>> --- a/nptl/pthread_rwlock_rdlock.c > >>> +++ b/nptl/pthread_rwlock_rdlock.c > >>> @@ -24,7 +24,7 @@ __pthread_rwlock_rdlock (pthread_rwlock_t > >>> *rwlock) { > >>> LIBC_PROBE (rdlock_entry, 1, rwlock); > >> > >> We need to move the systemtap probe to the 64-bit variant, so the > >> 64-bit time will still trigger it. > > > > Those are syscalls, which don't require timespec time. > > These are in fact systemtap markers [1] and they are not related to > syscalls. Ok. Thanks for the info (and link). > > > > > The LIBC_PROBE() macros were there before, so I left them as is and > > only changed __pthread_rwlock_rdlock_full() to > > __pthread_rwlock_rdlock_full64(). > > > > > > On the other hand - for example: > > __pthread_rwlock_clockwrlock() and __pthread_rwlock_clockwrlock64() > > are only calling __pthread_rwlock_wrlock_full64(), and there were no > > LIBC_PROBE() macros before. > > > > Do I understand correctly that you propose to add LIBC_PROBE() > > macros to e.g. __pthread_rwlock_clockwrlock64() ? > > The idea of the LIBC_PROBE is to trigger an systemtap even when the > function is called so systemtap can act accordingly (and the markers > are enable only if --enable-systemtap is used on configure). > Ok. > Without moving the LIBC_PROBE, 64-bit time architectures won't see > the probes anymore since they will call the 64-bit time version > instead. Please correct me if I'm wrong, but it seems to me that you are concerned if - __pthread_rwlock_rdlock_full64 - __pthread_rwlock_wrlock_full64 would be called as aliased (for e.g. x86-64) and redirected (for e.g. arm)? This is not the case as above functions are local (i.e. helpers) ones defined in ./nptl/pthread_rwlock_common.c (and are not exported). For them, the only change was to add '64' suffix. The LIBC_PROBE() calls have been left untouched, surrounding them, so I think that we don't need to move them. Or maybe I do miss something important here? Thanks in advance for your help and explanation. > > [1] https://sourceware.org/systemtap/wiki/glibcMarkers 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
On 02/10/2020 09:15, Lukasz Majewski wrote: > Hi Adhemerval, > >> On 30/09/2020 10:12, Lukasz Majewski wrote: >>>>> diff --git a/nptl/pthread_rwlock_rdlock.c >>>>> b/nptl/pthread_rwlock_rdlock.c index 04cecad395..2b8509a49c 100644 >>>>> --- a/nptl/pthread_rwlock_rdlock.c >>>>> +++ b/nptl/pthread_rwlock_rdlock.c >>>>> @@ -24,7 +24,7 @@ __pthread_rwlock_rdlock (pthread_rwlock_t >>>>> *rwlock) { >>>>> LIBC_PROBE (rdlock_entry, 1, rwlock); >>>> >>>> We need to move the systemtap probe to the 64-bit variant, so the >>>> 64-bit time will still trigger it. >>> >>> Those are syscalls, which don't require timespec time. >> >> These are in fact systemtap markers [1] and they are not related to >> syscalls. > > Ok. Thanks for the info (and link). > >> >>> >>> The LIBC_PROBE() macros were there before, so I left them as is and >>> only changed __pthread_rwlock_rdlock_full() to >>> __pthread_rwlock_rdlock_full64(). >>> >>> >>> On the other hand - for example: >>> __pthread_rwlock_clockwrlock() and __pthread_rwlock_clockwrlock64() >>> are only calling __pthread_rwlock_wrlock_full64(), and there were no >>> LIBC_PROBE() macros before. >>> >>> Do I understand correctly that you propose to add LIBC_PROBE() >>> macros to e.g. __pthread_rwlock_clockwrlock64() ? >> >> The idea of the LIBC_PROBE is to trigger an systemtap even when the >> function is called so systemtap can act accordingly (and the markers >> are enable only if --enable-systemtap is used on configure). >> > > Ok. > >> Without moving the LIBC_PROBE, 64-bit time architectures won't see >> the probes anymore since they will call the 64-bit time version >> instead. > > Please correct me if I'm wrong, but it seems to me that you are > concerned if > > - __pthread_rwlock_rdlock_full64 > - __pthread_rwlock_wrlock_full64 > > would be called as aliased (for e.g. x86-64) and redirected (for e.g. > arm)? > > This is not the case as above functions are local (i.e. helpers) ones > defined in ./nptl/pthread_rwlock_common.c (and are not exported). > > For them, the only change was to add '64' suffix. > > The LIBC_PROBE() calls have been left untouched, surrounding them, so I > think that we don't need to move them. > > > Or maybe I do miss something important here? Thanks in advance for your > help and explanation. In fact this is a wrong assumption from my part, there is no LIBC_PROBE on the timed variant rwlocks. The patch looks ok in this regard (sorry for the noise).
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h index 9bb44c8535..5bcc8a2db5 100644 --- a/nptl/pthreadP.h +++ b/nptl/pthreadP.h @@ -464,6 +464,10 @@ extern int __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex); # define __pthread_timedjoin_np64 __pthread_timedjoin_np # define __pthread_cond_timedwait64 __pthread_cond_timedwait # define __pthread_cond_clockwait64 __pthread_cond_clockwait +# define __pthread_rwlock_clockrdlock64 __pthread_rwlock_clockrdlock +# define __pthread_rwlock_clockwrlock64 __pthread_rwlock_clockwrlock +# define __pthread_rwlock_timedrdlock64 __pthread_rwlock_timedrdlock +# define __pthread_rwlock_timedwrlock64 __pthread_rwlock_timedwrlock #else extern int __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return, clockid_t clockid, @@ -481,6 +485,20 @@ extern int __pthread_cond_clockwait64 (pthread_cond_t *cond, clockid_t clockid, const struct __timespec64 *abstime); libpthread_hidden_proto (__pthread_cond_clockwait64) +extern int __pthread_rwlock_clockrdlock64 (pthread_rwlock_t *rwlock, + clockid_t clockid, + const struct __timespec64 *abstime); +libpthread_hidden_proto (__pthread_rwlock_clockrdlock64) +extern int __pthread_rwlock_clockwrlock64 (pthread_rwlock_t *rwlock, + clockid_t clockid, + const struct __timespec64 *abstime); +libpthread_hidden_proto (__pthread_rwlock_clockwrlock64) +extern int __pthread_rwlock_timedrdlock64 (pthread_rwlock_t *rwlock, + const struct __timespec64 *abstime); +libpthread_hidden_proto (__pthread_rwlock_timedrdlock64) +extern int __pthread_rwlock_timedwrlock64 (pthread_rwlock_t *rwlock, + const struct __timespec64 *abstime); +libpthread_hidden_proto (__pthread_rwlock_timedwrlock64) #endif extern int __pthread_cond_timedwait (pthread_cond_t *cond, diff --git a/nptl/pthread_rwlock_clockrdlock.c b/nptl/pthread_rwlock_clockrdlock.c index 4cedfd1dcd..d93b133c9e 100644 --- a/nptl/pthread_rwlock_clockrdlock.c +++ b/nptl/pthread_rwlock_clockrdlock.c @@ -21,8 +21,21 @@ /* See pthread_rwlock_common.c. */ int -pthread_rwlock_clockrdlock (pthread_rwlock_t *rwlock, clockid_t clockid, - const struct timespec *abstime) +__pthread_rwlock_clockrdlock64 (pthread_rwlock_t *rwlock, clockid_t clockid, + const struct __timespec64 *abstime) { - return __pthread_rwlock_rdlock_full (rwlock, clockid, abstime); + return __pthread_rwlock_rdlock_full64 (rwlock, clockid, abstime); } + +#if __TIMESIZE != 64 +libpthread_hidden_def (__pthread_rwlock_clockrdlock64) +int +__pthread_rwlock_clockrdlock (pthread_rwlock_t *rwlock, clockid_t clockid, + const struct timespec *abstime) +{ + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); + + return __pthread_rwlock_clockrdlock64 (rwlock, clockid, &ts64); +} +#endif +weak_alias (__pthread_rwlock_clockrdlock, pthread_rwlock_clockrdlock) diff --git a/nptl/pthread_rwlock_clockwrlock.c b/nptl/pthread_rwlock_clockwrlock.c index 7a954cf529..10a314c76d 100644 --- a/nptl/pthread_rwlock_clockwrlock.c +++ b/nptl/pthread_rwlock_clockwrlock.c @@ -21,8 +21,21 @@ /* See pthread_rwlock_common.c. */ int -pthread_rwlock_clockwrlock (pthread_rwlock_t *rwlock, clockid_t clockid, - const struct timespec *abstime) +__pthread_rwlock_clockwrlock64 (pthread_rwlock_t *rwlock, clockid_t clockid, + const struct __timespec64 *abstime) { - return __pthread_rwlock_wrlock_full (rwlock, clockid, abstime); + return __pthread_rwlock_wrlock_full64 (rwlock, clockid, abstime); } + +#if __TIMESIZE != 64 +libpthread_hidden_def (__pthread_rwlock_clockwrlock64) +int +__pthread_rwlock_clockwrlock (pthread_rwlock_t *rwlock, clockid_t clockid, + const struct timespec *abstime) +{ + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); + + return __pthread_rwlock_clockwrlock64 (rwlock, clockid, &ts64); +} +#endif +weak_alias (__pthread_rwlock_clockwrlock, pthread_rwlock_clockwrlock) diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c index 3fbc66ded2..4c9f582d3d 100644 --- a/nptl/pthread_rwlock_common.c +++ b/nptl/pthread_rwlock_common.c @@ -278,9 +278,8 @@ __pthread_rwlock_rdunlock (pthread_rwlock_t *rwlock) static __always_inline int -__pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, - clockid_t clockid, - const struct timespec *abstime) +__pthread_rwlock_rdlock_full64 (pthread_rwlock_t *rwlock, clockid_t clockid, + const struct __timespec64 *abstime) { unsigned int r; @@ -330,8 +329,9 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, & PTHREAD_RWLOCK_RWAITING) != 0) { int private = __pthread_rwlock_get_private (rwlock); - int err = futex_abstimed_wait (&rwlock->__data.__readers, - r, clockid, abstime, private); + int err = __futex_abstimed_wait64 (&rwlock->__data.__readers, + r, clockid, abstime, + private); /* We ignore EAGAIN and EINTR. On time-outs, we can just return because we don't need to clean up anything. */ if (err == ETIMEDOUT) @@ -457,9 +457,9 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, (&rwlock->__data.__wrphase_futex, &wpf, wpf | PTHREAD_RWLOCK_FUTEX_USED))) continue; - int err = futex_abstimed_wait (&rwlock->__data.__wrphase_futex, - 1 | PTHREAD_RWLOCK_FUTEX_USED, - clockid, abstime, private); + int err = __futex_abstimed_wait64 (&rwlock->__data.__wrphase_futex, + 1 | PTHREAD_RWLOCK_FUTEX_USED, + clockid, abstime, private); if (err == ETIMEDOUT) { /* If we timed out, we need to unregister. If no read phase @@ -585,9 +585,8 @@ __pthread_rwlock_wrunlock (pthread_rwlock_t *rwlock) static __always_inline int -__pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, - clockid_t clockid, - const struct timespec *abstime) +__pthread_rwlock_wrlock_full64 (pthread_rwlock_t *rwlock, clockid_t clockid, + const struct __timespec64 *abstime) { /* Make sure any passed in clockid and timeout value are valid. Note that the previous implementation assumed that this check *must* not be @@ -728,9 +727,9 @@ __pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, share the flag, and another writer will wake one of the writers in this group. */ may_share_futex_used_flag = true; - int err = futex_abstimed_wait (&rwlock->__data.__writers_futex, - 1 | PTHREAD_RWLOCK_FUTEX_USED, - clockid, abstime, private); + int err = __futex_abstimed_wait64 (&rwlock->__data.__writers_futex, + 1 | PTHREAD_RWLOCK_FUTEX_USED, + clockid, abstime, private); if (err == ETIMEDOUT) { if (prefer_writer) @@ -827,9 +826,9 @@ __pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, (&rwlock->__data.__wrphase_futex, &wpf, PTHREAD_RWLOCK_FUTEX_USED))) continue; - int err = futex_abstimed_wait (&rwlock->__data.__wrphase_futex, - PTHREAD_RWLOCK_FUTEX_USED, - clockid, abstime, private); + int err = __futex_abstimed_wait64 (&rwlock->__data.__wrphase_futex, + PTHREAD_RWLOCK_FUTEX_USED, + clockid, abstime, private); if (err == ETIMEDOUT) { if (rwlock->__data.__flags != PTHREAD_RWLOCK_PREFER_READER_NP) diff --git a/nptl/pthread_rwlock_rdlock.c b/nptl/pthread_rwlock_rdlock.c index 04cecad395..2b8509a49c 100644 --- a/nptl/pthread_rwlock_rdlock.c +++ b/nptl/pthread_rwlock_rdlock.c @@ -24,7 +24,7 @@ __pthread_rwlock_rdlock (pthread_rwlock_t *rwlock) { LIBC_PROBE (rdlock_entry, 1, rwlock); - int result = __pthread_rwlock_rdlock_full (rwlock, CLOCK_REALTIME, NULL); + int result = __pthread_rwlock_rdlock_full64 (rwlock, CLOCK_REALTIME, NULL); LIBC_PROBE (rdlock_acquire_read, 1, rwlock); return result; } diff --git a/nptl/pthread_rwlock_timedrdlock.c b/nptl/pthread_rwlock_timedrdlock.c index c5d8aee909..d5999a1419 100644 --- a/nptl/pthread_rwlock_timedrdlock.c +++ b/nptl/pthread_rwlock_timedrdlock.c @@ -20,8 +20,21 @@ /* See pthread_rwlock_common.c. */ int -pthread_rwlock_timedrdlock (pthread_rwlock_t *rwlock, - const struct timespec *abstime) +__pthread_rwlock_timedrdlock64 (pthread_rwlock_t *rwlock, + const struct __timespec64 *abstime) { - return __pthread_rwlock_rdlock_full (rwlock, CLOCK_REALTIME, abstime); + return __pthread_rwlock_rdlock_full64 (rwlock, CLOCK_REALTIME, abstime); } + +#if __TIMESIZE != 64 +libpthread_hidden_def (__pthread_rwlock_timedrdlock64) +int +__pthread_rwlock_timedrdlock (pthread_rwlock_t *rwlock, + const struct timespec *abstime) +{ + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); + + return __pthread_rwlock_timedrdlock64 (rwlock, &ts64); +} +#endif +weak_alias (__pthread_rwlock_timedrdlock, pthread_rwlock_timedrdlock) diff --git a/nptl/pthread_rwlock_timedwrlock.c b/nptl/pthread_rwlock_timedwrlock.c index ccee8b77d9..6c5284a172 100644 --- a/nptl/pthread_rwlock_timedwrlock.c +++ b/nptl/pthread_rwlock_timedwrlock.c @@ -20,8 +20,21 @@ /* See pthread_rwlock_common.c. */ int -pthread_rwlock_timedwrlock (pthread_rwlock_t *rwlock, - const struct timespec *abstime) +__pthread_rwlock_timedwrlock64 (pthread_rwlock_t *rwlock, + const struct __timespec64 *abstime) { - return __pthread_rwlock_wrlock_full (rwlock, CLOCK_REALTIME, abstime); + return __pthread_rwlock_wrlock_full64 (rwlock, CLOCK_REALTIME, abstime); } + +#if __TIMESIZE != 64 +libpthread_hidden_def (__pthread_rwlock_timedwrlock64) +int +__pthread_rwlock_timedwrlock (pthread_rwlock_t *rwlock, + const struct timespec *abstime) +{ + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime); + + return __pthread_rwlock_timedwrlock64 (rwlock, &ts64); +} +#endif +weak_alias (__pthread_rwlock_timedwrlock, pthread_rwlock_timedwrlock) diff --git a/nptl/pthread_rwlock_wrlock.c b/nptl/pthread_rwlock_wrlock.c index a42aa626f0..210e6cffdc 100644 --- a/nptl/pthread_rwlock_wrlock.c +++ b/nptl/pthread_rwlock_wrlock.c @@ -24,7 +24,7 @@ __pthread_rwlock_wrlock (pthread_rwlock_t *rwlock) { LIBC_PROBE (wrlock_entry, 1, rwlock); - int result = __pthread_rwlock_wrlock_full (rwlock, CLOCK_REALTIME, NULL); + int result = __pthread_rwlock_wrlock_full64 (rwlock, CLOCK_REALTIME, NULL); LIBC_PROBE (wrlock_acquire_write, 1, rwlock); return result; }
The pthread_rwlock_clockrdlock, pthread_rwlock_clockwrlock, pthread_rwlock_timedrdlock and pthread_rwlock_timedwrlock have been converted to support 64 bit time. This change uses new futex_abstimed_wait64 function in ./sysdeps/nptl/futex-helpers.c, which uses futex_time64 where possible. The pthread_rwlock_{clock|timed}{rd|wr}lock only accepts absolute time. Moreover, there is no need to check for NULL passed as *abstime pointer to the syscalls as those calls have exported symbols marked with __nonull attribute for abstime. For systems with __TIMESIZE != 64 && __WORDSIZE == 32: - Conversions between 64 bit time to 32 bit are necessary - Redirection to pthread_rwlock_{clock|timed}{rd|wr}lock will provide support for 64 bit time Build tests: ./src/scripts/build-many-glibcs.py glibcs Run-time tests: - Run specific tests on ARM/x86 32bit systems (qemu): https://github.com/lmajewski/meta-y2038 and run tests: https://github.com/lmajewski/y2038-tests/commits/master Above tests were performed with Y2038 redirection applied as well as without to test the proper usage of both __pthread_rwlock_{clock|timed}{rd|wr}lock64 and __pthread_rwlock_{clock|timed}{rd|wr}lock. Reviewed-by: Alistair Francis <alistair.francis@wdc.com> --- Changes for v2: - None --- nptl/pthreadP.h | 18 +++++++++++++++++ nptl/pthread_rwlock_clockrdlock.c | 19 +++++++++++++++--- nptl/pthread_rwlock_clockwrlock.c | 19 +++++++++++++++--- nptl/pthread_rwlock_common.c | 33 +++++++++++++++---------------- nptl/pthread_rwlock_rdlock.c | 2 +- nptl/pthread_rwlock_timedrdlock.c | 19 +++++++++++++++--- nptl/pthread_rwlock_timedwrlock.c | 19 +++++++++++++++--- nptl/pthread_rwlock_wrlock.c | 2 +- 8 files changed, 100 insertions(+), 31 deletions(-)