Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5428 bytes --]

On Fri, 6 Feb 2009, Chuck Lever wrote:

> On Feb 6, 2009, at Feb 6, 2009, 5:07 PM, David Miller wrote:
> >From: "J. Bruce Fields" <bfields@fieldses.org>
> >Date: Fri, 6 Feb 2009 16:20:43 -0500
> >
> > >On Fri, Feb 06, 2009 at 10:53:32PM +0200, Ilpo Järvinen wrote:
> > > >Grr, forgot to cc sunrpc maintainer... Added. Just ask if somebody wants
> > > >a complete resend.
> > >
> > >You probably wanted Trond, actually.  (And did this patch have a
> > >changelog?)
> >
> >Open your eyes :-)  That first "diff" you see is part of the
> >commit message not the patch itself.
> 
> There needs to be an explanation (in English not in source code)

Hmm, so you didn't heed Dave's warning... :-D

I think your "English" is too strict requirement. There's patch title 
which tells what is found, in plain English. And _the explanation_ is 
there too, but using two tools (and their names are certainly 
self-explanary): diff-funcs and codiff. The output of the tools shows why 
this is useful (and every developer capable of reviewing patches in the 
first place should be able to understand the output format of the tools in 
question). If you don't care the output but dismiss that as "source code" 
there's nothing I can do to help you _to review_ the patch by writing 
paragraphs of text about the change. But just to make it even easier for 
you I've added one sentence more, in English before codiff ;-).

> of why this change is needed, even if you are simply refactoring code or 
> correcting a comment.

It's there. Read again :-).

> A short description that starts with "SUNRPC: " is also recommended, and
> please copy linux-nfs@vger.kernel.org when submitting RPC-related patches.

Well, I've changed the title now to please all. :-)

--
[PATCH] SUNRPC: some common code found in xprtsock.c; call to that

$ diff-funcs xs_udp_write_space net/sunrpc/xprtsock.c
net/sunrpc/xprtsock.c xs_tcp_write_space
 --- net/sunrpc/xprtsock.c:xs_udp_write_space()
 +++ net/sunrpc/xprtsock.c:xs_tcp_write_space()
@@ -1,4 +1,4 @@
- * xs_udp_write_space - callback invoked when socket buffer space
+ * xs_tcp_write_space - callback invoked when socket buffer space
  *                             becomes available
  * @sk: socket whose state has changed
  *
@@ -7,12 +7,12 @@
  * progress, otherwise we'll waste resources thrashing kernel_sendmsg
  * with a bunch of small requests.
  */
