From patchwork Thu Jan 29 18:56:45 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Mason X-Patchwork-Id: 5744741 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 2A1659F38B for ; Thu, 29 Jan 2015 18:56:56 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 3785F2022D for ; Thu, 29 Jan 2015 18:56:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2D07E200DB for ; Thu, 29 Jan 2015 18:56:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756331AbbA2S4v (ORCPT ); Thu, 29 Jan 2015 13:56:51 -0500 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:56317 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751528AbbA2S4u (ORCPT ); Thu, 29 Jan 2015 13:56:50 -0500 Received: from pps.filterd (m0004346 [127.0.0.1]) by mx0a-00082601.pphosted.com (8.14.5/8.14.5) with SMTP id t0TIs7Ip026852; Thu, 29 Jan 2015 10:56:48 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=fb.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=facebook; bh=XHD1RnLEJ6o/NW5c9hrprl9I/NRVlnHpkb3gRKVdi9o=; b=iUdUO6yVXMpgnzdIh8sfi8liuHEDts6Ps/gN6Rr6Ydcz3cwTybny5vghF0jvSMVQMi3K /wa7EpfCH5ijtxLU3bHRGXpOxriTtHqUv/fEH9Q5biovHiQZqEEdD7r+qyAZfzlCo5hO sMSuohDjzIsk2fE53HMnkYJrAJs+zLM6T74= Received: from mail.thefacebook.com ([199.201.64.23]) by mx0a-00082601.pphosted.com with ESMTP id 1s7s0rh429-2 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=OK); Thu, 29 Jan 2015 10:56:47 -0800 Received: from localhost (192.168.16.4) by mail.thefacebook.com (192.168.16.14) with Microsoft SMTP Server (TLS) id 14.3.195.1; Thu, 29 Jan 2015 10:56:46 -0800 Date: Thu, 29 Jan 2015 13:56:45 -0500 From: Chris Mason To: Dan Carpenter CC: Alexander Viro , Andrew Morton , , Subject: Re: [patch -next] eventfd: type bug in eventfd_poll() Message-ID: <20150129185645.GA11557@ret.DHCP.TheFacebook.com> Mail-Followup-To: Chris Mason , Dan Carpenter , Alexander Viro , Andrew Morton , linux-fsdevel@vger.kernel.org, kernel-janitors@vger.kernel.org References: <20150119193319.GA32634@mwanda> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150119193319.GA32634@mwanda> User-Agent: Mutt/1.5.23 (2014-03-12) X-Originating-IP: [192.168.16.4] X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.13.68, 1.0.33, 0.0.0000 definitions=2015-01-29_06:2015-01-29, 2015-01-29, 1970-01-01 signatures=0 X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 kscore.is_bulkscore=0 kscore.compositescore=0 circleOfTrustscore=34.730744698248 compositescore=0.928746566069775 urlsuspect_oldscore=0.928746566069775 suspectscore=0 recipient_domain_to_sender_totalscore=0 phishscore=0 bulkscore=0 kscore.is_spamscore=0 recipient_to_sender_totalscore=0 recipient_domain_to_sender_domain_totalscore=64355 rbsscore=0.928746566069775 spamscore=0 recipient_to_sender_domain_totalscore=12 urlsuspectscore=0.9 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=7.0.1-1402240000 definitions=main-1501290183 X-FB-Internal: deliver Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID,T_RP_MATCHES_RCVD,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Jan 19, 2015 at 10:33:19PM +0300, Dan Carpenter wrote: > Since "count" is an unsigned int, then these conditions are never true: > > if (count == ULLONG_MAX) > events |= POLLERR; > if (ULLONG_MAX - 1 > count) > events |= POLLOUT; > > It should be a u64, because that's what ctx->count is. Also GCC > complains that "flags" is unused. > > Fixes: a90de8a54127 ('eventfd: don't take the spinlock in eventfd_poll') > Signed-off-by: Dan Carpenter > > diff --git a/fs/eventfd.c b/fs/eventfd.c > index 439e6f0..8d0c0df 100644 > --- a/fs/eventfd.c > +++ b/fs/eventfd.c > @@ -118,8 +118,7 @@ static unsigned int eventfd_poll(struct file *file, poll_table *wait) > { > struct eventfd_ctx *ctx = file->private_data; > unsigned int events = 0; > - unsigned long flags; > - unsigned int count; > + u64 count; > > poll_wait(file, &ctx->wqh, wait); > smp_rmb(); Andrew, not sure if you want to take Dan's incremental or a new v2. I ran this one through the ltp poll and event fd tests. From: Chris Mason Date: Wed, 28 Jan 2015 10:15:58 -0800 Subject: [PATCH v2] eventfd: don't take the spinlock in eventfd_poll The spinlock in eventfd_poll is trying to protect the count of events so it can decide if it should return POLLIN, POLLERR, or POLLOUT. But, because of the way we drop the lock after calling poll_wait, and drop it again before returning, we have the same pile of races with the lock as we do with a single read of ctx->count(). This replaces the lock with a read barrier and single read. eventfd_write does a single bump of ctx->count, so this should not add new races with adding events. eventfd_read is similar, it will do a single decrement with the lock held, and so we're making the race with concurrent readers slightly larger. This spinlock is the top CPU user in kernel code during one of our workloads. Removing it gives us a ~2% boost. Signed-off-by: Chris Mason Fixed-by: Dan Carpenter --- fs/eventfd.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) v1->v2 use a u64 for count and get rid of unused flags (Dan Carpenter) diff --git a/fs/eventfd.c b/fs/eventfd.c index 4b0a226..8d0c0df 100644 --- a/fs/eventfd.c +++ b/fs/eventfd.c @@ -118,18 +118,18 @@ static unsigned int eventfd_poll(struct file *file, poll_table *wait) { struct eventfd_ctx *ctx = file->private_data; unsigned int events = 0; - unsigned long flags; + u64 count; poll_wait(file, &ctx->wqh, wait); + smp_rmb(); + count = ctx->count; - spin_lock_irqsave(&ctx->wqh.lock, flags); - if (ctx->count > 0) + if (count > 0) events |= POLLIN; - if (ctx->count == ULLONG_MAX) + if (count == ULLONG_MAX) events |= POLLERR; - if (ULLONG_MAX - 1 > ctx->count) + if (ULLONG_MAX - 1 > count) events |= POLLOUT; - spin_unlock_irqrestore(&ctx->wqh.lock, flags); return events; }