From owner-freebsd-hackers@FreeBSD.ORG Mon Aug 25 00:57:06 2014 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E8B82F81 for ; Mon, 25 Aug 2014 00:57:05 +0000 (UTC) Received: from mail-wg0-x22d.google.com (mail-wg0-x22d.google.com [IPv6:2a00:1450:400c:c00::22d]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 801873937 for ; Mon, 25 Aug 2014 00:57:05 +0000 (UTC) Received: by mail-wg0-f45.google.com with SMTP id x12so12294336wgg.28 for ; Sun, 24 Aug 2014 17:57:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=iiJtjYh4LzOY5DiI0SEhjWlI+I3aMPKIKwz8oXxBi2s=; b=X+/T8T6ptyb7vRwo2wK4+FYBnIjaVkU754PgBhycGPjC0RhriPbxnuMsIi9hJqA66u Uxtr7FO+L1muTFotQohSA33nITXkO09GeizvJW9VWXkAsIUTLEdYf7QYzdqFM1qssqMs KU8oyEq6+VyFZlBEDvnNwSXX7N57QS8fpyDG8p1Oh4ZNf2mrOpr4IDEXAbU7AyeWJyKI ypajlUwh1FKfY27wU1j/nOdJuGXq7L27MQtxmLF22OXoSOUl+KXnjKErKLBRVx19LkV9 KSWCpqGRVNbsxFY2qvaLflb0KwvPYMwkIRifw05tpEFs24Jn5TR8OOomIvCGsd2rCqXx aaJQ== X-Received: by 10.180.83.8 with SMTP id m8mr11970012wiy.8.1408928223810; Sun, 24 Aug 2014 17:57:03 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id p3sm95693451wjb.21.2014.08.24.17.57.02 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sun, 24 Aug 2014 17:57:02 -0700 (PDT) Date: Mon, 25 Aug 2014 02:57:00 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: atomic_load_acq_int in sequential_heuristic Message-ID: <20140825005659.GA14344@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Konstantin Belousov , freebsd-hackers@freebsd.org References: <20140824115729.GC2045@dft-labs.eu> <20140824162331.GW2737@kib.kiev.ua> <20140824164236.GX2737@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140824164236.GX2737@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-hackers@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Aug 2014 00:57:06 -0000 On Sun, Aug 24, 2014 at 07:42:36PM +0300, Konstantin Belousov wrote: > On Sun, Aug 24, 2014 at 07:23:31PM +0300, Konstantin Belousov wrote: > > On Sun, Aug 24, 2014 at 01:57:29PM +0200, Mateusz Guzik wrote: > > > Writer side is: > > > fp->f_seqcount = (arg + bsize - 1) / bsize; > > > do { > > > new = old = fp->f_flag; > > > new |= FRDAHEAD; > > > } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new)); > > > > > > Reader side is: > > > if (atomic_load_acq_int(&(fp->f_flag)) & FRDAHEAD) > > > return (fp->f_seqcount << IO_SEQSHIFT); > > > > > > We can easily get the following despite load_acq: > > > CPU0 CPU1 > > > load_acq fp->f_flag > > > fp->f_seqcount = ... > > > store_rel fp->f_flag > > > read fp->f_seqcount > > > > > > So the barrier does not seem to serve any purpose. > > It does. > > > > Consider initial situation, when the flag is not set yet. There, we > > do not want to allow the reader to interpret automatically calculated > > f_seqcount as the user-supplied constant. Without barriers, we might > > read the flag as set, while user-provided value for f_seqcount is still > > not visible to processor doing read. > That said, I think now that there is a real bug. > > If we did not read the FRDAHEAD in sequential_heuristic(), we may > override user-supplied value for f_seqcount. I do not see other > solution than start to use locking. Right. How about abusing vnode lock for this purpose? All callers of sequential_heuristic have the vnode at least shared locked. FRDAHEAD setting code locks it shared. We can change that to exclusive, which will close the race and should not be problematic given that it is rather rare. diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index 7abdca0..643920b 100644 --- a/sys/kern/kern_descrip.c +++ b/sys/kern/kern_descrip.c @@ -762,18 +762,18 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg) } if (arg >= 0) { vp = fp->f_vnode; - error = vn_lock(vp, LK_SHARED); + error = vn_lock(vp, LK_EXCLUSIVE); if (error != 0) { fdrop(fp, td); break; } bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize; - VOP_UNLOCK(vp, 0); fp->f_seqcount = (arg + bsize - 1) / bsize; do { new = old = fp->f_flag; new |= FRDAHEAD; } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new)); + VOP_UNLOCK(vp, 0); } else { do { new = old = fp->f_flag; diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c index f1d19ac..98823f3 100644 --- a/sys/kern/vfs_vnops.c +++ b/sys/kern/vfs_vnops.c @@ -438,7 +438,8 @@ static int sequential_heuristic(struct uio *uio, struct file *fp) { - if (atomic_load_acq_int(&(fp->f_flag)) & FRDAHEAD) + ASSERT_VOP_LOCKED(fp->f_vnode, __func__); + if (fp->f_flag & FRDAHEAD) return (fp->f_seqcount << IO_SEQSHIFT); /* -- Mateusz Guzik