-static void xs_udp_write_space(struct sock *sk)
+static void xs_tcp_write_space(struct sock *sk)
 {
 	read_lock(&sk->sk_callback_lock);

-	/* from net/core/sock.c:sock_def_write_space */
-	if (sock_writeable(sk)) {
+	/* from net/core/stream.c:sk_stream_write_space */
+	if (sk_stream_wspace(sk) >= sk_stream_min_wspace(sk)) {
 		struct socket *sock;
 		struct rpc_xprt *xprt;

Besides less copy-paste, some space savings (measured allmodconfig 
x86_64) after the change:

$ codiff net/sunrpc/xprtsock.o net/sunrpc/xprtsock.o.new
net/sunrpc/xprtsock.c:
  xs_tcp_write_space | -163
  xs_udp_write_space | -163
 2 functions changed, 326 bytes removed

net/sunrpc/xprtsock.c:
  xs_write_space | +179
 1 function changed, 179 bytes added

net/sunrpc/xprtsock.o.new:
 3 functions changed, 179 bytes added, 326 bytes removed, diff: -147

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen-pxSi+dnQzZMxHbG02/KK1g@public.gmane.org>
---
 net/sunrpc/xprtsock.c |   53 +++++++++++++++++++-----------------------------
 1 files changed, 21 insertions(+), 32 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 5cbb404..b49e434 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1215,6 +1215,23 @@ out:
 	read_unlock(&sk->sk_callback_lock);
 }
 
+static void xs_write_space(struct sock *sk)
+{
+	struct socket *sock;
+	struct rpc_xprt *xprt;
+
+	if (unlikely(!(sock = sk->sk_socket)))
+		return;
+	clear_bit(SOCK_NOSPACE, &sock->flags);
+
+	if (unlikely(!(xprt = xprt_from_sock(sk))))
+		return;
+	if (test_and_clear_bit(SOCK_ASYNC_NOSPACE, &sock->flags) == 0)
+		return;
+
+	xprt_write_space(xprt);
+}
+
 /**
  * xs_udp_write_space - callback invoked when socket buffer space
  *                             becomes available
@@ -1230,23 +1247,9 @@ static void xs_udp_write_space(struct sock *sk)
 	read_lock(&sk->sk_callback_lock);
 
 	/* from net/core/sock.c:sock_def_write_space */
-	if (sock_writeable(sk)) {
-		struct socket *sock;
-		struct rpc_xprt *xprt;
-
-		if (unlikely(!(sock = sk->sk_socket)))
-			goto out;
-		clear_bit(SOCK_NOSPACE, &sock->flags);
-
-		if (unlikely(!(xprt = xprt_from_sock(sk))))
-			goto out;
-		if (test_and_clear_bit(SOCK_ASYNC_NOSPACE, &sock->flags) == 0)
-			goto out;
-
-		xprt_write_space(xprt);
-	}
+	if (sock_writeable(sk))
+		xs_write_space(sk);
 
- out:
 	read_unlock(&sk->sk_callback_lock);
 }
 
@@ -1265,23 +1268,9 @@ static void xs_tcp_write_space(struct sock *sk)
 	read_lock(&sk->sk_callback_lock);
 
 	/* from net/core/stream.c:sk_stream_write_space */
-	if (sk_stream_wspace(sk) >= sk_stream_min_wspace(sk)) {
-		struct socket *sock;
-		struct rpc_xprt *xprt;
-
-		if (unlikely(!(sock = sk->sk_socket)))
-			goto out;
-		clear_bit(SOCK_NOSPACE, &sock->flags);
+	if (sk_stream_wspace(sk) >= sk_stream_min_wspace(sk))
+		xs_write_space(sk);
 
-		if (unlikely(!(xprt = xprt_from_sock(sk))))
-			goto out;
-		if (test_and_clear_bit(SOCK_ASYNC_NOSPACE, &sock->flags) == 0)
-			goto out;
-
-		xprt_write_space(xprt);
-	}
-
- out:
 	read_unlock(&sk->sk_callback_lock);
 }
 
-- 
1.5.6.5

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[David Miller]

=46rom: "Ilpo J=E4rvinen" <ilpo.jarvinen-pxSi+dnQzZMxHbG02/KK1g@public.gmane.org>
Date: Sat, 7 Feb 2009 00:58:44 +0200 (EET)

> I think your "English" is too strict requirement.

I absolutely and whole-heartedly agree.  This set of
requirements is beyond rediculious and will do nothing
but deter people from contributing.

So unless that's what the goal is...

Anyways, I'll be applying Ilpo's great changes to the
net-next-2.6 GIT tree, without putting him through the
ringer like this.

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[Trond Myklebust]

On Fri, 2009-02-06 at 19:22 -0800, David Miller wrote:
> From: "Ilpo J=C3=A4rvinen" <ilpo.jarvinen-pxSi+dnQzZMxHbG02/KK1g@public.gmane.org>
> Date: Sat, 7 Feb 2009 00:58:44 +0200 (EET)
>=20
> > I think your "English" is too strict requirement.
>=20
> I absolutely and whole-heartedly agree.  This set of
> requirements is beyond rediculious and will do nothing
> but deter people from contributing.
>=20
> So unless that's what the goal is...
>=20
> Anyways, I'll be applying Ilpo's great changes to the
> net-next-2.6 GIT tree, without putting him through the
> ringer like this.

No. I find it quite unacceptable for the networking group to circumvent
the standard Linux review process in this manner. If you plan to push a
patch into the sunrpc code against the wishes of the community and
people that maintain that code then I'll be petitioning Linus to revert
it on general principle as soon as it hits his tree.

Like it or not, the changelog _is_ a part of the patch that is subject
to review and comment, and while I like the rest of the patch, I will
support Chuck and Bruce's comments that putting a bunch of diffstat
output into the changelog is not a substitute for a simple sentence. Al=
l
it takes is

"
Save 147 lines by moving the common code from xs_udp_write_space() and
xs_tcp_write_space() into a separate helper.

S-o-b: Ilpo....
"

and everyone will be happy. So please stop making a storm in a teacup
out of this...

Trond
--=20
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3253 bytes --]

On Sat, 7 Feb 2009, Trond Myklebust wrote:

