svn commit: r277835 - in head: lib/libpmc sys/arm/arm sys/arm/include sys/arm/ti sys/conf sys/dev/hwpmc sys/sys

[Rui Paulo]

On Jan 28, 2015, at 08:35, Ian Lepore <ian at freebsd.org> wrote:
> 
> On Wed, 2015-01-28 at 16:08 +0000, Ruslan Bukin wrote:
>> Author: br
>> Date: Wed Jan 28 16:08:07 2015
>> New Revision: 277835
>> URL: https://svnweb.freebsd.org/changeset/base/277835
>> 
>> Log:
>>  Add ARMv7 performance monitoring counters.
>> 
>>  Differential Revision:	https://reviews.freebsd.org/D1687
>>  Reviewed by:	rpaulo
>>  Sponsored by:	DARPA, AFRL
>> 
>> Added:
>>  head/sys/dev/hwpmc/hwpmc_armv7.c   (contents, props changed)
>>  head/sys/dev/hwpmc/hwpmc_armv7.h   (contents, props changed)
>> Modified:
>>  head/lib/libpmc/libpmc.c
>>  head/sys/arm/arm/intr.c
>>  head/sys/arm/include/pmc_mdep.h
>>  head/sys/arm/ti/files.ti
>>  head/sys/conf/files.arm
>>  head/sys/dev/hwpmc/hwpmc_arm.c
>>  head/sys/dev/hwpmc/pmc_events.h
>>  head/sys/sys/pmc.h
> 
> This was in phabricator for review for 27 hours before it got committed,
> that's not enough time to allow people to actually review it.  That
> would be not enough time for even a simple change, let alone over a
> thousand of lines of code.  It certainly wasn't reviewed by those
> actively working on arm pmc stuff recently (gnn and to a lesser degree,
> me).
> 
> Just from a quick glance at the part that wasn't truncated, I notice all
> the inline asm stuff is wrong -- it duplicates what's already available
> in cpu-v6.h.

I do agree that reviewers weren't given enough time and phabricator seems to make that worse by saying the code is "ready to land" (we're trying to change that).

In my defense, I only reviewed this because I implemented the original XScale PMC.  I also didn't know who else was working on ARM PMC, so I couldn't warn Ruslan.   Andrew was the one that added the ARM group to the review, so maybe he wasn't aware of it either.

--
Rui Paulo