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: AS17314 8.43.84.0/22 X-Spam-Status: No, score=-4.2 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 [8.43.85.97]) (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 3B3211F8C6 for ; Fri, 9 Jul 2021 14:14:22 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 220C3398B0F9 for ; Fri, 9 Jul 2021 14:14:21 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 220C3398B0F9 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1625840061; bh=NUjxO5bzihOztJt4CsCeELDbbGIuMEwmYEKsAISpbSs=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=lCOmpZyY2WBl2gkFKUBxGzBqkbm68JlK8X0fj8lKFz9kFMQKu3lrnoW8ySpYww16j tXAaxhtpJg2CVu7piOhFN84CCHNeO0S3PgpZNpr0YIO8PGUQ+fnEzUgPuCt94/YaW5 fPa8XVogv9dpkizYysebRZqI8gSk1XzoISvOXd/4= Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 6BC0B384780B for ; Fri, 9 Jul 2021 14:14:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6BC0B384780B Received: by mail-pg1-x52f.google.com with SMTP id 37so10125242pgq.0 for ; Fri, 09 Jul 2021 07:14:00 -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:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=NUjxO5bzihOztJt4CsCeELDbbGIuMEwmYEKsAISpbSs=; b=KppGGOMA7r/06yumc6O0uqsYIvJmnLV/webzi28ArF14UU2VzvnWrulnO1xOZCL/Q5 Fzx8POWhYfJnKhjy1PtcrCrm7DKRoTCKbvoEs7MJd7cxSyFR/Yc+qPggjpCZsmQD4I0c l52nz3sdq44JN3f4BnwK0Vf1jfyF97/i3IjbvhghCA21VC+9lAyKWt2Db3xSXTZ47iZG PP8BqlW28Xc/XYIGO7zQhrg2wxt5gHGgpy1YcPOP7TM0KeZmNVt2lQMX68i8ZonYug+w 9sxRLA3W9lp/I64mMvB7sgZJVJv0y87xeyyE7SzcJMzX6HYn5dRlaxs2lMO07PmwMTJi tnLg== X-Gm-Message-State: AOAM533rEYzVK0hsA/fzYp8KX/8PMMnj2rx9vSG0l2slb2LUpXpb1deS EJLAQSq2stJzbdlQ2S1HojHqpg== X-Google-Smtp-Source: ABdhPJzNFbLAWTfwkbikvk79FBb3hc7uL0tt/sPnk2uA2UgonwKFGKtv/jPEK2HickbUkIl+nGBagg== X-Received: by 2002:a62:a102:0:b029:2f4:a8e:cc44 with SMTP id b2-20020a62a1020000b02902f40a8ecc44mr37753485pff.38.1625840039487; Fri, 09 Jul 2021 07:13:59 -0700 (PDT) Received: from [192.168.1.108] ([177.194.59.218]) by smtp.gmail.com with ESMTPSA id c14sm7409993pgv.86.2021.07.09.07.13.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 09 Jul 2021 07:13:59 -0700 (PDT) Subject: Re: [PATCH v1 2/6] nptl: Introduce futex_lock_pi2() To: Kurt Kanzenbach , libc-alpha@sourceware.org References: <20210625081104.1134598-1-kurt@linutronix.de> <20210625081104.1134598-3-kurt@linutronix.de> Message-ID: Date: Fri, 9 Jul 2021 11:13:55 -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: <20210625081104.1134598-3-kurt@linutronix.de> 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 Cc: Florian Weimer , Sebastian Andrzej Siewior , Peter Zijlstra , Thomas Gleixner , Joseph Myers Errors-To: libc-alpha-bounces+e=80x24.org@sourceware.org Sender: "Libc-alpha" On 25/06/2021 05:11, Kurt Kanzenbach wrote: > This variant of futex_lock() has support for selectable clocks and priority > inheritance. The underlying FUTEX_LOCK_PI2 operation has been recently > introduced into the Linux kernel. > > It can be used for implementing pthread_mutex_clocklock(MONOTONIC)/PI. > > The code works like this: > > * If kernel support is assumed, then use FUTEX_LOCK_PI2 > * If not, distuingish between clockid: s/distuingish/distinguish > * For realtime use FUTEX_LOCK_PI > * For monotonic try to use FUTEX_LOCK_PI2 which might not be available > > Signed-off-by: Kurt Kanzenbach > --- > sysdeps/nptl/futex-internal.h | 131 ++++++++++++++++++++++++++++++ > sysdeps/nptl/lowlevellock-futex.h | 1 + > 2 files changed, 132 insertions(+) > > diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h > index 79a366604d9e..38c969831276 100644 > --- a/sysdeps/nptl/futex-internal.h > +++ b/sysdeps/nptl/futex-internal.h > @@ -303,6 +303,137 @@ futex_lock_pi64 (int *futex_word, const struct __timespec64 *abstime, > } > } > > +/* The operation checks the value of the futex, if the value is 0, then > + it is atomically set to the caller's thread ID. If the futex value is > + nonzero, it is atomically sets the FUTEX_WAITERS bit, which signals wrt > + other futex owner that it cannot unlock the futex in user space by > + atomically by setting its value to 0. > + > + If more than one wait operations is issued, the enqueueing of the waiters > + are done in descending priority order. > + > + The ABSTIME arguments provides an absolute timeout (measured against the > + CLOCK_REALTIME or CLOCK_MONOTONIC clock). If TIMEOUT is NULL, the operation > + will block indefinitely. > + > + Returns: > + > + - 0 if woken by a PI unlock operation or spuriously. > + - EAGAIN if the futex owner thread ID is about to exit, but has not yet > + handled the state cleanup. > + - 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 or if the clockid is invalid. > + - ETIMEDOUT if the ABSTIME expires. > +*/ > +static __always_inline int > +futex_lock_pi2_64 (int *futex_word, clockid_t clockid, > + const struct __timespec64 *abstime, int private) > +{ This routine has become quite complex and I think it should move it to the sysdeps/nptl/futex-internal.c, since it is called in two places now (pthread_mutex_lock.c and pthread_mutex_timedlock.c). I think also we should move futex_lock_pi2() futex-internal.c, so if any other implementation that might want PI-futex will use the proper implementation. > + unsigned int clockbit; > + int err; > + > + if (! lll_futex_supported_clockid (clockid)) > + return EINVAL; > + > + clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0; > + int op = __lll_private_flag (FUTEX_LOCK_PI2 | clockbit, private); > + > + /* FUTEX_LOCK_PI2 is a new futex operation. It supports selectable clocks > + whereas the old FUTEX_LOCK_PI does only support CLOCK_REALTIME. > + > + Therefore, the code works like this: > + > + - If kernel support is available, then use FUTEX_LOCK_PI2 > + - If not, distuingish between clockid: For realtime use FUTEX_LOCK_PI and > + for monotonic try to use FUTEX_LOCK_PI2 which might not be available. */ > + > +#if __ASSUME_FUTEX_LOCK_PI2 > + > +# ifdef __ASSUME_TIME64_SYSCALLS > + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, 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, 0, abstime); > + if (err == -ENOSYS) > + err = -EOVERFLOW; > + } > + else > + { > + struct timespec ts32; > + > + if (abstime != NULL) > + ts32 = valid_timespec64_to_timespec (*abstime); > + > + err = INTERNAL_SYSCALL_CALL (futex, futex_word, op, 0, > + abstime != NULL ? &ts32 : NULL); > + } > +# endif /* __ASSUME_TIME64_SYSCALLS */ > + I think we can assume that if FUTEX_LOCK_PI2 is support, 64-bit time_t syscalls are also supported. > +#else > + > + /* For CLOCK_MONOTONIC the only option is to use FUTEX_LOCK_PI2 */ > + if (abstime != NULL && clockid != CLOCK_REALTIME) > + { > +# ifdef __ASSUME_TIME64_SYSCALLS > + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, 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, 0, > + abstime); > + } > + else > + { > + struct timespec ts32; > + > + if (abstime != NULL) > + ts32 = valid_timespec64_to_timespec (*abstime); > + > + err = INTERNAL_SYSCALL_CALL (futex, futex_word, op, 0, > + abstime != NULL ? &ts32 : NULL); We will hit here only if the timeout if not null, so there is no need to check it again. > + } > + > + > + /* FUTEX_LOCK_PI2 is not available on this kernel */ > + if (err == -ENOSYS) > + return EINVAL; > + } > + else > + { > + /* Otherwise use CLOCK_REALTIME and FUTEX_LOCK_PI */ > + return futex_lock_pi64 (futex_word, abstime, private); > + } > +#endif /* __ASSUME_FUTEX_LOCK_PI2 */ > + I think we can simplify to only one futex pi operation to something like: int futex_lock_pi64 (int *futex_word, const struct __timespec64 *abstime, int private) { int op_pi2 = _lll_private_flag (FUTEX_LOCK_PI2 | clockbit, private); #if __ASSUME_FUTEX_LOCK_PI2 /* Assume __ASSUME_TIME64_SYSCALLS. */ err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi2, 0, abstime); #else int op_pi1 = _lll_private_flag (FUTEX_LOCK_PI | clockbit, private), /* For CLOCK_MONOTONIC the only option is to use FUTEX_LOCK_PI2. */ 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, &ts32); } # 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 (); } } I think there is no much gain in adding another PI futex internal function, so we can just use the same name. > + 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 (); > + } > +} > + > /* Wakes the top priority waiter that called a futex_lock_pi operation on > the futex. > > diff --git a/sysdeps/nptl/lowlevellock-futex.h b/sysdeps/nptl/lowlevellock-futex.h > index 66ebfe50f4c1..abda179e0de2 100644 > --- a/sysdeps/nptl/lowlevellock-futex.h > +++ b/sysdeps/nptl/lowlevellock-futex.h > @@ -38,6 +38,7 @@ > #define FUTEX_WAKE_BITSET 10 > #define FUTEX_WAIT_REQUEUE_PI 11 > #define FUTEX_CMP_REQUEUE_PI 12 > +#define FUTEX_LOCK_PI2 13 > #define FUTEX_PRIVATE_FLAG 128 > #define FUTEX_CLOCK_REALTIME 256 > > Ok.