git: af366d353b84 - main - amd64: implement strlen in assembly

Tue Feb 9 13:41:18 UTC 2021

On 2/9/21, Jessica Clarke <jrtc27 at freebsd.org> wrote:
>> On 8 Feb 2021, at 23:13, Kevin Bowling <kevin.bowling at kev009.com> wrote:
>>
>> FreeBSD does not require pre-commit approval unless called out
>> specifically.  Are you volunteering to review the changes, and if so
>> where is your guidance?  These messages are otherwise unhelpful.
>
> It is not a hard requirement, but it is strongly encouraged. Section 7
> of the committer's guide says:
>
> 	• All non-trivial changes should be reviewed before they are
> 	committed to the repository.
>
> This was a non-trivial change. I was particularly frustrated to see
> this commit go in without review having previously called out mjg@ for
> not getting any reviews for his (now reverted) previous strlen change.
>

There is a lot to say here and I don't know if I'll manage to make my
points come across. First I'll address reviews in general and then
I'll comment on strlen.

The key is that reviews in this project notoriously fail to
reduce/prevent breakage. I know this because several patches written
by me which got accepted with little to no objection turned out to
have problems which would have been found if review was better. I also
know because I see the very same problem with reviews involving other
people. Note I'm not talking about expecting reviews to guarantee
bug-free commits. I'm talking about bugs one would expect a review to
find, but did not. It got to the point where I put no stock in
'reviewed by'. Examples below.

Ultimately what reduces/prevents breakage is testing and that's what
I'm normally doing. Of course not all problems can be found in
testing, so this boils down to making a call whether a review is
warranted and what it can do to begin with.

I do think a review is mandatory when making a non-cosmetic change to
an area worked on by someone else. Similarly, if there are multiple
people active somewhere, it's probably best to coordinate. Review may
be a great idea if a design choice has to be made or certain people
have expertise from the problem domain, even if they are not active
somewhere.

I don't believe the project has manpower to perform good quality code
reviews of every single change. The developer has to make a judgment
call whether the change warrants a review and sometimes they will make
a mistake here, just like even a good review will sometimes fail to
spot problems it otherwise would. I do believe shallow reviews allow
people to dodge criticism for breakage -- after all, someone else
accepted the change.

Examples from recent past (all fixed):
https://reviews.freebsd.org/D28091 Add -NODEBUG variant of
GENERIC-MMCCAM kernel configuration.
Some discussion, accepted by 2 people, has a typo in the kernel config
breaking tinderbox

https://reviews.freebsd.org/D28102 amd64: compare TLB shootdown target
to all_cpus
Nothing pointed out, accepted by 4 people, crashes on boot when the
kernel is built with INVARIANTS.

https://reviews.freebsd.org/D28284 elfctl: allow features to be
specified by value
Nothing pointed out, accepted by 1 person, has '!=' instead of '|='
breaking the change.

https://reviews.freebsd.org/D28306 nfs client: block
vnode_pager_setsize() calls from nfscl_loadattrcache in nfs_write
There was a question about something but the patch got accepted,
breaks writes by partially pushing garbage into files.

https://reviews.freebsd.org/D27609 ndis(4): remove as previous announced
Nothing pointed out, accepted by 1 person. Breaks the build in 2 ways
and leaves leftovers (grep git log for ndis and you will see what I
mean). Change like this is arguably to review, but bare minimum which
could have been done is to ask if they built tinderbox and perhaps git
grep -i ndis on your own.

This in my experience is the standard.

Which brings me to C strlen.

Before committing my patch to the C variant I did the due diligence
for little endian -- I have glibc test suite up and running, I have a
small test program on my own and finally I ran several workloads on
it. For big endian I only ran the small test program on ref13-ppc64
and called it a day. That was a failure to do the due diligence -- I
should spent time to set up the test suite in that environment, but
could not be arsed to do it. I should have also asked someone running
the hardware to the code by just using it. Bad call on my end, a bug
was reported and I promptly reverted the change.

Would a review prevent the problem? That is plausible, but given track
record indicated above, I would not count on it. More, if reviews were
mandatory, I would expect their quality to go down even further,
making them even less likely to prevent breakage.

And now we reach this change.

I did the due diligence in testing (glibc, my test jig, actual
workloads). Because of this, while I can never claim the routine is
bug free, I doubt bugs (if any) would be found in a review and this is
why I did not ask for one.

In contrast, here are examples from my TODO list where I do plan to
get a review if I write a patch:
- nofault/onfault so that C routines can handle interaction with
userspace memory -- several decisions to make here
- hot patching of sdt probes -- same as above
- copyinstr using the mycroft trick -- possibly questionable hacks pop up

That said, I do own to my mistake with C strlen (shallow testing for
big endian) and I do stand behind not getting a review for this patch.

I believe you have good intentions, but are simply suffering from
confirmation bias. Perhaps the breakage introduced by me is something
which popped up for you.

If you want an example of something which is frustrating, I suggest
you take a look at how often the build is broken and who keeps
reporting or fixing it.

>> On Mon, Feb 8, 2021 at 12:37 PM Jessica Clarke <jrtc27 at freebsd.org>
>> wrote:
>>>
>>> On 8 Feb 2021, at 19:15, Mateusz Guzik <mjg at FreeBSD.org> wrote:
>>>>
>>>> The branch main has been updated by mjg:
>>>>
>>>> URL:
>>>> https://cgit.FreeBSD.org/src/commit/?id=af366d353b84bdc4e730f0fc563853abc338271c
>>>>
>>>> commit af366d353b84bdc4e730f0fc563853abc338271c
>>>> Author:     Mateusz Guzik <mjg at FreeBSD.org>
>>>> AuthorDate: 2021-02-08 17:01:48 +0000
>>>> Commit:     Mateusz Guzik <mjg at FreeBSD.org>
>>>> CommitDate: 2021-02-08 19:15:21 +0000
>>>>
>>>>   amd64: implement strlen in assembly
>>>>
>>>>   The C variant in libkern performs excessive branching to find the
>>>>   non-zero byte instead of using the bsfq instruction. The same code
>>>>   patched to use it is still slower than the routine implemented here
>>>>   as the compiler keeps neglecting to perform certain optimizations
>>>>   (like using leaq).
>>>>
>>>>   On top of that the routine can is a starting point for copyinstr
>>>>   which operates on words instead of bytes.
>>>>
>>>>   Tested with glibc test suite.
>>>>
>>>>   Sample results (calls/s):
>>>>
>>>>   Haswell:
>>>>   $(perl -e "print 'A' x 3"):
>>>>   stock:  211198039
>>>>   patched:338626619
>>>>   asm:    465609618
>>>>
>>>>   $(perl -e "print 'A' x 100"):
>>>>   stock:   83151997
>>>>   patched: 98285919
>>>>   asm:    120719888
>>>>
>>>>   AMD EPYC 7R32:
>>>>   $(perl -e "print 'A' x 3"):
>>>>   stock:  282523617
>>>>   asm:    491498172
>>>>
>>>>   $(perl -e "print 'A' x 100"):
>>>>   stock:  114857172
>>>>   asm:    112082057
>>>
>>> No Reviewed by? More than one pair of eyes on non-trivial assembly is
>>> almost always a good idea.
>>>
>>> Jess
>>>
>>> _______________________________________________
>>> dev-commits-src-main at freebsd.org mailing list
>>> https://lists.freebsd.org/mailman/listinfo/dev-commits-src-main
>>> To unsubscribe, send any mail to
>>> "dev-commits-src-main-unsubscribe at freebsd.org"
>
>

-- 
Mateusz Guzik <mjguzik gmail.com>