[Intel-gfx] Maintainer-review fluff (was: Re: [PATCH 01/12] drm/i915: plumb VM into object operations)

[Daniel Vetter]

On Tue, Aug 6, 2013 at 4:50 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
> A few direct responses and my 2 cents at the end. This is all my
> humble opinion, feel free to disagree or ignore it :)

I think you make some excellent points, so thanks a lot for joining
the discussion.

> 2013/8/6 Daniel Vetter <daniel at ffwll.ch>:
>>
>> I often throw in a refactoring suggestion when people work on a
>> feature, that's right. Often it is also a crappy idea, but imo for
>> long-term maintainance a neat&tidy codebase is really important. So
>> I'll just throw them out and see what sticks with people.
>>
>
> The problem is that if you throw and it doesn't stick, then people
> feel you won't merge it. So they kinda feel they have to do it all the
> time.
>
> Another thing is that sometimes the refactoring is just plain
> bikeshedding, and that leads to demotivated workers. People write
> things on their way, but then they are forced to do it in another way,
> which is also correct, but just different, and wastes a lot of time.
> And I'm not talking specifically about Daniel's suggestions, everybody
> does this kind of bikeshedding (well, I'm sure I do). If someone gives
> a bikeshed to a patch, Daniel will see there's an unattended review
> comment and will not merge the patch at all, so basically a random
> reviewer can easily block someone else's patch. I guess we all should
> try to give less bikeshedding, including me.

Yeah, that happens. With all the stuff going on I relly can't keep
track of everything, so if it looks like the patch author and the
reviewer are still going back&forth I just wait. And like I've
explained in private once I don't like stepping in as the maintainer
when this happens since I'm not the topic expert by far, so my
assessment will be about as good as a coin-toss. Of course if the
question centers around integration issues with the overall codebase
I'll happily chime in.

I think the only way to reduce time wasted in such stuck discussions
is to admit that the best solution isn't clear and that adding a fixme
comment somewhere to look at the issue again for the next platform
(bug, regression, feature, ...) that touches the same area. Or maybe
reconsider once everything has landed and it's clear what then
end-result really looks like.

>> One prime example is Ville's watermark patches, which have been ready
>> (he only did a very few v2 versions for bikesheds) since over a month
>> ago. But stuck since no one bothered to review them.
>
> Actually I subscribed myself to review (on review board) and purposely
> waited until he was back from vacation before I would start the
> review. I also did early 0-day testing on real hardware, which is IMHO
> way much more useful than just reviewing. Something that happened many
> times for me in the past: I reviewed a patch, thought it was correct,
> then decided to boot the patch before sending the R-B email and found
> a bug.

Imo review shouldn't require you to apply the patches and test them.
Of course if it helps you to convince yourself the patch is good I'm
fine with that approach. But myself if I have doubts I prefer to check
whether a testcase/selfcheck exists to exercise that corner case (and
so will prevent this from also ever breaking again). Testing itself
should be done by the developer (or bug reporter). Hopefully the
developer patch test system that QA is now rolling out will help a lot
in that regard.

> And my 2 cents:
>
> Daniel and Jesse are based on different premises, which means they
> will basically discuss forever until they realize that.
>
> In an exaggerated view, Daniel's premises:
> - Merging patches with bugs is unacceptable
>   - Colorary: users should never have to report bugs/regressions
> - Delaying patch merging due to refactoring or review comments will
> always make it better
>
> In the same exaggerated view, Jesse's premises:
> - Actual user/developer testing is more valuable than review and refactoring
>   - Colorary: merging code with bugs is acceptable, we want the bug reports
> - Endless code churn due to review/refactoring may actually introduce
> bugs not present in the first version
>
> Please tell me if I'm wrong.

At least from my pov I think this is a very accurate description of
our different assumptions and how that shapes how we perceive these
process issues.

> From my point of view, this is all about tradeoffs and you two stand
> on different positions in these tradeoffs. Example:
> - Time time you save by not doing all the refactoring/bikeshedding can
> be spent doing bug fixing or reviewing/testing someone else's patches.
>   - But the question is: which one is more worth it? An hour
> refactoring/rebasing so the code behaves exactly like $reviewer wants,
> or an hour staring at bugzilla or reviewing/testing patches?
>   - From my point of view, it seems Daniel assumes people will always
> spend 0 time fixing bugs, that's why he requests people so much
> refactoring: the tradeoff slider is completely at one side. But that's
> kind of a vicious/virtuous cycle: the more he increases his "quality
> standards", the more we'll spend time on the refactorings, so we'll
> spend even less time on bugzilla", so Daniel will increase the
> standards even more due to even less time spent on bugzilla, and so
> on.

tbh I haven't considered that I might cause a negative feedback cycle here.

One thing that seems to work (at least for me) is when we have good
testcase. With QA's much improved regression reporting I can then
directly assign a bug to the patch auther of the offending commit.
That seems to help a lot in distributing the regression handling work.

But more tests aren't a magic solution since they also take a lot of
time to write. And in a few areas our test coverage gaps are still so
big that relying on tests only for good quality and much less on
clean&clear code which is easy to review isn't really a workable
approach. But I'd be willing to trade off more tests for less bikeshed
in review since imo the two parts are at least partial substitutes.
Thus far though writing tests seems to often come as an afterthough
and not as the first thing, so I guess this doesn't work too well with
our current team. Personally I don't like writing testcases too much,
even though it's fun to blow up the kernel ;-) And it often helps a
_lot_ with understanding the exact nature of a bug/issue, at least for
me.

Another approach could be if developers try to proactively work a bit
on issues in they're area and take active ownership, I'm much more
inclined to just merge patches in this case. Examples are how Jani
wrestles around with the backlight code or how you constantly hunt
down unclaimed register issues. Unfortunately that requires that
people follow the bugspam and m-l mail flood, which is a major time
drain :(

> One thing which we didn't discuss explicitly right now  and IMHO is
> important is how people *feel* about all this. It seems to me that the
> current amount of reworking required is making some people (e.g.,
> Jesse, Ben) demotivated and unhappy. While this is not really a
> measurable thing, I'm sure it negatively affects the rate we improve
> our code base and fix our bugs. If we bikeshed a feature to the point
> where the author gets fed up with it and just wants it to get merged,
> there's a high chance that future bugs discovered on this feature
> won't be solved that quickly due the stressful experience the author
> had with the feature. And sometimes the unavoidable "I'll just
> implement whatever review comments I get because I'm so tired about
> this series and now I just want to get it merged" attitude is a very
> nice way to introduce bugs.

Yep, people are the most important thing, technical issues can usually
be solved much easier. Maybe we need to look for different approaches
that suit people better (everyone's a bit different), like the idea
above to emphasis tests more instead of code cleanliness and
consistency. E.g. for your current pc8+ stuff I've somewhat decided
that I'm not going to drop bikesheds, but just make sure the testcase
looks good. Well throw a few ideas around while reading the patches,
but those are just ideas ... again a case I guess where you can
mistake my suggestions as requirements :(

I need to work on making such idea-throwing clearer.

Otherwise I'm running a bit low on ideas how we could change the patch
polishing for upstream to better suit people and prevent fatalistic
"this isn't really my work anymore" resingation. Ideas?

> And one more thing. IMHO this discussion should all be on how we deal
> with the people on our team, who get paid to write this code. When
> external people contribute patches to us, IMHO we should give them big
> thanks, send emails with many smileys, and hold all our spotted
> bikesheds to separate patches that we'll send later. Too high quality
> standards doesn't seem to be a good way to encourage people who don't
> dominate our code base.

I disagree. External contributions should follow the same standards as
our own code. And just because we're paid to do this doesn't mean I
won't be really happy about a tricky bugfix or a cool feature. Afaic
remember the only non-intel feature that was merged that imo didn't
live up to my standards was the initial i915 prime support from Dave.
And I've clearly stated that I won't merge the patch through my tree
and listed the reasons why I think it's not ready.

> My possible suggestions:
>
> - We already have drm-intel-next-queued as a barrier to protect
> against bugs in merged patches (it's a barrier to drm-intel-next,
> which external people should be using). Even though I do not spend
> that much time on bugzilla bugs, I do rebase on dinq/nightly every day
> and try to make sure all the regressions I spot are fixed, and I count
> this as "bug fixing time". What if we resist our OCDs and urge to
> request reworks, then merge patches to dinq more often? To compensate
> for this, if anybody reports a single problem in a patch or series
> present on dinq, it gets immediately reverted (which means dinq will
> either do lots of rebasing or contain many many reverts). And we try
> to keep drm-intel-next away from all the dinq madness. Does that sound
> maintainable?

I occasionally botch a revert/merge/rebase and since it wouldn't scale
when I ask people to cross check my tree in detail every time (or
people just assume I didn't botch it) those slip out. So I prefer if I
don't have to maintain more volatile trees.

I'm also not terribly in favour of merging stuff early and hoping for
reworks since often the attention moves immediately to the next thing.
E.g. VECS support was merged after a long delay when finally some
basic tests popped up. But then a slight change from Mika to better
exercise some seqno wrap/gpu reset corner cases showed that semaphores
don't work with VECS. QA dutifully reported this bug and Chris
analysis the gpu hang state. Ever since then this was ignored. So I
somewhat agree with Dave here, at least sometimes ...

I'm also not sure that an immediate revert rule is the right approach.
Often an issue is just minor (e.g. the modeset state checker trips
up), dropping the patch right away might be the wrong approach. Of
course if something doesn't get fixed quickly that's not great,
either.

> - Another idea I already gave a few times is to accept features more
> easily, but leave them disabled by default until all the required
> reworks are there. Daniel rejected this idea because he feels people
> won't do the reworks and will leave the feature disabled by default
> forever. My counter-argument: 99% of the features we do are somehow
> tracked by PMs, we should make sure the PMs know features are still
> disabled, and perhaps open sub-tasks on the feature tracking systems
> to document that the feature is not yet completed since it's not
> enabled by default.

I'm not sure how much that would help. If something is disabled by
default it won't getting beaten on by QA. And Jesse is right that we
just need that coverage, but to discover corner case bugs but also to
ensure a feature doesn't regress. If we merge something disabled by
default I fear it'll bitrot as quickly as an unmerged patch series.
But we leave in the delusion that it all still works. So I'm not sure
it's a good approach, but with psr we kinda have this as a real-world
experiment running. Let's see how it goes ...

> In other words: this problem is too hard, it's about tradeoffs and
> there's no perfect solution that will please everybody.

Yeah, I think your approach of clearly stating this as a tradeoff
issue cleared up things a lot for me. I think we need to actively hunt
for opportunities and new ideas. I've added a few of my own above, but
I think it's clear that there's no silver bullet.

One idea I'm pondering is whether a much more detailed breakdown of a
task/feature/... and how to get the test coverage and all the parts
merged could help. At least from my pov a big part of the frustration
seems to stem from the fact that the upstreaming process is highly
variable, and like I've said a few times I think we can do much
better. At least once we've tried this a few times and have some
experience. But again this is not for free but involves quite some
work. And I guess I need to be highly involved or even do large parts
of that break-down to make sure nothing gets missed, and I kinda don't
want to sign up for that work ;-)

> My just 2 cents, I hope to not have offended anybody :(

Not at all, and I think your input has been very valuable to the discussion.

Thanks a lot,
Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch