From owner-svn-src-head@freebsd.org Sun Jan 22 09:22:34 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7CD4BCBC461; Sun, 22 Jan 2017 09:22:34 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm0-x242.google.com (mail-wm0-x242.google.com [IPv6:2a00:1450:400c:c09::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 1120035E; Sun, 22 Jan 2017 09:22:34 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm0-x242.google.com with SMTP id d140so18264816wmd.2; Sun, 22 Jan 2017 01:22:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=CDKrYz+R5WqJ8/08Wq9a5MiR5J3mbQyN2KHJQt2bbqw=; b=F2L6QQZf0bmpRi9wBa1dkukvuJa6eEmI56vLPPIV+/HF4go3JNw28APaaRkELptjQL 32idnIHUDd+ESTGdvPsfSivl1mfcPq08p+uAi1tu7E/k1XgXvHPxLnwcU98aEXWinnPY 5T6ksh6F2NurA0kctNtP0RotYwzdr64iVhDZPTMLGlXMhp90lA8OIPaqEB0JpytnBzSf i82B0Qoeu6Id08vTfEIIhLvBp1tx+JHuukboyL/jk8S4AEfZGwpvwtStc2NkXxgsOnpU X1sJAHHOx/E9wlzsLDlwSV0YEOqOS3JZG0556VJBLHCFo7wsVpgJ7MPVxxrW3LB5zslS jcvw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=CDKrYz+R5WqJ8/08Wq9a5MiR5J3mbQyN2KHJQt2bbqw=; b=V6YQrH3LPyk/QSm46OB+zSwGs813SHTZm3Q+A5cZT80VtHM6jXbZnUJ1wn6lBPhbt1 qpqPwmTJ4U/cbOqSISKqEIqrZDDWREeYt9x3gczM2BjIsTyHwwGAtau22TXi8wntfmuF yHf9wW6sQ0zdJw7JT4oqf7W5um03tAlh4wiFIwHDoGBZKVqVzwODwuV2YT6WHyPXSfRu UKbjfG4w56wa26X8BfotVlax/rsVFcKWvMUNbaipdwkYrANV3f5rE/Opk9urAwn64TCd ftpPiZqDZR5A5X/aiDPjP3VQKPDU4GC8bW8OsvEuqibZllVDoTey0LomCxquL7Ov1AC3 C2Qg== X-Gm-Message-State: AIkVDXIt/MaEZewY1B6HT+FsHJUh1t1YTNJLssM9h8So6HIc3RUIbWXZBTuSQRiQM2npQg== X-Received: by 10.223.136.197 with SMTP id g5mr18727744wrg.56.1485076952242; Sun, 22 Jan 2017 01:22:32 -0800 (PST) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by smtp.gmail.com with ESMTPSA id 36sm8446141wrz.8.2017.01.22.01.22.30 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Sun, 22 Jan 2017 01:22:31 -0800 (PST) Date: Sun, 22 Jan 2017 10:22:28 +0100 From: Mateusz Guzik To: Bruce Evans Cc: Konstantin Belousov , Mateusz Guzik , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r312600 - head/sys/kern Message-ID: <20170122092228.GC20930@dft-labs.eu> References: <201701211838.v0LIcHIv072626@repo.freebsd.org> <20170121195114.GA2349@kib.kiev.ua> <20170122115716.T953@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20170122115716.T953@besplex.bde.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 22 Jan 2017 09:22:34 -0000 On Sun, Jan 22, 2017 at 12:07:56PM +1100, Bruce Evans wrote: > On Sat, 21 Jan 2017, Konstantin Belousov wrote: > > >On Sat, Jan 21, 2017 at 06:38:17PM +0000, Mateusz Guzik wrote: > >>... > >>Log: > >> vfs: refactor _vn_lock > >> > >> Stop testing for LK_RETRY and error multiple times. Also postpone the > >> VI_DOOMED until after LK_RETRY was seen as it reads from the vnode. > >> > >> No functional changes. > > Many style bugs. > Indeed, the commit was weirdly bad. > >>+ flags &= ~LK_INTERLOCK; /* Interlock is always dropped. */ > >>+ KASSERT((flags & LK_RETRY) == 0 || error == 0, > >>+ ("LK_RETRY set with incompatible flags (0x%x) or an error occurred (%d)", > >>+ flags, error)); > > This is even more grossly misreformatted than the other KASSERT(). The new > style bugs are: > - random first contuation indent (it happens to be 2 tabs) > - long line longer than before (further messed up by the indentation) > - random second contuation indent (it actually lines up with the first, > except it uses gnu-style lining up parentheses instead of KNF) > There was a subsequent commit addressing it, see r312601. > >>+ if (flags & LK_RETRY) { > >Stylish test is > > if ((flags & LK_RETRY) != 0) { > >>+ if ((error != 0)) > >Too many (). > > > >>+ goto retry; > >>+ if ((vp->v_iflag & VI_DOOMED)) { > >Too many braces again, or missed != 0. > >> VOP_UNLOCK(vp, 0); > >> error = ENOENT; > >>- break; > >Also, this does the functional change, it seems to completely break LK_RERY > >logic. If LK_RETRY is specified, VI_DOOMED must not result in ENOENT. > > Later commits further unimproved style by adding __predict_ugly() and > long lines from blind substitution of that. __predict_ugly() is almost > as useful as 'register', but more invasive. > There were no __predict changes to this function. I did add some to vn_closefile in r312602. Indeed, one line is now 82 chars and arguably could be modified to fit the limit. I have to disagree about the usefulness remark. If you check generated assembly for amd64 (included below), you will see the uncommon code is moved out of the way and in particular there are no forward jumps in the common case. With __predict_false: [snip prologue] 0xffffffff8084ecaf <+31>: mov 0x24(%rbx),%eax 0xffffffff8084ecb2 <+34>: test $0x40,%ah 0xffffffff8084ecb5 <+37>: jne 0xffffffff8084ece2 0xffffffff8084ecb7 <+39>: mov 0x24(%rbx),%esi 0xffffffff8084ecba <+42>: mov 0x10(%rbx),%rdx 0xffffffff8084ecbe <+46>: mov %r14,%rdi 0xffffffff8084ecc1 <+49>: mov %r15,%rcx 0xffffffff8084ecc4 <+52>: callq 0xffffffff80850000 0xffffffff8084ecc9 <+57>: mov %eax,%r15d 0xffffffff8084eccc <+60>: mov 0x24(%rbx),%eax 0xffffffff8084eccf <+63>: test $0x40,%ah 0xffffffff8084ecd2 <+66>: jne 0xffffffff8084ecf5 [snip cleanup] 0xffffffff8084ece1 <+81>: retq Without __predict_false: [snip prologue] 0xffffffff8084ecaf <+31>: mov 0x24(%rbx),%eax 0xffffffff8084ecb2 <+34>: test $0x40,%ah 0xffffffff8084ecb5 <+37>: je 0xffffffff8084ecc8 0xffffffff8084ecb7 <+39>: movzwl 0x20(%rbx),%eax 0xffffffff8084ecbb <+43>: cmp $0x1,%eax 0xffffffff8084ecbe <+46>: jne 0xffffffff8084ecc8 0xffffffff8084ecc0 <+48>: mov %r14,%rdi 0xffffffff8084ecc3 <+51>: callq 0xffffffff8083e270 0xffffffff8084ecc8 <+56>: mov 0x24(%rbx),%esi 0xffffffff8084eccb <+59>: mov 0x10(%rbx),%rdx 0xffffffff8084eccf <+63>: mov %r14,%rdi 0xffffffff8084ecd2 <+66>: mov %r15,%rcx 0xffffffff8084ecd5 <+69>: callq 0xffffffff80850000 0xffffffff8084ecda <+74>: mov %eax,%r15d 0xffffffff8084ecdd <+77>: mov 0x24(%rbx),%eax 0xffffffff8084ece0 <+80>: test $0x40,%ah 0xffffffff8084ece3 <+83>: je 0xffffffff8084ed45 0xffffffff8084ece5 <+85>: movzwl 0x20(%rbx),%eax 0xffffffff8084ece9 <+89>: cmp $0x1,%eax 0xffffffff8084ecec <+92>: jne 0xffffffff8084ed45 0xffffffff8084ecee <+94>: movw $0x0,-0x22(%rbp) [skip some parts, +181 marks the beginning of cleanup] 0xffffffff8084ed52 <+194>: retq -- Mateusz Guzik