On Nov 30, 2007, at 4:39 PM, Tom Tucker wrote:
On Fri, 2007-11-30 at 15:46 -0500, Chuck Lever wrote:
On Nov 29, 2007, at 5:40 PM, Tom Tucker wrote:
+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;
+}
As far as I can tell you never return "required." The result of
svc_udp_has_wspace() is really a boolean, right? Or did I miss your
point?
+
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?
If sk_reserved goes negative, it will be converted to unsigned, and
become a very large positive number. The result of the sum will be
recast back to an int when it's assigned to "required," and we
probably get a reasonable result. I doubt an explicit cast will
change things at all.
Instead, perhaps we should add an explicit check to ensure
sk_reserved is a reasonable positive value before doing any other
checks. (Likewise in the UDP case as well).
I wonder if this is really the correct write space check to use for
TCP, though. I remember fixing a similar issue in the RPC client a
long time ago -- both UDP and TCP used the same wspace check. It
resulted in the sk_write_space callback hammering on the RPC client,
and forward progress on TCP socket writes would slow to a crawl.
You probably want something like this instead:
set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
required = atomic_read(&svsk->sk_reserved) + serv->sv_max_mesg;
wspace = sk_stream_wspace(svsk->sk_sk);
if (wspace < sk_stream_min_wspace(svsk->sk_sk))
return 0;
if (required * 2 > wspace)
return 0;
clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
return 1;
The first test mimics sk_stream_write_space() and xs_tcp_write_space
(). I'm still unsure what to do about the possibility of one of
these signed integers going negative on us.
Bruce? Neil? What sayest thou?
+ 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
