Message-Id: <C1261653-18B9-4E6F-A2A1-A1BEC957324B@oracle.com>
Date:	Tue, 10 Feb 2009 12:12:32 -0500
From:	Chuck Lever <chuck.lever@...cle.com>
To:	Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
Cc:	"J. Bruce Fields" <bfields@...ldses.org>,
	Trond Myklebust <Trond.Myklebust@...app.com>,
	David Miller <davem@...emloft.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Trond Myklebust <trond@...app.com>,
	Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

Hi Ilpo-

On Feb 10, 2009, at 8:12 AM, Ilpo Järvinen wrote:
> On Mon, 9 Feb 2009, Chuck Lever wrote:
>
>> On Feb 8, 2009, at 12:25 AM, J. Bruce Fields wrote:
>>> On Sat, Feb 07, 2009 at 06:56:03PM +0200, Ilpo Järvinen wrote:
>>>> I personally think that people were honestly just mislead to  
>>>> think that
>>>> diff-funcs output is part of the patch
>>>
>>> Yes, exactly, in my case, that's all it was--the "diff" I thought  
>>> I saw
>>> in my 2-second skim looked odd, and I figured a comment might have
>>> gotten dropped off in the process of replying and adding cc's-- 
>>> hence the
>>> original question.
>>
>> I apologize if my request for "English" seemed Anglo-centric.  What  
>> I intended
>> was "non-machine-generated" and expressed in the natural language  
>> that is
>> usual for Linux kernel patch descriptions, which appears to be  
>> English.
>
> ...I still argue that if title gives the same information as that
> sentence would, the sentence is optional.

I'm OK with that.  In the original patch, I think both were somewhat  
difficult to parse.  My request is to try harder next time to make  
them clear.  It sounds like we agree here.

> Some patches don't even have
> patch description beyond title and there's very little problem with  
> that.
> As I admitted earlier the title wasn't as good as it should have  
> (and I
> even sent a followup to please all including even the in body sentence
> plus the fixed title). Not that I find the title really a problem to  
> sane
> people who understand that code-duplication is bad and getting rid  
> of it
> is what one should do in general, as long as it's possible without  
> making
> a mess out of it.

Like all things software, absolute statements are usually always  
proven incorrect (eg "goto is spawn of the devil and should never be  
used").  Code duplication is _usually_ bad.  Sometimes there is a  
reason for it.  We want to avoid a knee-jerk response of eliminating  
it wherever we find it, without another thought.

>> I agree with you that the diff output in the patch description was  
>> confusing,
>> and yes, after Dave's first comment I understood what the  
>> description meant.
>> I believe patch authors owe it to maintainers, who have dozens of  
>> patches to
>> review a day, to keep it as simple and easy as possible.  I have  
>> tried to work
>> with individual maintainers where possible to improve the  
>> legibility of my
>> patches, though at times I have fallen short.
>>
>> Although interesting, the tool output doesn't seem necessary to  
>> justify the
>> proposed change.
>
> I don't understand why a tool output cannot "say" the same as some  
> English
> sentence...

It can.  But it's interesting to observe that the point of your  
proposed change was to reduce lines of code and code obscurity.  Your  
description, however, was much longer and more obscure than it needed  
to be.  :-)

I was too harsh to ask for no machine-generated patch descriptions.   
The point is, is the description meaningful and concise?

> And if a proof that the code blocks that were merged were
> infact identical (instead of having some hard to spot difference)  
> doesn't
> seem like useful information to reviewer I don't really know what to  
> say.

Verification of your patch is different than justification.  But  
that's a minor point.

>> In addition to crafting a legible description, my suggestions were  
>> about
>> cc'ing the proper mailing list, and routing this patch directly to  
>> the sunrpc
>> kernel maintainers in the first place, all of which are listed  
>> within easy
>> reach in MAINTAINERS.
>
> Please note that I couldn't guess that I'd have to look for another  
> entry
> after first SUNRPC entry in the MAINTAINERS. So asking outsider to  
> know
> who is who in sunrpc realm is not very likely to be a practical
> requirement either. And even to add into confusion the latest commits
> in that particular directory had the last S-o-B line as Bruce's. How  
> could
> I have guessed? Forgetting to lookup the address from MAINTAINERS  
> into the
> first mail is something which just happened and I'm certainly guilty  
> to
> that :-).

