Skip to contentSkip to conversation

knfsd: delete per transport

RFC PATCH In review Greg Banks 3 participants 7 messages May 16, 2007 – May 18, 2007

Subsystems (3)
lore
Share & tools
lore

Greg Banks

author
#0 lore raw

RFC PATCH 2/14 knfsd: delete per transport

Add sko_detach and sko_free methods to svc_sock_ops and change svc_delete_socket() and svc_sock_put() to use them.

---

 include/linux/sunrpc/svcsock.h |   12 ++++++
 net/sunrpc/svcsock.c           |   54 +++++++++++++++++++++++-------
 2 files changed, 55 insertions(+), 11 deletions(-)
net/sunrpc/svcsock.c +43 -11
Index: linux/net/sunrpc/svcsock.c
===================================================================
@@ -83,6 +83,8 @@ static void		svc_udp_data_ready(struct s
 static int		svc_udp_recvfrom(struct svc_rqst *);
 static int		svc_udp_sendto(struct svc_rqst *);
 static void		svc_close_socket(struct svc_sock *svsk);
+static void		svc_tcpip_detach(struct svc_sock *);
+static void		svc_tcpip_free(struct svc_sock *);
 
 static struct svc_deferred_req *svc_deferred_dequeue(struct svc_sock *svsk);
 static int svc_deferred_recv(struct svc_rqst *rqstp);
@@ -377,14 +379,9 @@ svc_sock_put(struct svc_sock *svsk)
 	if (atomic_dec_and_test(&svsk->sk_inuse)) {
 		BUG_ON(! test_bit(SK_DEAD, &svsk->sk_flags));
 
-		dprintk("svc: releasing dead socket\n");
-		if (svsk->sk_sock->file)
-			sockfd_put(svsk->sk_sock);
-		else
-			sock_release(svsk->sk_sock);
 		if (svsk->sk_info_authunix != NULL)
 			svcauth_unix_info_release(svsk->sk_info_authunix);
-		kfree(svsk);
+    	    	svsk->sk_ops->sko_free(svsk);
 	}
 }
 
@@ -886,7 +883,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
 static const struct svc_sock_ops svc_udp_ops = {
 	.sko_name = "udp",
 	.sko_recvfrom = svc_udp_recvfrom,
-	.sko_sendto = svc_udp_sendto
+	.sko_sendto = svc_udp_sendto,
+	.sko_detach = svc_tcpip_detach,
+	.sko_free = svc_tcpip_free
 };
 
 static void
@@ -1328,7 +1327,9 @@ svc_tcp_sendto(struct svc_rqst *rqstp)
 static const struct svc_sock_ops svc_tcp_ops = {
 	.sko_name = "tcp",
 	.sko_recvfrom = svc_tcp_recvfrom,
-	.sko_sendto = svc_tcp_sendto
+	.sko_sendto = svc_tcp_sendto,
+	.sko_detach = svc_tcpip_detach,
+	.sko_free = svc_tcpip_free
 };
 
 static void
@@ -1768,6 +1769,38 @@ bummer:
 }
 
 /*
+ * Detach the svc_sock from the socket so that no
+ * more callbacks occur.
+ */
+static void
+svc_tcpip_detach(struct svc_sock *svsk)
+{
+	struct sock *sk = svsk->sk_sk;
+
+	dprintk("svc: svc_sock_detach(%p)\n", svsk);
+
+	/* put back the old socket callbacks */
+	sk->sk_state_change = svsk->sk_ostate;
+	sk->sk_data_ready = svsk->sk_odata;
+	sk->sk_write_space = svsk->sk_owspace;
+}
+
+/*
+ * Free the svc_sock's socket resources and the svc_sock itself.
+ */
+static void
+svc_tcpip_free(struct svc_sock *svsk)
+{
+	dprintk("svc: svc_sock_free(%p)\n", svsk);
+
+	if (svsk->sk_sock->file)
+		sockfd_put(svsk->sk_sock);
+	else
+		sock_release(svsk->sk_sock);
+	kfree(svsk);
+}
+
+/*
  * Remove a dead socket
  */
 static void
@@ -1781,9 +1814,8 @@ svc_delete_socket(struct svc_sock *svsk)
 	serv = svsk->sk_server;
 	sk = svsk->sk_sk;
 
-	sk->sk_state_change = svsk->sk_ostate;
-	sk->sk_data_ready = svsk->sk_odata;
-	sk->sk_write_space = svsk->sk_owspace;
+	if (svsk->sk_ops->sko_detach)
+		svsk->sk_ops->sko_detach(svsk);
 
 	spin_lock_bh(&serv->sv_lock);
 