> On Fri, 2009-02-06 at 19:22 -0800, David Miller wrote:
> > From: "Ilpo Järvinen" <ilpo.jarvinen-pxSi+dnQzZMxHbG02/KK1g@public.gmane.org>
> > Date: Sat, 7 Feb 2009 00:58:44 +0200 (EET)
> > 
> > > I think your "English" is too strict requirement.
> > 
> > I absolutely and whole-heartedly agree.  This set of
> > requirements is beyond rediculious and will do nothing
> > but deter people from contributing.
> > 
> > So unless that's what the goal is...
> > 
> > Anyways, I'll be applying Ilpo's great changes to the
> > net-next-2.6 GIT tree, without putting him through the
> > ringer like this.
> 
> No. I find it quite unacceptable for the networking group to circumvent
> the standard Linux review process in this manner. If you plan to push a
> patch into the sunrpc code against the wishes of the community and
> people that maintain that code then I'll be petitioning Linus to revert
> it on general principle as soon as it hits his tree.

:-)

> Like it or not, the changelog _is_ a part of the patch that is subject
> to review and comment, and while I like the rest of the patch, I will
> support Chuck and Bruce's comments that putting a bunch of diffstat
> output into the changelog is not a substitute for a simple sentence.

But this is likely based on a false assumption (see my thought about that
below), and I didn't have diffstat afaik :-).

> All it takes is
> 
> "
> Save 147 lines by moving the common code from xs_udp_write_space() and
> xs_tcp_write_space() into a separate helper.
> 
> S-o-b: Ilpo....
> "

Can you please tell if you're still under impression that there wasn't
any changelog? What about those other guys who raised their concerns?
(I can certainly understand why this impression might develop but it's a 
problem in the canonial patch format, not in my actual changelog, I even 
need to circumvent ^--- by adding a space the to prevent tools for making 
a mistake). It will make a lot more sense after getting applied.
...Because there certainly was a changelog, and I didn't have any 
diffstat output there in the changelog?!? The output that *is* in the 
changelog is supposed to *help* the reviewer to see what will be still 
necessary to keep in the separate functions, but I guess people missed 
that because the changelog looks too similar to what you will categorize 
as patch (diff-funcs uses output which is familiar to all who who deal 
with patches, and that's for purpose!).

> and everyone will be happy. So please stop making a storm in a teacup
> out of this...

I even tried to make sunrpc people happy by sending the followup with some 
plain English in changelog too (though it seems very obvious as you could 
spell it out above without a question besides the s/bytes/lines/ :-)) and
even more clear title. I agree that I should have put, "; add helper" or
something like that to the title which is exactly what I did in the 
followup.

I personally think that people were honestly just mislead to think that 
diff-funcs output is part of the patch -> thus it raises complaining 
about missing changelog (which is not true).

But whatever :-). Feel free to revert the first one and put the followup 
which has something to satisfy all :-).

-- 
 i.

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[Trond Myklebust]