Yes, we should fix the SUNRPC entries in MAINTAINERS.

> ...I've had enough down turning experiences with random dev lists  
> listed
> in the MAINTAINERS (mostly spammy subsriber only ones, nearly infinite
> response time, etc things) that I mostly ignore them nowadays though  
> this
> time it would have been vger list so no subscription required madness.
> Instead I either select lkml or netdev, this time netdev because of
> the obvious presence under net/ hierarcy.

My experience is usually the opposite.  I post a patch directly to a  
maintainer and get a "please use such-and-such list" response.  The  
exception is usually very specific device drivers.

But you're right, it would be nice to have some consistency here  
across the kernel.

>> But I wonder how we ended up with these two copies of the same  
>> logic.  It may
>> be that at some point in the past we had more different logic in  
>> the two
>> write_space functions, but I suppose we can split it out again if  
>> we ever need
>> to.  Thinking aloud, at some point we may have lost a little  
>> history here, or
>> we may have perhaps lost any implicit operational dependencies on  
>> other parts
>> of xprtsock.c (like the TCP state logic) that are not expressed in  
>> the source
>> code or comments.
>
> Well, this kind of thinking is beyond me and requires more knowledge  
> about
> internals of the code.

My concern with immediately accepting a patch that appears to be  
solely machine-generated is exactly this.  The submission didn't leave  
me with the impression that there was a human who had taken the time  
to understand history (git log/annotate) and how the code works, what  
the source code was attempting to express, or whether the resulting  
patch was even tested.  Usually you can glean a bit of this from a  
lengthier patch description.

Yes, yours was a simple and proper change in the end, but still.   
Given the long "dialog" just had on lkml about compiler (machine- 
generated) optimizations, I think we would all be just a little  
skeptical (or perhaps careful) about such automated transformations.

> IMHO, this kind of speculation changes nothing
> really, and besides the functions _were_ different to begin with as  
> was
> proven in the patch description :-). ...Would they have been fully
> identical you would have seen a bit different patch ;-).
>
> If you think there's something wrong, please fix, you're free to do  
> so,
> I don't understand enough. But we don't keep all those #if 0 blocks
> around either just because somebody thinks that something will get
> implemented one day (not that you directly made such assertion,  
> probably
> saying mildy the opposite instead). Ironically, you would have been
> perfectly happy with the particular functions unless a cleanup patch
> against them gets made... :-)

You are correct, the analysis doesn't change anything, but the  
analysis is worth at least noting during review.  It helps us  
understand where we have to make the code more clear.

The question is whether the code itself documents dependencies on  
other parts of the code, whether such dependencies are only documented  
by comments, or whether it is entirely implicit and up to the folks  
who work on it to understand and remember. This is usually only an  
issue when someone who has never worked on the code submits a patch.

The goal of open source code is to facilitate other folks modifying  
our code.  For open source especially it is a larger issue than simple  
code maintenance.  It is not enough for open source code simply to  
work.  If people outside our team (or outside the larger Linux  
community) can't easily understand the important features of our open  
source code, then we've still failed, even if the legal barriers have  
been lifted by the GPL.

Thus I'm especially sensitive to areas where our source code does not  
express itself well to folks who are not familiar.  We want to invite  
wider review, and that's simply not possible if nobody understands  
this code base but ourselves.

>> The newly shared code appears to be socket-specific, so it does not
>> belong in the shared function we already have, xprt_write_space(),  
>> which
>> is meant to be transport-generic.
>
> IMHO, this is the most useful response so far! :-) Usually the hardest
> obstacles I face with non-familiar code is its structure and naming.


--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists