From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x82f.google.com (mail-qt1-x82f.google.com [IPv6:2607:f8b0:4864:20::82f]) by sourceware.org (Postfix) with ESMTPS id C0F3C3851C2E for ; Mon, 21 Jun 2021 21:32:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C0F3C3851C2E Received: by mail-qt1-x82f.google.com with SMTP id c22so7026723qtn.1 for ; Mon, 21 Jun 2021 14:32:45 -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=3NDwBrA/jn8ro5WB0dCgC+9JXzdeN8zRmq6EKD1WTO4=; b=RAlop6JOPcn6drfqAUFzCEqSCteLdDzUBosKCKIssId+s8l6ImFZzKjg6nu9r2dzey +C44gPv1CyEQqfqqtB+kxaRalywWB5IspWuBHpUoeCA9sc/eERjaW4qkbdPovCMLs9IH gUnI0ANhh9+9hY4tSdK0MD0Ck1kyKPcqXCLAr6Z7fZoibsZRN5jrJziNTYEw4b0wThCm 0WXo6XdppoULjPSUR8atCZ30si30ne/ekCqxiZym87bt/Z/X2RoCwQ3cYJic39QRPHIl vPp2gsOH3aLetsoH5UP/oT6jd4jwrdXqoEqeSApxAOYpuWkrx8uLxlf8VpVxVHFKxgpp xsWA== X-Gm-Message-State: AOAM531JDsleUkAeRVf9O7lLnPc6d/YimHlEqTL0P9XL3N3PDfQd70zg QNGbei1gSsiUgNwU36eklS9Xhw== X-Google-Smtp-Source: ABdhPJzJCMvnoaThC7qRIsVRJ/+N4f8EmhkyyVWkHf0fVUQHkqnru84h7QbIKqegk1ZgFZyqisYMJw== X-Received: by 2002:a05:622a:392:: with SMTP id j18mr570938qtx.195.1624311165349; Mon, 21 Jun 2021 14:32:45 -0700 (PDT) Received: from [192.168.1.108] ([177.194.59.218]) by smtp.gmail.com with ESMTPSA id e3sm245447qts.34.2021.06.21.14.32.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 21 Jun 2021 14:32:45 -0700 (PDT) Subject: Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2 To: Kurt Kanzenbach , libc-alpha@sourceware.org Cc: Florian Weimer , Carlos O'Donell , Thomas Gleixner , Sebastian Andrzej Siewior References: <20210621111650.1164689-1-kurt@linutronix.de> From: Adhemerval Zanella Message-ID: Date: Mon, 21 Jun 2021 18:32:41 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <20210621111650.1164689-1-kurt@linutronix.de> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_NUMSUBJECT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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: Mon, 21 Jun 2021 21:32:47 -0000 On 21/06/2021 08:16, Kurt Kanzenbach wrote: > Hi, > > Linux real time application often make use of mutexes with priority inheritance > enabled due to problems such as unbounded priority inversion. In addition, some > applications make use of timeouts e.g., by utilizing pthread_mutex_clocklock(). > > However, the combination of priority inheritance enabled mutexes with timeouts > based on CLOCK_MONOTONIC doesn't work. That is because the underlying Linux > kernel futex interface doesn't support it, yet. Using CLOCK_REALTIME is > possible, but it is subject to adjustments (NTP, PTP, ...). Therefore, Thomas > proposed to add a new futex operation called FUTEX_LOCK_PI2 with support for > selectable clocks [1]. That patch series is not merged, yet. > > Here is a proof of concept patch set adding FUTEX_LOCK_PI2 in order to support > pthread_mutex_clocklock(MONOTONIC)/PI. The idea is to use futex_lock_pi2() by > default, and fallback to futex_lock_pi() in case the kernel is too old. I think, > you get the idea. > > There's also a bugreport for glibc with a test case: > > https://sourceware.org/bugzilla/show_bug.cgi?id=26801 > > Thoughts? > > Thanks, > Kurt Currently we check for PI mutex support on pthread_mutex_init which basically check for futex_cmpxchg_enabled within kernel (so it fails only on a handful configurations). For FUTEX_LOCK_PI2 I think we will need to rework it, since we are moving the futex PI failure from pthread_mutex_init to pthread_mutex_{timed}lock. It means that we will need to remove the prio_inherit_missing() test on pthread_mutex_init and make the pthread_mutex_{timed}lock fail instead (not sure if we should use ENOTSUP or keep with current EINVAL). The proposed futex_lockpi2_supported() incurs in an extra syscall on every mutex slow path, we should avoid it. I would like also to avoid the CRIU issue as well, so I think it would be better to avoid any caching (as done by prio_inherit_missing()), and optimize the FUTEX_LOCK_PI2 to be used only when the timeout for clock different than CLOCK_REALTIME is required. So instead it would be better to move the logic solely on futex_lock_pi() (I am assuming FUTEX_LOCK_PI2 would be only added for futex_time64): static __always_inline int futex_lock_pi2_64 (int *futex_word, const struct __timespec64 *abtime, int private) { # if __ASSUME_FUTEX_LOCK_PI2 return INTERNAL_SYSCALL_CALL (futex_time64, futex_word, __lll_private_flag (FUTEX_LOCK_PI2, private), 0, abstime); # else if (abstime != NULL && clockid != CLOCK_REALTIME) return INTERNAL_SYSCALL_CALL (futex_time64, futex_word, __lll_private_flag (FUTEX_LOCK_PI2, private), 0, abstime); else { int err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, __lll_private_flag (FUTEX_LOCK_PI, private), 0, abstime); if (err == -ENOSYS) err = -EOVERFLOW; } # endif /* __ASSUME_FUTEX_LOCK_PI2 */ } static __always_inline int futex_lock_pi64 (int *futex_word, const struct __timespec64 *abstime, int private) { int err; #ifdef __ASSUME_TIME64_SYSCALLS err = futex_lock_pi2_64 (futex_word, abstime, private); #else /* __ASSUME_TIME64_SYSCALLS */ bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec) if (need_time64) { err = futex_lock_pi2_64 (futex_word, abstime, private); } else { struct timespec ts32; if (abstime != NULL) ts32 = valid_timespec64_to_timespec (*abstime); err = INTERNAL_SYSCALL_CALL (futex, futex_word, __lll_private_flag (FUTEX_LOCK_PI, private), 0, abstime != NULL ? &ts32 : NULL); } #endif /* __ASSUME_TIME64_SYSCALLS */ [...] } It would make the changes on pthread_mutex code minimal, it would be only to remove the extra check for clockid and adjust the comment. Also, as Joseph has noted the __ASSUME_FUTEX_LOCK_PI2 should be the first patch. It also does not make sense to add the __ASSUME_FUTEX_LOCK_PI2 on tests, they need to be kernel agnostic so you will need to handle a possible EINVAL/ENOSUP failure instead. > > [1] - https://lkml.kernel.org/lkml/20210422194417.866740847@linutronix.de/ > > Kurt Kanzenbach (3): > nptl: Introduce futex_lock_pi2() > nptl: Use futex_lock_pi2() > nptl: Include CLOCK_MONOTONIC in mutex tests > > nptl/pthread_mutex_timedlock.c | 24 ++++-- > nptl/tst-mutexpi10.c | 8 ++ > sysdeps/nptl/futex-internal.h | 94 +++++++++++++++++++++++ > sysdeps/nptl/lowlevellock-futex.h | 1 + > sysdeps/pthread/tst-mutex5.c | 3 +- > sysdeps/pthread/tst-mutex9.c | 3 +- > sysdeps/unix/sysv/linux/kernel-features.h | 8 ++ > 7 files changed, 133 insertions(+), 8 deletions(-) >