From owner-freebsd-hackers@FreeBSD.ORG Mon Aug 25 13:04:43 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 2562579D for ; Mon, 25 Aug 2014 13:04:43 +0000 (UTC) Received: from mail-wi0-x231.google.com (mail-wi0-x231.google.com [IPv6:2a00:1450:400c:c05::231]) (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 AB60D38ED for ; Mon, 25 Aug 2014 13:04:42 +0000 (UTC) Received: by mail-wi0-f177.google.com with SMTP id ho1so2483764wib.16 for ; Mon, 25 Aug 2014 06:04:40 -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=vTyHXA9oftomgYNjoGVVjIKnhHRoHuQt9EwvjxVpWLM=; b=ot4g+FFWFsjp3fOEPWq5qZCCkupfI6c4LZzrp3/soPCZIXSE9/uQ4rjJE7JeBas4Fu v+PE4IXo1tQq4DHW3jL84aR8czZi2KmZT8+5rMSvJGF2kOUaZsOLzQ+g4qAOQwSF823g 3H2y2EUc/S0i/Cfyz69quSRc1ZV11K88UA5BdNwkctvYMj5o1UTP58/XpsnlAY4XwKn6 WUHI2vDYhVTlBg/70vBpjkVTIxE3ciRoNNCHo8Ni40eg1LKciDhAqXy6rUdM2EJjOuj3 kuybhmCgtLdDdXo7VKTW1sLpPn5Qjt5+Z8RODsSaYZf/c9LS3UFbi2JJ/alqyuYp6wHc cpvw== X-Received: by 10.194.62.19 with SMTP id u19mr2270240wjr.120.1408971880198; Mon, 25 Aug 2014 06:04:40 -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 vn10sm99954074wjc.28.2014.08.25.06.04.38 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 25 Aug 2014 06:04:39 -0700 (PDT) Date: Mon, 25 Aug 2014 15:04:33 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: atomic_load_acq_int in sequential_heuristic Message-ID: <20140825130433.GD14344@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> <20140825091056.GC14344@dft-labs.eu> <20140825111000.GC2737@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140825111000.GC2737@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 13:04:43 -0000 On Mon, Aug 25, 2014 at 02:10:01PM +0300, Konstantin Belousov wrote: > On Mon, Aug 25, 2014 at 11:10:56AM +0200, Mateusz Guzik wrote: > > 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. > Could you provide me with the test case which demonstrates the problem ? > Nothing special: https://people.freebsd.org/~mjg/patches/F_READAHEAD.c > The fcntl(2) prototype in sys/fcntl.h is variadic, so int arg argument > is not promoted. On the other hand, syscalls.master declares arg as long. > Did you tried to pass -1L as third argument to disable ? > Yes, -1L deals with the problem. I would still argue that using 'tmp' like the rest of the function would not hurt as a cheap solution. > > > > 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(). > I would also add a sentence that we care about f_seqcount update in > seq_heur(). > /* * Exclusive lock synchronizes against f_seqcount reads and writes in * sequential_heuristic(). */ > Another place to add the locking annotation is the struct file in > sys/file.h. Now f_seqcount is 'protected' by the vnode lock. > I am not sure how to express the locking model shortly. > /* * (a) f_vnode lock required (shared allows both reads and writes) */ -- Mateusz Guzik