svn commit: r312600 - head/sys/kern

[Mateusz Guzik]

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 <vn_closefile+82>
   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 <vn_close>
   0xffffffff8084ecc9 <+57>:	mov    %eax,%r15d
   0xffffffff8084eccc <+60>:	mov    0x24(%rbx),%eax
   0xffffffff8084eccf <+63>:	test   $0x40,%ah
   0xffffffff8084ecd2 <+66>:	jne    0xffffffff8084ecf5 <vn_closefile+101>
[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 <vn_closefile+56>
   0xffffffff8084ecb7 <+39>:	movzwl 0x20(%rbx),%eax
   0xffffffff8084ecbb <+43>:	cmp    $0x1,%eax
   0xffffffff8084ecbe <+46>:	jne    0xffffffff8084ecc8 <vn_closefile+56>
   0xffffffff8084ecc0 <+48>:	mov    %r14,%rdi
   0xffffffff8084ecc3 <+51>:	callq  0xffffffff8083e270 <vrefact>
   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 <vn_close>
   0xffffffff8084ecda <+74>:	mov    %eax,%r15d
   0xffffffff8084ecdd <+77>:	mov    0x24(%rbx),%eax
   0xffffffff8084ece0 <+80>:	test   $0x40,%ah
   0xffffffff8084ece3 <+83>:	je     0xffffffff8084ed45 <vn_closefile+181>
   0xffffffff8084ece5 <+85>:	movzwl 0x20(%rbx),%eax
   0xffffffff8084ece9 <+89>:	cmp    $0x1,%eax
   0xffffffff8084ecec <+92>:	jne    0xffffffff8084ed45 <vn_closefile+181>
   0xffffffff8084ecee <+94>:	movw   $0x0,-0x22(%rbp)
[skip some parts, +181 marks the beginning of cleanup]
   0xffffffff8084ed52 <+194>:	retq   

-- 
Mateusz Guzik <mjguzik gmail.com>