From owner-freebsd-hackers@FreeBSD.ORG Mon Aug 25 09:11:02 2014 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 3FE88521 for ; Mon, 25 Aug 2014 09:11:02 +0000 (UTC) Received: from mail-wi0-x22d.google.com (mail-wi0-x22d.google.com [IPv6:2a00:1450:400c:c05::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 C705933B1 for ; Mon, 25 Aug 2014 09:11:01 +0000 (UTC) Received: by mail-wi0-f173.google.com with SMTP id f8so2211247wiw.0 for ; Mon, 25 Aug 2014 02:11:00 -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=S2t4f8WgfItObdynRzwst8edJm0HbFBEgqkPCm/M+do=; b=TvpGj3SvyeJt3YfqAhpuAkjJVvWFHxBowMc06c77qzpPK+KOEOtkDJ88z27W45NiaA XKqoiIelN7ew2FuIZjTzgrfbOMgUyIEk3gF73sblQq57kf05cjnnj5REbhhNSoGizMPR FAB4faBWhBgd/VuabaP/TSALVm3OoTOQ9r9Ovw3yFO1uWMEosw9CuPE+bi7uhVrgFC4p W7T1ZyBOKwc4jC2fcM7E+yLIdE4S65V+AcgEu8caiCIWJTTRETNwRqLP9RaZKi6g1T7K KulQh2i3mDoK8IkQsgqo5o1f+106n1hNg3ZiVGn/3zKtDSo1Rna2G1ufp/jyAuOz3XRf PY3Q== X-Received: by 10.194.95.234 with SMTP id dn10mr8179532wjb.73.1408957859943; Mon, 25 Aug 2014 02:10:59 -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 jx10sm98472688wjc.7.2014.08.25.02.10.58 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 25 Aug 2014 02:10:59 -0700 (PDT) Date: Mon, 25 Aug 2014 11:10:56 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: atomic_load_acq_int in sequential_heuristic Message-ID: <20140825091056.GC14344@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> <20140825005659.GA14344@dft-labs.eu> <20140825073404.GZ2737@kib.kiev.ua> <20140825081526.GB14344@dft-labs.eu> <20140825083539.GB2737@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140825083539.GB2737@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 09:11:02 -0000 On Mon, Aug 25, 2014 at 11:35:39AM +0300, Konstantin Belousov wrote: > > + atomic_set_int(&fp->f_flag, FHASLOCK); > You misspelled FRDAHEAD as FHASLOCK, below as well. > Was this tested ? > Oops, damn copy-pasto. Sorry. > > + VOP_UNLOCK(vp, 0); > > } else { > > - do { > > - new = old = fp->f_flag; > > - new &= ~FRDAHEAD; > > - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new)); > > + atomic_clear_int(&fp->f_flag, FHASLOCK); > So what about extending the vnode lock to cover the flag reset ? > Sure. So this time I tested it properly and found out it is impossible to disable the hint. The test is: -1 is truncated and then read from intptr_t which yields a big positive number instead. Other users in the function use int tmp to work around this issue. diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index 7abdca0..774f647 100644 --- a/sys/kern/kern_descrip.c +++ b/sys/kern/kern_descrip.c @@ -760,7 +760,8 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg) error = EBADF; break; } - if (arg >= 0) { + tmp = arg; + if (tmp >= 0) { vp = fp->f_vnode; error = vn_lock(vp, LK_SHARED); if (error != 0) { @@ -769,7 +770,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg) } bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize; VOP_UNLOCK(vp, 0); - fp->f_seqcount = (arg + bsize - 1) / bsize; + fp->f_seqcount = (tmp + bsize - 1) / bsize; do { new = old = fp->f_flag; new |= FRDAHEAD; Then the patch in question: diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index 774f647..4efadb0 100644 --- a/sys/kern/kern_descrip.c +++ b/sys/kern/kern_descrip.c @@ -476,7 +476,6 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg) struct vnode *vp; cap_rights_t rights; int error, flg, tmp; - u_int old, new; uint64_t bsize; off_t foffset; @@ -760,27 +759,25 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg) error = EBADF; break; } + vp = fp->f_vnode; + /* + * Exclusive lock synchronizes against + * sequential_heuristic(). + */ + error = vn_lock(vp, LK_EXCLUSIVE); + if (error != 0) { + fdrop(fp, td); + break; + } tmp = arg; if (tmp >= 0) { - vp = fp->f_vnode; - error = vn_lock(vp, LK_SHARED); - if (error != 0) { - fdrop(fp, td); - break; - } bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize; - VOP_UNLOCK(vp, 0); fp->f_seqcount = (tmp + bsize - 1) / bsize; - do { - new = old = fp->f_flag; - new |= FRDAHEAD; - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new)); + atomic_set_int(&fp->f_flag, FRDAHEAD); } else { - do { - new = old = fp->f_flag; - new &= ~FRDAHEAD; - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new)); + atomic_clear_int(&fp->f_flag, FRDAHEAD); } + VOP_UNLOCK(vp, 0); fdrop(fp, td); break; 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