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=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_1 autolearn=no 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 691F5C433E0 for ; Thu, 18 Feb 2021 20:12:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 19A9960230 for ; Thu, 18 Feb 2021 20:12:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230403AbhBRUMR (ORCPT ); Thu, 18 Feb 2021 15:12:17 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:43052 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231441AbhBRUJx (ORCPT ); Thu, 18 Feb 2021 15:09:53 -0500 Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: tonyk) with ESMTPSA id 482F51F45D91 Subject: Re: [RFC PATCH 01/13] futex2: Implement wait and wake functions To: Peter Zijlstra Cc: Thomas Gleixner , Ingo Molnar , Darren Hart , linux-kernel@vger.kernel.org, Steven Rostedt , Sebastian Andrzej Siewior , kernel@collabora.com, krisman@collabora.com, pgriffais@valvesoftware.com, z.figura12@gmail.com, joel@joelfernandes.org, malteskarupke@fastmail.fm, linux-api@vger.kernel.org, fweimer@redhat.com, libc-alpha@sourceware.org, linux-kselftest@vger.kernel.org, shuah@kernel.org, acme@kernel.org, corbet@lwn.net References: <20210215152404.250281-1-andrealmeid@collabora.com> <20210215152404.250281-2-andrealmeid@collabora.com> From: =?UTF-8?Q?Andr=c3=a9_Almeida?= Message-ID: Date: Thu, 18 Feb 2021 17:09:02 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Peter, Às 06:02 de 16/02/21, Peter Zijlstra escreveu: > On Mon, Feb 15, 2021 at 12:23:52PM -0300, André Almeida wrote: >> +static int __futex_wait(struct futexv_head *futexv, unsigned int nr_futexes, >> + struct hrtimer_sleeper *timeout) >> +{ >> + int ret; >> + >> + while (1) { >> + int awakened = -1; >> + > > Might be easier to understand if the set_current_state() is here, > instead of squirreled away in futex_enqueue(). > I placed set_current_state() inside futex_enqueue() because we need to set RUNNING and then INTERRUPTIBLE again for the retry path. >> + ret = futex_enqueue(futexv, nr_futexes, &awakened); >> + >> + if (ret) { >> + if (awakened >= 0) >> + return awakened; >> + return ret; >> + } >> + >> + /* Before sleeping, check if someone was woken */ >> + if (!futexv->hint && (!timeout || timeout->task)) >> + freezable_schedule(); >> + >> + __set_current_state(TASK_RUNNING); > > This is typically after the loop. > Sorry, which loop? >> + >> + /* >> + * One of those things triggered this wake: >> + * >> + * * We have been removed from the bucket. futex_wake() woke >> + * us. We just need to dequeue and return 0 to userspace. >> + * >> + * However, if no futex was dequeued by a futex_wake(): >> + * >> + * * If the there's a timeout and it has expired, >> + * return -ETIMEDOUT. >> + * >> + * * If there is a signal pending, something wants to kill our >> + * thread, return -ERESTARTSYS. >> + * >> + * * If there's no signal pending, it was a spurious wake >> + * (scheduler gave us a change to do some work, even if we > > chance? Indeed, fixed. > >> + * don't want to). We need to remove ourselves from the >> + * bucket and add again, to prevent losing wakeups in the >> + * meantime. >> + */ > > Anyway, doing a dequeue and enqueue for spurious wakes is a bit of an > anti-pattern that can lead to starvation. I've not actually looked at > much detail yet as this is my first read-through, but did figure I'd > mention it. > So we could just leave everything enqueued for spurious wake? I was expecting that we would need to do all the work again (including rechecking *uaddr == val) to see if we didn't miss a futex_wake() between the kernel thread waking (spuriously) and going to sleep again. >> + >> + ret = futex_dequeue_multiple(futexv, nr_futexes); >> + >> + /* Normal wake */ >> + if (ret >= 0) >> + return ret; >> + >> + if (timeout && !timeout->task) >> + return -ETIMEDOUT; >> + >> + if (signal_pending(current)) >> + return -ERESTARTSYS; >> + >> + /* Spurious wake, do everything again */ >> + } >> +} Thanks, André