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

[Ben Widawsky]

On Fri, Jul 26, 2013 at 11:51:00AM +0200, Daniel Vetter wrote:
> HI all,
> 
> So Ben&I had a bit a private discussion and one thing I've explained a bit
> more in detail is what kind of review I'm doing as maintainer. I've
> figured this is generally useful. We've also discussed a bit that for
> developers without their own lab it would be nice if QA could test random
> branches on their set of machines. But imo that'll take quite a while,
> there's lots of other stuff to improve in QA land first. Anyway, here's
> it:
> 
> Now an explanation for why this freaked me out, which is essentially an
> explanation of what I do when I do maintainer reviews:
> 
> Probably the most important question I ask myself when reading a patch is
> "if a regression would bisect to this, and the bisect is the only useful
> piece of evidence, would I stand a chance to understand it?".  Your patch
> is big, has the appearance of doing a few unrelated things and could very
> well hide a bug which would take me an awful lot of time to spot. So imo
> the answer for your patch is a clear "no".
> 
> I've merged a few such patches in the past  where I've had a similar hunch
> and regretted it almost always. I've also sometimes split-up the patch
> while applying, but that approach doesn't scale any more with our rather
> big team.

You should never do this, IMO. If you require the patches to be split in
your tree, the developer should do it. See below for reasons I think
this sucks.

> 
> The second thing I try to figure out is whether the patch author is indeed
> the local expert on the topic at hand now. With our team size and patch
> flow I don't stand a chance if I try to understand everything to the last
> detail. Instead I try to assess this through the proxy of convincing
> myself the the patch submitter understands stuff much better than I do. I
> tend to check that by asking random questions, proposing alternative
> approaches and also by rating code/patch clarity. The obj_set_color
> double-loop very much gave me the impression that you didn't have a clear
> idea about how exactly this should work, so that  hunk trigger this
> maintainer hunch.
> 
> I admit that this is all rather fluffy and very much an inexact science,
> but it's the only tools I have as a maintainer. The alternative of doing
> shit myself or checking everything myself in-depth just doesnt scale.
> 
> Cheers, Daniel
> 
> 
> On Mon, Jul 22, 2013 at 4:08 AM, Ben Widawsky <ben at bwidawsk.net> wrote:

I think the subthread Jesse started had a bunch of good points, but
concisely I see 3 problems with our current process (and these were
addressed in my original mail, but I guess you didn't want to air my
dirty laundry :p):

1. Delay hurts QA. Balking on patches because they're hard to review
limits QA on that patch, and reduces QA time on the fixed up patches. I
agree this is something which is fixable within QA, but it doesn't exist
at present.

2. We don't have a way to bound review/merge. I tried to do this on this
series. After your initial review, I gave a list of things I was going
to fix, and asked you for an ack that if I fixed those, you would merge.
IMO, you didn't stick to this agreement, and came back with rework
requests on a patch I had already submitted. I don't know how to fix
this one because I think you should be entitled to change your mind.

A caveat to this: I did make some mistakes on rebase that needed
addressing. ie. the ends justified the means.

3a. Reworking code introduces bugs. I feel I am more guilty here than
most, but, consider even in the best case of those new bugs being
caught in review. In such a case, you've now introduced at least 2 extra
revs, and 2 extra lag cycles waiting for review. That assumes further
work doesn't spiral into more requested fixups, or more bugs. In the
less ideal case, you've simply introduced a new bug in addition to the
delay.

3b. Patch splitting is art not science.

There is a really delicate balance between splitting patches because
it's logically a functional split vs. splitting things up to make things
easier to chew on. Now in my case specifically, I think overall the
series has improved, and I found some crud that got squashed in which
shouldn't have been there. I also believe a lot of the splitting really
doesn't make much sense other than for review purposes and sometimes
that is okay.

In my case, I had a huge patch, but a lot of that patch was actually a
sed job of "s/obj/obj,vm/."  You came back with, "you're doing a bunch
of extra lookups." That was exactly the point of the patch; the extra
lookups should have made the review simpler, and could be cleaned up
later.

My point is: A larger quantity of small patches is not always easier to
review than a small quantity of large patches. Large patch series review
often requires the reviewer to keep a lot of context as they review.

*4. The result of all this is I think a lot of the time we (the
developers) end up writing your patch for you. While I respect your
opinion very highly, and I think more often than not that your way is
better, it's just inefficient.


I'll wrap this all up with, I don't envy you. On a bunch of emails, I've
seen you be apologetic for putting developers in between a rock, and a
hard place (you, and program management). I recognize you have the same
dilemma with Dave/Linus, and the rest of us developers. I think the
overall strategy should be to improve QA, but then you have to take the
leap of limiting your requests for reworks, and accepting QAs stamp of
approval.

-- 
Ben Widawsky, Intel Open Source Technology Center