Message-ID: <Pine.LNX.4.64.0902070040530.31875@wrl-59.cs.helsinki.fi>
Date:	Sat, 7 Feb 2009 00:58:44 +0200 (EET)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	Chuck Lever <chuck.lever@...cle.com>
cc:	"J. Bruce Fields" <bfields@...ldses.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

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@...ldses.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@...r.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@...sinki.fi>
---
 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

Powered by blists - more mailing lists