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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7EACAC001B0 for ; Thu, 13 Jul 2023 08:56:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233741AbjGMI41 (ORCPT ); Thu, 13 Jul 2023 04:56:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55822 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232663AbjGMI40 (ORCPT ); Thu, 13 Jul 2023 04:56:26 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0CA7F12E; Thu, 13 Jul 2023 01:56:26 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 9506061A91; Thu, 13 Jul 2023 08:56:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0C52AC433C7; Thu, 13 Jul 2023 08:56:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1689238585; bh=fXAs8WA2MGh2xR497240sNMs6n7reD0dpJ8m9xGMUa8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=X1yAPWUwuO4iyD+7JcINl6beVztAa0Mvu8ZNb2RKzUNYrAq/NRcZGZMN3asORDhpo C2dJYnxbmnTguhDcwem2Sy4GIEmQJ4mvKOOizyMRsOh+PeWCYWKs8XIaN1hFoEuDQq fAEBfrdoHpPADPHBtyoWkOlgC4kVeHiiOFY45LaXx2/PvfEKuc3KSSqEcpigW+R07U PXk3+5hDwMfyXNJ7tS5io/C84qln5RJywbmtwVoMKW4j6ORWQry3yFckOBCePCkUB7 SHZwnBCYVkPNlj3Nl90S3twQih+T9I1E2SDdy+zW8rID4mWj0m1Gcm8OGhuMpOBZV+ YBVmYNQ8vrreQ== Date: Thu, 13 Jul 2023 10:56:19 +0200 From: Christian Brauner To: wenyang.linux@foxmail.com Cc: Alexander Viro , Jens Axboe , Christoph Hellwig , Dylan Yudaken , David Woodhouse , Matthew Wilcox , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] eventfd: avoid unnecessary wakeups in eventfd_write() Message-ID: <20230713-wellen-heftig-b950ad3e64d2@brauner> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Thu, Jul 13, 2023 at 12:42:32AM +0800, wenyang.linux@foxmail.com wrote: > From: Wen Yang > > In eventfd_write(), when ucnt is 0 and ctx->count is also 0, > current->in_eventfd will be set to 1, which may affect eventfd_signal(), > and unnecessary wakeups will also be performed. > > Fix this issue by ensuring that ctx->count is not zero. > > Signed-off-by: Wen Yang > Cc: Alexander Viro > Cc: Jens Axboe > Cc: Christian Brauner > Cc: Christoph Hellwig > Cc: Dylan Yudaken > Cc: David Woodhouse > Cc: Matthew Wilcox > Cc: linux-fsdevel@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > fs/eventfd.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/fs/eventfd.c b/fs/eventfd.c > index 33a918f9566c..254b18ff0e00 100644 > --- a/fs/eventfd.c > +++ b/fs/eventfd.c > @@ -281,10 +281,12 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c > } > if (likely(res > 0)) { > ctx->count += ucnt; > - current->in_eventfd = 1; > - if (waitqueue_active(&ctx->wqh)) > - wake_up_locked_poll(&ctx->wqh, EPOLLIN); > - current->in_eventfd = 0; > + if (ctx->count) { > + current->in_eventfd = 1; > + if (waitqueue_active(&ctx->wqh)) > + wake_up_locked_poll(&ctx->wqh, EPOLLIN); > + current->in_eventfd = 0; > + } > } > spin_unlock_irq(&ctx->wqh.lock); I don't think we can do this. Consider the following: struct pollfd pfd = { .events = POLLIN | POLLOUT, }; int fd = eventfd(0, 0); if (fd < 0) return -1; write(fd, &w, sizeof(__u64)); poll(&pfd, 1, -1); printf("%d\n", pfd.revents & POLLOUT); Currently, the eventfd_poll() will do: ULLONG_MAX - 1 > ctx->count informing pollers with POLLOUT that the eventfd is writable, iow, that the count has overflowed. After your change such POLLOUT waiters will hang forever even though the eventfd is writable. So currently, a zero write on an eventfd can be used to inform another process that they can write. This breaks this completely. Caller's that don't want to be woken up on zero writes should just not set POLLOUT: struct pollfd pfd = { .events = POLLIN, }; int fd = eventfd(0, 0); if (fd < 0) return -1; write(fd, &w, sizeof(__u64)); poll(&pfd, 1, -1); This will wait until someone actually writes something.