From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-4.1 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI,NICE_REPLY_A, RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id BA47B1F8C6 for ; Thu, 12 Aug 2021 12:41:30 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D153E3853C2E for ; Thu, 12 Aug 2021 12:41:29 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D153E3853C2E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1628772089; bh=qOZwGur2mbYCHaM0/epEfX0a2QMClfEN9UvfKyihapY=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=vhm1M1w7wYbMe6OBQN7i7MQ074j89Ft5epk7vLx8xn0pzWUIOO2L75B+pFEbP2Set 3GyuEW16iwzhFgfHuvmemRo2WtuSOdNv8Nq0vr6JZRNz/DODkNfgS3fg2G6LenoZnc agA6ImtKqgeMcG2WqbEZ2opNydfwpFDVFryxLihM= Received: from mail-qv1-xf2c.google.com (mail-qv1-xf2c.google.com [IPv6:2607:f8b0:4864:20::f2c]) by sourceware.org (Postfix) with ESMTPS id 2A4503853C20 for ; Thu, 12 Aug 2021 12:41:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2A4503853C20 Received: by mail-qv1-xf2c.google.com with SMTP id dt3so1153713qvb.6 for ; Thu, 12 Aug 2021 05:41:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=qOZwGur2mbYCHaM0/epEfX0a2QMClfEN9UvfKyihapY=; b=gRjk9oRgezUiQI8tc6+gzsFdY/BifBq0DhofKBGHc/OfdF+h7VuKG3IunSadrgzO8g so9pv78rdn0qxZI18UpErIYDFt/e9eHIgHe+ZjEkBVl1OwcvMBGWxJxURKXb2PKqu8DO HUy+gW+i74zMYbTT8uqpeIv3JpdL5dyhIvGkAxeYTKt9S+AF7qzG/V6SKwUvTcOTwFJJ F/RVGXXPY90XR9H99Dr62gNRzwHdEy0Y5j5bH9VQRoxJuv8Qmx7Al8yneD5/Q1mgHXKn qAKnn0sMu34BPQfNGwns/lIoC+ZEft87YN3BjTKdLyLsyBkLAphbSJ4Ef+9v/DUXNoIq fDFg== X-Gm-Message-State: AOAM531+hSwZbwdmzG47ebMBSKLjvaLlDUK1JSkcxAM6r/im+glKHqan fiWC/du/tYmgVb+9htj+vj8Ypj0reZJEKA== X-Google-Smtp-Source: ABdhPJzat5y1DwOQ34AkIc4okK35AHmUE8N7TjYVaYvH2U/7IJuFjojsPJD4kr5vtO/PIkPYvuaokw== X-Received: by 2002:a05:6214:902:: with SMTP id dj2mr1685501qvb.62.1628772060661; Thu, 12 Aug 2021 05:41:00 -0700 (PDT) Received: from ?IPv6:2804:431:c7cb:9dce:d510:7aa1:3ecd:f80c? ([2804:431:c7cb:9dce:d510:7aa1:3ecd:f80c]) by smtp.gmail.com with ESMTPSA id m197sm1215719qke.54.2021.08.12.05.40.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 12 Aug 2021 05:41:00 -0700 (PDT) Subject: Re: [PATCH 2/4] nptl: Use FUTEX_LOCK_PI2 when available To: Kurt Kanzenbach , libc-alpha@sourceware.org References: <20210811200133.3869287-1-adhemerval.zanella@linaro.org> <20210811200133.3869287-3-adhemerval.zanella@linaro.org> <87im0b6u6e.fsf@kurt> Message-ID: <3991be9a-2a41-c211-a4ce-f7efdad67cc5@linaro.org> Date: Thu, 12 Aug 2021 09:40:58 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <87im0b6u6e.fsf@kurt> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: , From: Adhemerval Zanella via Libc-alpha Reply-To: Adhemerval Zanella Errors-To: libc-alpha-bounces+e=80x24.org@sourceware.org Sender: "Libc-alpha" On 12/08/2021 03:42, Kurt Kanzenbach wrote: > Hi Adhemerval, > > thanks for the respin of this series. Just some minor comments below. > > On Wed Aug 11 2021, Adhemerval Zanella wrote: >> This patch uses the new futex PI operation provided by Linux v5.14 >> when it is required. >> >> The futex_lock_pi64() is moved to futex-internal.c (since it used on >> two different places and its code size might be large depending of the >> kernel configuration) and clockid is added as an argument. >> >> Co-authored-by: Kurt Kanzenbach >> --- >> nptl/futex-internal.c | 63 +++++++++++++++++++++++++++++++ >> nptl/pthread_mutex_lock.c | 3 +- >> nptl/pthread_mutex_timedlock.c | 3 +- >> sysdeps/nptl/futex-internal.h | 54 +------------------------- >> sysdeps/nptl/lowlevellock-futex.h | 1 + >> 5 files changed, 70 insertions(+), 54 deletions(-) >> >> diff --git a/nptl/futex-internal.c b/nptl/futex-internal.c >> index e74647a9d4..58605b2fca 100644 >> --- a/nptl/futex-internal.c >> +++ b/nptl/futex-internal.c >> @@ -140,3 +140,66 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word, >> abstime, private, true); >> } >> libc_hidden_def (__futex_abstimed_wait_cancelable64) >> + >> +int >> +__futex_lock_pi64 (int *futex_word, clockid_t clockid, >> + const struct __timespec64 *abstime, int private) >> +{ >> + int err; > > Is the clockid check not needed? > > if (! lll_futex_supported_clockid (clockid)) > return EINVAL; The ___pthread_mutex_clocklock64() already checks it (as other functions that uses a clockid_t). I also tested for invalid clocks on tst-mutexpi10.c. And I think that the check on __futex_abstimed_wait_common() is superfluous.. > >> + >> + unsigned int clockbit = clockid == CLOCK_REALTIME >> + ? FUTEX_CLOCK_REALTIME : 0; >> + int op_pi2 = __lll_private_flag (FUTEX_LOCK_PI2 | clockbit, private); >> +#if __ASSUME_FUTEX_LOCK_PI2 >> + /* Assume __ASSUME_TIME64_SYSCALLS since FUTEX_LOCK_PI2 was added later. */ >> + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi2, 0, abstime); >> +#else >> + /* FUTEX_LOCK_PI does not support clock selection, so for CLOCK_MONOTONIC >> + the only option is to use FUTEX_LOCK_PI2. */ >> + int op_pi1 = __lll_private_flag (FUTEX_LOCK_PI, private); >> + int op_pi = abstime != NULL && clockid != CLOCK_REALTIME ? op_pi2 : op_pi1; >> + >> +# ifdef __ASSUME_TIME64_SYSCALLS >> + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi, 0, abstime); >> +# else >> + bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec); >> + if (need_time64) >> + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi, 0, abstime); >> + else >> + { >> + struct timespec ts32, *pts32 = NULL; >> + if (abstime != NULL) >> + { >> + ts32 = valid_timespec64_to_timespec (*abstime); >> + pts32 = &ts32; >> + } >> + err = INTERNAL_SYSCALL_CALL (futex, futex_word, op_pi, 0, pts32); >> + } >> +# endif /* __ASSUME_TIME64_SYSCALLS */ >> + /* FUTEX_LOCK_PI2 is not available on this kernel. */ >> + if (err == -ENOSYS) >> + err = -EINVAL; >> +#endif /* __ASSUME_FUTEX_LOCK_PI2 */ >> + >> + switch (err) >> + { >> + case 0: >> + case -EAGAIN: >> + case -EINTR: >> + case -ETIMEDOUT: >> + case -ESRCH: >> + case -EDEADLK: >> + case -EINVAL: /* This indicates either state corruption or that the kernel >> + found a waiter on futex address which is waiting via >> + FUTEX_WAIT or FUTEX_WAIT_BITSET. This is reported on >> + some futex_lock_pi usage (pthread_mutex_timedlock for >> + instance). */ >> + return -err; >> + >> + case -EFAULT: /* 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 (); >> + } >> +} >> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c >> index da624f322d..8e2ff2793f 100644 >> --- a/nptl/pthread_mutex_lock.c >> +++ b/nptl/pthread_mutex_lock.c >> @@ -422,7 +422,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) >> int private = (robust >> ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex) >> : PTHREAD_MUTEX_PSHARED (mutex)); >> - int e = futex_lock_pi64 (&mutex->__data.__lock, NULL, private); >> + int e = __futex_lock_pi64 (&mutex->__data.__lock, 0 /* ununsed */, > > And then use CLOCK_REALTIME here? If timeout is not define the clock is ignore, isn't? > >> + NULL, private); >> if (e == ESRCH || e == EDEADLK) >> { >> assert (e != EDEADLK >> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c >> index 11ad7005d0..ca51da6f6c 100644 >> --- a/nptl/pthread_mutex_timedlock.c >> +++ b/nptl/pthread_mutex_timedlock.c >> @@ -370,7 +370,8 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, >> int private = (robust >> ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex) >> : PTHREAD_MUTEX_PSHARED (mutex)); >> - int e = futex_lock_pi64 (&mutex->__data.__lock, abstime, private); >> + int e = __futex_lock_pi64 (&mutex->__data.__lock, clockid, abstime, >> + private); >> if (e == ETIMEDOUT) >> return ETIMEDOUT; >> else if (e == ESRCH || e == EDEADLK) >> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h >> index 79a366604d..f0b7f814cc 100644 >> --- a/sysdeps/nptl/futex-internal.h >> +++ b/sysdeps/nptl/futex-internal.h >> @@ -250,58 +250,8 @@ futex_wake (unsigned int* futex_word, int processes_to_wake, int private) >> futex. >> - ETIMEDOUT if the ABSTIME expires. >> */ > > I think the documentation above should be slightly updated for this function: Thanks, I updated it locally. > > --- a/sysdeps/nptl/futex-internal.h > +++ b/sysdeps/nptl/futex-internal.h > @@ -236,8 +236,8 @@ futex_wake (unsigned int* futex_word, in > are done in descending priority order. > > The ABSTIME arguments provides an absolute timeout (measured against the > - CLOCK_REALTIME clock). If TIMEOUT is NULL, the operation will block > - indefinitely. > + CLOCK_REALTIME or CLOCK_MONOTONIC clock). If TIMEOUT is NULL, the operation > + will block indefinitely. > > Returns: > > @@ -247,61 +247,11 @@ futex_wake (unsigned int* futex_word, in > - EDEADLK if the futex is already locked by the caller. > - ESRCH if the thread ID int he futex does not exist. > - EINVAL is the state is corrupted or if there is a waiter on the > - futex. > + futex or if the clockid is invalid. > - ETIMEDOUT if the ABSTIME expires. > */ > > Thanks, > Kurt >