svn commit: r341327 - head/sys/dev/sfxge

[Andrew Rybchenko]

On 30.11.2018 22:01, 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).

Yes, I think it would be very-very useful.

> 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.

Makes sense. Two FreeBSD-specific are waiting right now :)

> 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.

I think it is still useful for the project to have more granular changes.
At least it easier to understand in the future motivation of these 
changes etc.
Not a strong opinion as well, but I'd prefer to keep these changes as is
(except squashing fixes as I do to avoid known breakages in the middle).

> 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.

I can't promise, but I'll try to avoid so huge delays in the future.
I'm trying to submit when I'm more confident that changes are
stable and probability of problems is less.

Andrew.