From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from avasout04.plus.net (avasout04.plus.net [212.159.14.19]) by sourceware.org (Postfix) with ESMTPS id 08B25388A41B for ; Wed, 25 Nov 2020 15:46:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 08B25388A41B Received: from deneb ([80.229.24.9]) by smtp with ESMTP id hx0Mkl0wWrXCchx0OkY4Wt; Wed, 25 Nov 2020 15:46:52 +0000 X-Clacks-Overhead: "GNU Terry Pratchett" X-CM-Score: 0.00 X-CNFS-Analysis: v=2.3 cv=Q+xJH7+a c=1 sm=1 tr=0 a=E/9URZZQ5L3bK/voZ0g0HQ==:117 a=E/9URZZQ5L3bK/voZ0g0HQ==:17 a=kj9zAlcOel0A:10 a=nNwsprhYR40A:10 a=3ZumZcghDVIeFjomfNYA:9 a=CjuIK1q_8ugA:10 Received: from mac by deneb with local (Exim 4.92) (envelope-from ) id 1khx0M-0003BV-S3; Wed, 25 Nov 2020 15:46:50 +0000 Date: Wed, 25 Nov 2020 15:46:50 +0000 From: Mike Crowe To: Adhemerval Zanella Cc: libc-alpha@sourceware.org, Michael Kerrisk Subject: Re: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h Message-ID: <20201125154650.GA10858@mcrowe.com> References: <20201123195256.3336217-1-adhemerval.zanella@linaro.org> <20201123195256.3336217-9-adhemerval.zanella@linaro.org> <20201125153231.GA1391@mcrowe.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CMAE-Envelope: MS4wfP/xtF9+NzVvhsBvCasTRojw7weY+mQk2Dg2nL7tXdzr/ja+WuAbII/CEfBZnSqfmmrWuaURR0herpJLqWvathh9aSabgmlOe/WMu41bPyxdw3YfxhG0 8SvO5GQ+TDJhfqEebbLDLc+dnizw09kDsqdXlsxc/N7MvELPxNr6+jIA X-Spam-Status: No, score=-13.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, 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, 25 Nov 2020 15:46:54 -0000 On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote: > > > On 25/11/2020 12:32, Mike Crowe wrote: > > On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote: > >> The idea is to make NPTL implementation to use on the functions > >> provided by futex-internal.h. > >> > >> Checked on x86_64-linux-gnu and i686-linux-gnu. > >> --- > >> nptl/lowlevellock.c | 6 +++--- > >> nptl/pthread_mutex_lock.c | 9 +++++---- > >> nptl/pthread_mutex_setprioceiling.c | 5 +++-- > >> nptl/pthread_mutex_timedlock.c | 6 +++--- > >> 4 files changed, 14 insertions(+), 12 deletions(-) > > > > [snip] > > > >> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > >> index e643eab258..343acf6107 100644 > >> --- a/nptl/pthread_mutex_timedlock.c > >> +++ b/nptl/pthread_mutex_timedlock.c > >> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > >> goto failpp; > >> } > >> > >> - lll_futex_timed_wait (&mutex->__data.__lock, > >> - ceilval | 2, &rt, > >> - PTHREAD_MUTEX_PSHARED (mutex)); > >> + __futex_abstimed_wait64 ( > >> + (unsigned int *) &mutex->__data.__lock, clockid, > >> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); > > > > I think you've replaced the lll_futex_timed_wait call that expects a > > relative timeout with a __futex_abstimed_wait64 call that expects an > > absolute timeout, yet you still appear to be passing the relative timeout. > > > > However, it turns out that the implementation for the > > PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been > > completely broken with clockid != CLOCK_REALTIME ever since I added it in > > 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout > > is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the > > time this was a less obvious __gettimeofday call.) > > > > I'll work on writing some test cases for the those types of mutex in the > > hope of catching both flaws before fixing them. > > Indeed, there is no need to calculate the relative timeout anymore. I think > the fix below should pass the absolute timeout directly. I will check > a possible regression tests as well. OK. I won't then. Thanks. > diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > index aaaafa21ce..86c5f4446e 100644 > --- a/nptl/pthread_mutex_timedlock.c > +++ b/nptl/pthread_mutex_timedlock.c > @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > if (__pthread_current_priority () > ceiling) > { > result = EINVAL; > - failpp: > if (oldprio != -1) > __pthread_tpp_change_priority (oldprio, -1); > return result; > @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > > if (oldval != ceilval) > { > - /* Reject invalid timeouts. */ > - if (! valid_nanoseconds (abstime->tv_nsec)) > - { > - result = EINVAL; > - goto failpp; > - } If this is removed then is there a risk of getting into a busy loop if someone passes a bogus timespec? (Regardless of the answer, it makes sense to ensure that is tested somehow.) > - > - struct __timespec64 rt; > - > - /* Get the current time. */ > - __clock_gettime64 (CLOCK_REALTIME, &rt); > - > - /* Compute relative timeout. */ > - rt.tv_sec = abstime->tv_sec - rt.tv_sec; > - rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec; > - if (rt.tv_nsec < 0) > - { > - rt.tv_nsec += 1000000000; > - --rt.tv_sec; > - } > - > - /* Already timed out? */ > - if (rt.tv_sec < 0) > - { > - result = ETIMEDOUT; > - goto failpp; > - } > - > __futex_abstimed_wait64 ( > (unsigned int *) &mutex->__data.__lock, clockid, > - ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); > + ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex)); > } > } > while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock, LGTM. Thanks. Mike.