include/linux/sunrpc/svcsock.h +12 -0
Index: linux/include/linux/sunrpc/svcsock.h
===================================================================
@@ -15,6 +15,18 @@ struct svc_sock_ops {
 	const char		*sko_name;
 	int			(*sko_recvfrom)(struct svc_rqst *rqstp);
 	int			(*sko_sendto)(struct svc_rqst *rqstp);
+	/*
+	 * Detach the svc_sock from it's socket, so that the
+	 * svc_sock will not be enqueued any more.  This is
+	 * the first stage in the destruction of a svc_sock.
+	 */
+	void			(*sko_detach)(struct svc_sock *);
+	/*
+	 * Release all network-level resources held by the svc_sock,
+	 * and the svc_sock itself.  This is the final stage in the
+	 * destruction of a svc_sock.
+	 */
+	void			(*sko_free)(struct svc_sock *);
 };
 
 /*
Signature · 12 lines
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
#1 lore raw

Trivial comments, since I couldn't find any real problems:

net/sunrpc/svcsock.c svc_udp_sendto(struct svc_rqst *rqstp) · line 886

Quoted 6 lines · Greg Banks
net/sunrpc/svcsock.c +1 -1
@@ -886,7 +883,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
 static const struct svc_sock_ops svc_udp_ops = {
 	.sko_name = "udp",
 	.sko_recvfrom = svc_udp_recvfrom,
-	.sko_sendto = svc_udp_sendto
+	.sko_sendto = svc_udp_sendto,

Any objection to always just including a trailing comma and saving a little noise in each diff? That seems to be common practice elsewhere.

net/sunrpc/svcsock.c bummer: · line 1794

Quoted 4 lines
net/sunrpc/svcsock.c +4 -0
+static void
+svc_tcpip_free(struct svc_sock *svsk)
+{
+	dprintk("svc: svc_sock_free(%p)\n", svsk);

We lived without these dprintk's before; do we need them now?

include/linux/sunrpc/svcsock.h struct svc_sock_ops { · line 19

Quoted 8 lines
include/linux/sunrpc/svcsock.h +2 -0
@@ -15,6 +15,18 @@ struct svc_sock_ops {
 	const char		*sko_name;
 	int			(*sko_recvfrom)(struct svc_rqst *rqstp);
 	int			(*sko_sendto)(struct svc_rqst *rqstp);
+	/*
+	 * Detach the svc_sock from it's socket, so that the

s/it's/its/

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#2 lore raw
Quoted 1 line · J. Bruce Fields

Trivial comments, since I couldn't find any real problems:

Well, that's hopeful ;-)

Quoted 10 lines · Greg Banks

net/sunrpc/svcsock.c svc_udp_sendto(struct svc_rqst *rqstp) · line 886

Quoted 6 lines
net/sunrpc/svcsock.c +1 -1
@@ -886,7 +883,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
 static const struct svc_sock_ops svc_udp_ops = {
 	.sko_name = "udp",
 	.sko_recvfrom = svc_udp_recvfrom,
-	.sko_sendto = svc_udp_sendto
+	.sko_sendto = svc_udp_sendto,

Any objection to always just including a trailing comma and saving a little noise in each diff? That seems to be common practice elsewhere.

Fixed.

Quoted 6 lines

net/sunrpc/svcsock.c bummer: · line 1794

Quoted 4 lines
net/sunrpc/svcsock.c +4 -0
+static void
+svc_tcpip_free(struct svc_sock *svsk)
+{
+	dprintk("svc: svc_sock_free(%p)\n", svsk);

We lived without these dprintk's before; do we need them now?

Not really. Removed.

Quoted 6 lines

include/linux/sunrpc/svcsock.h struct svc_sock_ops { · line 19

Quoted 3 lines
include/linux/sunrpc/svcsock.h +2 -0
 	int			(*sko_sendto)(struct svc_rqst *rqstp);
+	/*
+	 * Detach the svc_sock from it's socket, so that the

s/it's/its/

Doh! Fixed.

Thanks Bruce.

Greg.

Signature · 12 lines
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
#3 lore raw
In reply to Greg Banks

net/sunrpc/svcsock.c svc_udp_sendto(struct svc_rqst *rqstp) · line 889

Quoted 10 lines · On Thursday May 17, [email protected] wrote:
net/sunrpc/svcsock.c +3 -1
@@ -886,7 +883,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
 static const struct svc_sock_ops svc_udp_ops = {
 	.sko_name = "udp",
 	.sko_recvfrom = svc_udp_recvfrom,
-	.sko_sendto = svc_udp_sendto
+	.sko_sendto = svc_udp_sendto,
+	.sko_detach = svc_tcpip_detach,
+	.sko_free = svc_tcpip_free
 };

Tiny nit:
Despite widespread usage, I don't see udp as being a part of tcp/ip. So having a function names *tcpip* in the svc_udp_ops looks just wrong.
Maybe svc_ipsock_detach, svc_ipsock_free ??

NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#4 lore raw
Quoted 17 lines · Neil Brown

net/sunrpc/svcsock.c svc_udp_sendto(struct svc_rqst *rqstp) · line 889

Quoted 10 lines · On Thursday May 17, [email protected] wrote:
net/sunrpc/svcsock.c +3 -1
@@ -886,7 +883,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
 static const struct svc_sock_ops svc_udp_ops = {
 	.sko_name = "udp",
 	.sko_recvfrom = svc_udp_recvfrom,
-	.sko_sendto = svc_udp_sendto
+	.sko_sendto = svc_udp_sendto,
+	.sko_detach = svc_tcpip_detach,
+	.sko_free = svc_tcpip_free
 };

Tiny nit:
Despite widespread usage, I don't see udp as being a part of tcp/ip. So having a function names *tcpip* in the svc_udp_ops looks just wrong.
Maybe svc_ipsock_detach, svc_ipsock_free ??

I struggled with this.

The obvious thing would have been to use the word "sock" because what these functions have in common is that they deal with sockets rather than TCP/IP or even IP. None of "tcpip" or "ipsock" accurately reflect what these functions do.

But unfortunately our abstract structure is called "svc_sock". So doing the obvious would result in enormous confusion: is svc_sock_foo() a generic transport function or a specific sockets-based one?

I considered renaming the existing svc_sock to svc_xprt, which would be another horribly unpronouncable identifier(*) but would at least be appropriately abstract and consistent with the client.

Then the naming would all be obvious and correct: svc_xprt_foo() is a generic transport function, svc_sock_foo() is a more specific socket-based transport function used as common code between TCP and UDP transports. Yay.

But it would be a much larger patch so I shied away.

I'm happy to do the naming any way you want, just let me know.

* what did the Sun people who originally coined RPC terminology have against vowels, I wonder?

Greg.

Signature · 12 lines
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
#5 lore raw
Quoted 11 lines · On Friday May 18, [email protected] wrote:

I considered renaming the existing svc_sock to svc_xprt, which would be another horribly unpronouncable identifier(*) but would at least be appropriately abstract and consistent with the client.

Then the naming would all be obvious and correct: svc_xprt_foo() is a generic transport function, svc_sock_foo() is a more specific socket-based transport function used as common code between TCP and UDP transports. Yay.

But it would be a much larger patch so I shied away.

Sensible.

Quoted 2 lines

I'm happy to do the naming any way you want, just let me know.

I accept that "ipsock" is not accurate (as it is the 'sock' bit that is significant, the 'ip' is fairly irrelevant) but it is at least better than tcpip. Another option is
svc_detach_sock / svc_free_sock

Which avoid mentioned "svc_sock", but still says what you want.

So go with that, unless you find it objectionable, in which case got with svc_ipsock_*.

NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#6 lore raw
Quoted 23 lines · Neil Brown
Quoted 11 lines · On Friday May 18, [email protected] wrote:

I considered renaming the existing svc_sock to svc_xprt, which would be another horribly unpronouncable identifier(*) but would at least be appropriately abstract and consistent with the client.

Then the naming would all be obvious and correct: svc_xprt_foo() is a generic transport function, svc_sock_foo() is a more specific socket-based transport function used as common code between TCP and UDP transports. Yay.

But it would be a much larger patch so I shied away.

Sensible.

Quoted 2 lines

I'm happy to do the naming any way you want, just let me know.

I accept that "ipsock" is not accurate (as it is the 'sock' bit that is significant, the 'ip' is fairly irrelevant) but it is at least better than tcpip. Another option is
svc_detach_sock / svc_free_sock

Quoted 4 lines

Which avoid mentioned "svc_sock", but still says what you want.

So go with that, unless you find it objectionable, in which case got with svc_ipsock_*.

I really like having the module at the start and the verb at the end, so I'll go for svc_ipsock_foo().

Greg.

Signature · 12 lines
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs