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 <[EMAIL PROTECTED]>
> > ---
> >
> > 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
-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
