Date: Sun, 22 Jan 2017 10:22:28 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Konstantin Belousov <kostikbel@gmail.com>, Mateusz Guzik <mjg@freebsd.org>, 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> In-Reply-To: <20170122115716.T953@besplex.bde.org> References: <201701211838.v0LIcHIv072626@repo.freebsd.org> <20170121195114.GA2349@kib.kiev.ua> <20170122115716.T953@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170122092228.GC20930>