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:

quoted

On Feb 8, 2009, at 12:25 AM, J. Bruce Fields wrote:

quoted

On Sat, Feb 07, 2009 at 06:56:03PM +0200, Ilpo Järvinen wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

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 linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html