Re: [RFC,PATCH 09/38] svc: Add a transport function that checks for write space

[Tom Tucker <tom@opengridcomputing.com>]

On Fri, 2007-11-30 at 15:46 -0500, Chuck Lever wrote:
> On Nov 29, 2007, at 5:40 PM, Tom Tucker wrote:
> > In order to avoid blocking a service thread, the receive side checks
> > to see if there is sufficient write space to reply to the request.
> > Each transport has a different mechanism for determining if there is
> > enough write space to reply.
> >
> > The code that checked for white space was coupled with code that
> 
> s/white space/write space/

ok.

> 
> > checked for CLOSE and CONN. These checks have been broken out into
> > separate statements to make the code easier to read.
> >
> > Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> > ---
> >
> >  include/linux/sunrpc/svc_xprt.h |    1 +
> >  net/sunrpc/svcsock.c            |   60 ++++++++++++++++++++++++++++ 
> > +++++------
> >  2 files changed, 51 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/ 
> > svc_xprt.h
> > index 8501115..3adc8f3 100644
> > --- a/include/linux/sunrpc/svc_xprt.h
> > +++ b/include/linux/sunrpc/svc_xprt.h
> > @@ -10,6 +10,7 @@
> >  #include <linux/sunrpc/svc.h>
> >
> >  struct svc_xprt_ops {
> > +	int		(*xpo_has_wspace)(struct svc_xprt *);
> >  	int		(*xpo_recvfrom)(struct svc_rqst *);
> >  	void		(*xpo_prep_reply_hdr)(struct svc_rqst *);
> >  	int		(*xpo_sendto)(struct svc_rqst *);
> > diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> > index 510ad45..b796244 100644
> > --- a/net/sunrpc/svcsock.c
> > +++ b/net/sunrpc/svcsock.c
> > @@ -269,22 +269,24 @@ svc_sock_enqueue(struct svc_sock *svsk)
> >  	BUG_ON(svsk->sk_pool != NULL);
> >  	svsk->sk_pool = pool;
> >
> > -	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
> > -	if (((atomic_read(&svsk->sk_reserved) + serv->sv_max_mesg)*2
> > -	     > svc_sock_wspace(svsk))
> > -	    && !test_bit(SK_CLOSE, &svsk->sk_flags)
> > -	    && !test_bit(SK_CONN, &svsk->sk_flags)) {
> > +	/* Handle pending connection */
> > +	if (test_bit(SK_CONN, &svsk->sk_flags))
> > +		goto process;
> > +
> > +	/* Handle close in-progress */
> > +	if (test_bit(SK_CLOSE, &svsk->sk_flags))
> > +		goto process;
> > +
> > +	/* Check if we have space to reply to a request */
> > +	if (!svsk->sk_xprt.xpt_ops->xpo_has_wspace(&svsk->sk_xprt)) {
> >  		/* Don't enqueue while not enough space for reply */
> > -		dprintk("svc: socket %p  no space, %d*2 > %ld, not enqueued\n",
> > -			svsk->sk_sk, atomic_read(&svsk->sk_reserved)+serv->sv_max_mesg,
> > -			svc_sock_wspace(svsk));
> > +		dprintk("svc: no write space, socket %p  not enqueued\n", svsk);
> 
> Since you remove the only callers of svc_sock_wspace here, you can  
> probably safely delete that function in this patch as well.
> 

ok.

> >  		svsk->sk_pool = NULL;
> >  		clear_bit(SK_BUSY, &svsk->sk_flags);
> >  		goto out_unlock;
> >  	}
> > -	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
> > -
> >
> > + process:
> >  	if (!list_empty(&pool->sp_threads)) {
> >  		rqstp = list_entry(pool->sp_threads.next,
> >  				   struct svc_rqst,
> > @@ -897,6 +899,24 @@ static void svc_udp_prep_reply_hdr(struct  
> > svc_rqst *rqstp)
> >  {
> >  }
> >
> > +static int svc_udp_has_wspace(struct svc_xprt *xprt)
> > +{
> > +	struct svc_sock *svsk = container_of(xprt, struct svc_sock,  
> > sk_xprt);
> > +	struct svc_serv	*serv = svsk->sk_server;
> > +	int required;
> > +
> > +	/*
> > +	 * Set the SOCK_NOSPACE flag before checking the available
> > +	 * sock space.
> > +	 */
> > +	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
> > +	required = atomic_read(&svsk->sk_reserved) + serv->sv_max_mesg;
> 
> The result of the sum is unsigned, but then we stuff it into a signed  
> integer...
> 
> > +	if (required*2 > sock_wspace(svsk->sk_sk))
> > +		return 0;
> 
> ...and then this introduces a mixed sign comparison (harmless  
> AFAICT).  Perhaps "required" should be an unsigned long.
> 

So for svc_udp_has_wspace, it makes sense for required to 
be unsigned, and then demote to signed on return. Yes?

> 
> Also, some may prefer "<< 1" to "* 2".  I'm not sure it makes much  
> difference here.  Arguably, it might be slightly better documentation  
> to double "required" before the if statement.
> 
> > +	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
> > +	return 1;
> > +}
> > +
> >  static struct svc_xprt_ops svc_udp_ops = {
> >  	.xpo_recvfrom = svc_udp_recvfrom,
> >  	.xpo_sendto = svc_udp_sendto,
> > @@ -904,6 +924,7 @@ static struct svc_xprt_ops svc_udp_ops = {
> >  	.xpo_detach = svc_sock_detach,
> >  	.xpo_free = svc_sock_free,
> >  	.xpo_prep_reply_hdr = svc_udp_prep_reply_hdr,
> > +	.xpo_has_wspace = svc_udp_has_wspace,
> >  };
> >
> >  static struct svc_xprt_class svc_udp_class = {
> > @@ -1366,6 +1387,24 @@ static void svc_tcp_prep_reply_hdr(struct  
> > svc_rqst *rqstp)
> >  	svc_putnl(resv, 0);
> >  }
> >
> > +static int svc_tcp_has_wspace(struct svc_xprt *xprt)
> > +{
> > +	struct svc_sock *svsk = container_of(xprt, struct svc_sock,  
> > sk_xprt);
> > +	struct svc_serv	*serv = svsk->sk_server;
> > +	int required;
> > +
> > +	/*
> > +	 * Set the SOCK_NOSPACE flag before checking the available
> > +	 * sock space.
> > +	 */
> > +	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
> > +	required = atomic_read(&svsk->sk_reserved) + serv->sv_max_mesg;
> 
> Ibid.
> 
> > +	if (required*2 > sk_stream_wspace(svsk->sk_sk))
> > +		return 0;
> 
> Oddly sk_stream_wspace() returns an int, but sock_space() returns an  
> unsigned long.  Sigh...

For this one, let's leave required signed and add an explicit cast to
serv->sv_max_mesg. Sound ok?

What a mess...

> 
> > +	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
> > +	return 1;
> > +}
> > +
> >  static struct svc_xprt_ops svc_tcp_ops = {
> >  	.xpo_recvfrom = svc_tcp_recvfrom,
> >  	.xpo_sendto = svc_tcp_sendto,
> > @@ -1373,6 +1412,7 @@ static struct svc_xprt_ops svc_tcp_ops = {
> >  	.xpo_detach = svc_sock_detach,
> >  	.xpo_free = svc_sock_free,
> >  	.xpo_prep_reply_hdr = svc_tcp_prep_reply_hdr,
> > +	.xpo_has_wspace = svc_tcp_has_wspace,
> >  };
> >
> >  static struct svc_xprt_class svc_tcp_class = {
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com