From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 29249C4338F for ; Mon, 9 Aug 2021 08:18:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F415B61076 for ; Mon, 9 Aug 2021 08:18:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233886AbhHIISo (ORCPT ); Mon, 9 Aug 2021 04:18:44 -0400 Received: from Galois.linutronix.de ([193.142.43.55]:34864 "EHLO galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233588AbhHIISn (ORCPT ); Mon, 9 Aug 2021 04:18:43 -0400 From: Thomas Gleixner DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1628497102; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=zlMdws6cyohkcKoE6LS+JeHUqOT6wr3OqaNW970zyGs=; b=lzL1EENgaymAqIl1jCW9hxawXEhOeoNQ2NqfUecla0ivj7gtrpx37WtJKhx09FPfM26XWK F78/CNMq06WuOrQjHsVgl5BBw65hl1EGFu0lgpFEh6I4nU0bEzZ7A5Ax7bvR5K7+QmByeL sw7oG8wz+FC6J39tRwT0phXcI9OnUqE6HQHJGJyDQdJgjWLAM+az8YQ30ULFhut53ve8FT nOKqvLGEpcn2J0S67oJq/35X2RSstOv687FhYEw82OmpFslpyR1tUZ+bGKCBUoO3jY2WG9 fG4W3L6UbHaYmKbcDxuMceL1MJCy0KeL7QALH+BsQV6L7MUdAyrqo2MFoWrYQQ== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1628497102; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=zlMdws6cyohkcKoE6LS+JeHUqOT6wr3OqaNW970zyGs=; b=vWImuFQkcz+k7EjHGJ33+34dzM/uf9L53hfMpbqU2PsmULZlQWKK/kgmbJIv3dLPA3GUJG ntuiCZc0ItqafOBw== To: Davidlohr Bueso Cc: LKML , Peter Zijlstra , Ingo Molnar , Juri Lelli , Steven Rostedt , Daniel Bristot de Oliveira , Will Deacon , Waiman Long , Boqun Feng , Sebastian Andrzej Siewior , Mike Galbraith Subject: Re: [patch V3 56/64] futex: Correct the number of requeued waiters for PI In-Reply-To: <20210808170535.kotqd5t677tijh4o@offworld> References: <20210805151300.330412127@linutronix.de> <20210805153956.051961951@linutronix.de> <20210808170535.kotqd5t677tijh4o@offworld> Date: Mon, 09 Aug 2021 10:18:22 +0200 Message-ID: <87o8a7t4j5.ffs@tglx> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Aug 08 2021 at 10:05, Davidlohr Bueso wrote: > On Thu, 05 Aug 2021, Thomas Gleixner wrote: > >>From: Thomas Gleixner >> >>The accounting is wrong when either the PI sanity check or the >>requeue PI operation fails. Adjust it in the failure path. > > Ok fortunately these accounting errors are benign considering they > are in error paths. This also made me wonder about the requeue PI > top-waiter wakeup from futex_proxy_trylock_atomic(), which is always > required with nr_wakers == 1. We account for it on the successful > case we acquired the lock on it's behalf (and thus requeue_pi_wake_futex > was called), but if the corresponding lookup_pi_state fails, we'll retry. > So, shouldn't the task_count++ only be considered when we know the > requeueing is next (a successful top_waiter acquiring the lock+pi state)? > > @@ -2260,7 +2260,6 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags, > */ > if (ret > 0) { > WARN_ON(pi_state); > - task_count++; > /* > * If we acquired the lock, then the user space value > * of uaddr2 should be vpid. It cannot be changed by > @@ -2275,6 +2274,8 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags, > */ > ret = lookup_pi_state(uaddr2, ret, hb2, &key2, > &pi_state, &exiting); > + if (!ret) > + task_count++; > } Yes, but if futex_proxy_trylock_atomic() succeeds and lookup_pi_state() fails then the user space futex value is already the VPID of the top waiter and a retry will then fail the futex_proxy_trylock_atomic(). > switch (ret) { > > Also, looking at the code, I think we can use the goto retry_private > optimization for private futexes upon futex_proxy_trylock_atomic > lookup_pi_state errors: > > @@ -2290,8 +2290,11 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags, > double_unlock_hb(hb1, hb2); > hb_waiters_dec(hb2); > ret = fault_in_user_writeable(uaddr2); > - if (!ret) > + if (!ret) { > + if (!(flags & FLAGS_SHARED)) > + goto retry_private; > goto retry; > + } Yes, we can, but let me stare at that code some more. Thanks, tglx