Date: Fri, 30 Nov 2018 11:12:20 -0800 From: John Baldwin <jhb@FreeBSD.org> To: Philip Paeps <philip@freebsd.org> Cc: Andrew Rybchenko <arybchik@FreeBSD.org>, gnn@FreeBSD.org, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r341327 - head/sys/dev/sfxge Message-ID: <fe51e92f-6214-589d-e9dc-1e3568265fc7@FreeBSD.org> In-Reply-To: <0A8EBFBB-37DA-439F-A8D3-099A13128AD9@freebsd.org> References: <201811300711.wAU7B5R6084752@repo.freebsd.org> <d60f54ae-a1a6-bad5-73f0-c898d0579089@FreeBSD.org> <7545b110-4930-d49f-f99f-7ec17db7e7a7@oktetlabs.ru> <be19ca93-6703-e329-b070-f338ec264f0b@FreeBSD.org> <0A8EBFBB-37DA-439F-A8D3-099A13128AD9@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11/30/18 11:01 AM, Philip Paeps wrote: > On 2018-11-30 19:36:27 (+0100), John Baldwin wrote: >> On 11/30/18 10:15 AM, Andrew Rybchenko wrote: >>> On 30.11.2018 20:30, John Baldwin wrote: >>>> On 11/29/18 11:11 PM, Andrew Rybchenko wrote: >>>>> Author: arybchik >>>>> Date: Fri Nov 30 07:11:05 2018 >>>>> New Revision: 341327 >>>>> URL: https://svnweb.freebsd.org/changeset/base/341327 >>>>> >>>>> Log: >>>>> sfxge(4): rollback last seen VLAN TCI if Tx packet is dropped >>>>> >>>>> Early processing of a packet on transmit may change last seen >>>>> VLAN TCI in the queue context. If such a packet is eventually >>>>> dropped, last seen VLAN TCI must be set to its previous value. >>>>> >>>>> Submitted by: Ivan Malov <Ivan.Malov at oktetlabs.ru> >>>>> Sponsored by: Solarflare Communications, Inc. >>>>> MFC after: 1 week >>>>> Differential Revision: https://reviews.freebsd.org/D18288 >>>> >>>> Just as a general comment. There's no point in creating a review in >>>> phabricator if you aren't going to get any actual review feedback >>>> via the tool. That just adds noise. (I've spotchecked a few of the >>>> recent sfxge commits and they all seem to create a review that then >>>> gets committed a few hours later without any feedback, etc.) >>> >>> All these changesets is the result of development in Solarflare. All >>> these changesets were reviewed internally and in fact many have later >>> fixes which are simply squashed in. >>> >>> We have discussed it with George (gnn@) some time ago and he asked to >>> submit reviews anyway and wait at least a day or two before commit. >>> Yes in this particular case these 2 hundreds of patches is the result >>> of 2 years of development. So, I'd waited some time and started to >>> commit in blocks. >>> >>> This time I've not included np@ and bz@ in reviewers since I've not >>> got reviewed before and it would be too much spam. >>> >>> We have discussed it with Philip (philip@) shortly. As I understand >>> he has no time now to review it. >>> >>> Basically I'm ready to follow any sensible policy. I don't think it >>> makes to wait forever. If there are any volunteers I'll be happy to >>> include more people in reviewers. >> >> I don't think you have to wait forever, and if the changes were >> reviewed internally that counts for review, I just don't want to add >> noise and clutter to phabricator and commit logs. > > I think it makes sense to have Phabricator reviews for the > FreeBSD-specific parts of sfxge(4). > > The internal review at Solarflare is definitely good enough to commit > without waiting for review -- certainly on the parts of the driver that > are generated from the common source -- but there is some value to > having the FreeBSD-specific bits sit in Phabricator for a few days. > > The storm of commits in the last week is exceptional because it > represents two years of changes. With hindsight, just bulk-committing > those to Subversion would have been a better idea. > > In the future though, and when in the steady state of commits trickling > in rather than flooding in, I still appreciate the reviews in > Phabricator. Particularly for the FreeBSD-specific parts of the driver. That's fine as long as you are going to do actual reviews. The only point I raised was about putting things in phab but not really using phab. Let's use it when it makes sense, but let's avoid things that look like dummy reviews to satisfy a checkbox. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?fe51e92f-6214-589d-e9dc-1e3568265fc7>