On Sat, 2009-02-07 at 18:56 +0200, Ilpo J=C3=A4rvinen wrote:
> Can you please tell if you're still under impression that there wasn'=
t
> any changelog? What about those other guys who raised their concerns?
> (I can certainly understand why this impression might develop but it'=
s a=20
> problem in the canonial patch format, not in my actual changelog, I e=
ven=20
> need to circumvent ^--- by adding a space the to prevent tools for ma=
king=20
> a mistake). It will make a lot more sense after getting applied.
> ...Because there certainly was a changelog, and I didn't have any=20
> diffstat output there in the changelog?!? The output that *is* in the=
=20
> changelog is supposed to *help* the reviewer to see what will be stil=
l=20
> necessary to keep in the separate functions, but I guess people misse=
d=20
> that because the changelog looks too similar to what you will categor=
ize=20
> as patch (diff-funcs uses output which is familiar to all who who dea=
l=20
> with patches, and that's for purpose!).

If Chuck and Bruce are happy with it, then there is no problem, however
I have yet to see an acked-by from either one of them. My point is that
the process for accepting patches into the kernel involves ensuring tha=
t
there is consensus. It does not allow for one group to ride roughshod
over the objections of another.

   Trond
--=20
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[David Miller]

From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Sat, 07 Feb 2009 12:22:01 -0500

> My point is that the process for accepting patches into the kernel
> involves ensuring that there is consensus.

You haven't been hacking the kernel very long if you actually
believe this.

When people are being rediculious or unreasonable, subsystem
or higher maintainers make exective decisions all the time.

"revert based upon principle" ?!?!  If you think Linus will do that
for a correct change, you're kidding yourself.

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[J. Bruce Fields]

On Sat, Feb 07, 2009 at 06:56:03PM +0200, Ilpo J=C3=A4rvinen wrote:
> I personally think that people were honestly just mislead to think th=
at
> 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 th=
e
original question.

Anyway, the patch looks obviously fine to me, and the diff-func and
codiff output are helpful.  (Where's diff-func from?)

In similar cases in the future, if you want to cater to the more tired
and/or mentally challenged among us (which may just be me), some clue
that visually distinguishes the quoted diff output in the comment might
help; e.g., maybe indent it like the below.

(But that's intended as a suggestion, not a requirement.)

--b.

Subject: Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

diff-func output suggests these functions are very similar:

	$ diff-funcs xs_udp_write_space net/sunrpc/xprtsock.c
	 ...(etc)

and codiff shows the space savings:

	$ codiff net/sunrpc/xprtsock.o net/sunrpc/xprtsock.o.new
	...(etc)

Signed-off-by: Ilpo J=C3=A4rvinen <ilpo.jarvinen-pxSi+dnQzZMxHbG02/KK1g@public.gmane.org>

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[Chuck Lever]

On Feb 8, 2009, at 12:25 AM, J. Bruce Fields wrote:
> On Sat, Feb 07, 2009 at 06:56:03PM +0200, Ilpo J=E4rvinen wrote:
>> I personally think that people were honestly just mislead to think =20
>> that
>> diff-funcs output is part of the patch
>
> Yes, exactly, in my case, that's all it was--the "diff" I thought I =20
> 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 =
=20
> the
> original question.

I apologize if my request for "English" seemed Anglo-centric.  What I =20
intended was "non-machine-generated" and expressed in the natural =20
language that is usual for Linux kernel patch descriptions, which =20
appears to be English.

Not being a maintainer myself, I can't enforce any requirement here.  =20
I was simply suggesting ways to help the patch make its way =20
expediently to reviewers and people who can get it into the kernel.

I agree with you that the diff output in the patch description was =20
confusing, and yes, after Dave's first comment I understood what the =20
description meant.  I believe patch authors owe it to maintainers, who =
=20
have dozens of patches to review a day, to keep it as simple and easy =20
as possible.  I have tried to work with individual maintainers where =20
possible to improve the legibility of my patches, though at times I =20
have fallen short.

Although interesting, the tool output doesn't seem necessary to =20
justify the proposed change.  A single sentence, as Trond suggested, =20
would have been far more concise.  And Bruce's recommendations for =20
setting off machine-generated content in a patch description are cogent=
=2E

In addition to crafting a legible description, my suggestions were =20
about cc'ing the proper mailing list, and routing this patch directly =20
to the sunrpc kernel maintainers in the first place, all of which are =20
listed within easy reach in MAINTAINERS.  A handful of us on linux-nfs =
=20
happen to read netdev, but not all of us.  We do try to stick to a =20
fair review process here, which includes archiving the patch and =20
reviewer comments via linux-nfs-u79uwXL29TaiAVqoAR/hOA@public.gmane.org

J. Bruce Fields continues:
> Anyway, the patch looks obviously fine to me[.]

I don't have an immediate problem with the patch's content either, as =20
long as it has been tested and does not introduce any new sparse or =20
compiler warnings.  I'm all for the intelligent elimination of gotos =20
and duplicated code.

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

The newly shared code appears to be socket-specific, so it does not =20
belong in the shared function we already have, xprt_write_space(), =20
which is meant to be transport-generic.

Acked-by: Chuck Lever <chuck.lever@oracle.com>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5017 bytes --]

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

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

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

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

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

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


-- 
 i.

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[Ilpo Järvinen]

On Sun, 8 Feb 2009, J. Bruce Fields wrote:

> (Where's diff-func from?)

My own tool collection. I can make it available if you want to.

> In similar cases in the future, if you want to cater to the more tired
> and/or mentally challenged among us (which may just be me), some clue
> that visually distinguishes the quoted diff output in the comment might
> help; e.g., maybe indent it like the below.
> 
> (But that's intended as a suggestion, not a requirement.)

Yeah, a good idea... I'll sed some spaces in front of them in the future 
to make the difference more obvious. The expected outcome though is that 
somebody probably then hits reply button too soon claiming that the patch 
is broken :-).


-- 
 i.

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[Chuck Lever]

Hi Ilpo-

On Feb 10, 2009, at 8:12 AM, Ilpo J=E4rvinen 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=E4rvinen wrote:
>>>> I personally think that people were honestly just mislead to =20
>>>> think that
>>>> diff-funcs output is part of the patch
>>>
>>> Yes, exactly, in my case, that's all it was--the "diff" I thought =20
>>> 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--=20
>>> hence the
>>> original question.
>>
>> I apologize if my request for "English" seemed Anglo-centric.  What =
=20
>> I intended
>> was "non-machine-generated" and expressed in the natural language =20
>> that is
>> usual for Linux kernel patch descriptions, which appears to be =20
>> 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 =20
difficult to parse.  My request is to try harder next time to make =20
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 =20
> that.
> As I admitted earlier the title wasn't as good as it should have =20
> (and I
> even sent a followup to please all including even the in body sentenc=
e
> plus the fixed title). Not that I find the title really a problem to =
=20
> sane
> people who understand that code-duplication is bad and getting rid =20
> of it
> is what one should do in general, as long as it's possible without =20
> making
> a mess out of it.

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

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

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

I was too harsh to ask for no machine-generated patch descriptions.  =20
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) =20
> doesn't
> seem like useful information to reviewer I don't really know what to =
=20
> say.

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

>> In addition to crafting a legible description, my suggestions were =20
>> about
>> cc'ing the proper mailing list, and routing this patch directly to =20
>> the sunrpc
>> kernel maintainers in the first place, all of which are listed =20
>> within easy
>> reach in MAINTAINERS.
>
> Please note that I couldn't guess that I'd have to look for another =20
> entry
> after first SUNRPC entry in the MAINTAINERS. So asking outsider to =20
> 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 =
=20
> could
> I have guessed? Forgetting to lookup the address from MAINTAINERS =20
> into the
> first mail is something which just happened and I'm certainly guilty =
=20
> to
> that :-).

Yes, we should fix the SUNRPC entries in MAINTAINERS.

> ...I've had enough down turning experiences with random dev lists =20
> listed
> in the MAINTAINERS (mostly spammy subsriber only ones, nearly infinit=
e
> response time, etc things) that I mostly ignore them nowadays though =
=20
> this
> time it would have been vger list so no subscription required madness=
=2E
> 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 =20
maintainer and get a "please use such-and-such list" response.  The =20
exception is usually very specific device drivers.

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

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

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

Yes, yours was a simple and proper change in the end, but still.  =20
Given the long "dialog" just had on lkml about compiler (machine-=20
generated) optimizations, I think we would all be just a little =20
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 =20
> 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 =20
> 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, =20
> 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 =20
analysis is worth at least noting during review.  It helps us =20
understand where we have to make the code more clear.

The question is whether the code itself documents dependencies on =20
other parts of the code, whether such dependencies are only documented =
=20
by comments, or whether it is entirely implicit and up to the folks =20
who work on it to understand and remember. This is usually only an =20
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 =20
our code.  For open source especially it is a larger issue than simple =
=20
code maintenance.  It is not enough for open source code simply to =20
work.  If people outside our team (or outside the larger Linux =20
community) can't easily understand the important features of our open =20
source code, then we've still failed, even if the legal barriers have =20
been lifted by the GPL.

Thus I'm especially sensitive to areas where our source code does not =20
express itself well to folks who are not familiar.  We want to invite =20
wider review, and that's simply not possible if nobody understands =20
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(), =20
>> which
>> is meant to be transport-generic.
>
> IMHO, this is the most useful response so far! :-) Usually the hardes=
t
> obstacles I face with non-familiar code is its structure and naming.


--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] net/sunrpc/xprtsock.c: some common code found

[J. Bruce Fields]

On Tue, Feb 10, 2009 at 03:20:36PM +0200, Ilpo J=C3=A4rvinen wrote:
> On Sun, 8 Feb 2009, J. Bruce Fields wrote:
>=20
> > (Where's diff-func from?)
>=20
> My own tool collection. I can make it available if you want to.

Sure, I'd be curious.

--b.

>=20
> > In similar cases in the future, if you want to cater to the more ti=
red
> > and/or mentally challenged among us (which may just be me), some cl=
ue
> > that visually distinguishes the quoted diff output in the comment m=
ight
> > help; e.g., maybe indent it like the below.
> >=20
> > (But that's intended as a suggestion, not a requirement.)
>=20
> Yeah, a good idea... I'll sed some spaces in front of them in the fut=
ure=20
> to make the difference more obvious. The expected outcome though is t=
hat=20
> somebody probably then hits reply button too soon claiming that the p=
atch=20
> is broken :-).
>=20
>=20
> --=20
>  i.