Date: Wed, 28 Jan 2015 09:02:10 -0800 From: Rui Paulo <rpaulo@me.com> To: Ian Lepore <ian@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Ruslan Bukin <br@FreeBSD.org>, src-committers@freebsd.org Subject: Re: svn commit: r277835 - in head: lib/libpmc sys/arm/arm sys/arm/include sys/arm/ti sys/conf sys/dev/hwpmc sys/sys Message-ID: <8F8272CF-DD73-41EF-8096-E8C79AD903CB@me.com> In-Reply-To: <1422462903.15718.60.camel@freebsd.org> References: <201501281608.t0SG88gs009253@svn.freebsd.org> <1422462903.15718.60.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Jan 28, 2015, at 08:35, Ian Lepore <ian@freebsd.org> wrote: >=20 > 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 >>=20 >> Log: >> Add ARMv7 performance monitoring counters. >>=20 >> Differential Revision: https://reviews.freebsd.org/D1687 >> Reviewed by: rpaulo >> Sponsored by: DARPA, AFRL >>=20 >> 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 >=20 > 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). >=20 > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8F8272CF-DD73-41EF-8096-E8C79AD903CB>