From: "J. Bruce Fields" Subject: Re: [RFC] [PATCH 1/1] tcp-autotuning-on-recv-window-fix Date: Wed, 22 Oct 2008 17:52:49 -0400 Message-ID: <20081022215249.GD7454@fieldses.org> References: <48FE200A.6070805@citi.umich.edu> <20081022194605.GA4409@fieldses.org> <48FF9B87.6050909@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Olga Kornievskaia , linux-nfs@vger.kernel.org To: Dean Hildebrand Return-path: Received: from mail.fieldses.org ([66.93.2.214]:44811 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753772AbYJVVww (ORCPT ); Wed, 22 Oct 2008 17:52:52 -0400 In-Reply-To: <48FF9B87.6050909@gmail.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Wed, Oct 22, 2008 at 02:30:47PM -0700, Dean Hildebrand wrote: > > > J. Bruce Fields wrote: >> On Tue, Oct 21, 2008 at 02:31:38PM -0400, Olga Kornievskaia wrote: >> >>> From: Olga Kornievskaia >>> Date: Tue, 21 Oct 2008 14:13:47 -0400 >>> Subject: [RFC] [PATCH 1/1] tcp-autotuning-on-recv-window-fix >>> >>> This patch allows for the NFSv4 server to make use of TCP autotuning behaviour >>> which was previously disabled by setting sk_userlocks variable. >>> >>> This patch sets the receive buffers to be big enough to receive the >>> whole RPC request. This buffer size had to be set for the listening >>> socket and not >>> the accept socket as it was previously done. >> >> The point there being that our previous buffer-size settings were made >> too late to actually have an affect? >> >> >>> This patch removes the code that readjust the receive/send buffer sizes for >>> the accepted socket. Previously this code was used to influence the TCP >>> window management behaviour which is no longer needed when autotuning >>> is enabled. >> >> Could we get a really brief summary of the performance improvement for a >> high-speed network, to include in the commit message? >> >> The one remaining worry I recall is that we assume the tcp autotuning >> never decreases the size of the buffer below the size we initially >> requested. Apparently that assumption is true. There's some worry >> about whether that's true by design or merely true of the current >> implementation. >> > If it does happen, I assume the fix there is to set the minimum tcp > buffer settings big enough for a single request? That might be a workaround. The fix would be a kernel patch. That doesn't look likely. > In fact, the big impact here I believe is that NFS will finally start > using the linux tcp buffer settings (like every other tool). You're talking about the various sysctls? Since they're global, I don't think they're really very useful as tools to tune nfsd. --b. > Is there > any way to > get some documentation out there that this is the case? Maybe an > addition to the website would be the right place for now? > Dean >> That doesn't look like a big worry--I'm inclined to apply this patch as >> is--but moving the sk_{rcv,snd}buf assignments to a simple function in >> the networking code and documenting the requirements there might be a >> nice thing to do (as a separate patch). >> >> --b. >> >> >>> Signed-off-by: Olga Kornievskaia >>> --- >>> net/sunrpc/svcsock.c | 35 +++++++---------------------------- >>> 1 files changed, 7 insertions(+), 28 deletions(-) >>> >>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c >>> index 3e65719..4bb535e 100644 >>> --- a/net/sunrpc/svcsock.c >>> +++ b/net/sunrpc/svcsock.c >>> @@ -349,7 +349,6 @@ static void svc_sock_setbufsize(struct socket *sock, unsigned int snd, >>> lock_sock(sock->sk); >>> sock->sk->sk_sndbuf = snd * 2; >>> sock->sk->sk_rcvbuf = rcv * 2; >>> - sock->sk->sk_userlocks |= SOCK_SNDBUF_LOCK|SOCK_RCVBUF_LOCK; >>> release_sock(sock->sk); >>> #endif >>> } >>> @@ -801,23 +800,6 @@ static int svc_tcp_recvfrom(struct svc_rqst *rqstp) >>> test_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags), >>> test_bit(XPT_CLOSE, &svsk->sk_xprt.xpt_flags)); >>> - if (test_and_clear_bit(XPT_CHNGBUF, &svsk->sk_xprt.xpt_flags)) >>> - /* sndbuf needs to have room for one request >>> - * per thread, otherwise we can stall even when the >>> - * network isn't a bottleneck. >>> - * >>> - * We count all threads rather than threads in a >>> - * particular pool, which provides an upper bound >>> - * on the number of threads which will access the socket. >>> - * >>> - * rcvbuf just needs to be able to hold a few requests. >>> - * Normally they will be removed from the queue >>> - * as soon a a complete request arrives. >>> - */ >>> - svc_sock_setbufsize(svsk->sk_sock, >>> - (serv->sv_nrthreads+3) * serv->sv_max_mesg, >>> - 3 * serv->sv_max_mesg); >>> - >>> clear_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags); >>> /* Receive data. If we haven't got the record length yet, get >>> @@ -1065,15 +1047,6 @@ static void svc_tcp_init(struct svc_sock *svsk, struct svc_serv *serv) >>> tcp_sk(sk)->nonagle |= TCP_NAGLE_OFF; >>> - /* initialise setting must have enough space to >>> - * receive and respond to one request. >>> - * svc_tcp_recvfrom will re-adjust if necessary >>> - */ >>> - svc_sock_setbufsize(svsk->sk_sock, >>> - 3 * svsk->sk_xprt.xpt_server->sv_max_mesg, >>> - 3 * svsk->sk_xprt.xpt_server->sv_max_mesg); >>> - >>> - set_bit(XPT_CHNGBUF, &svsk->sk_xprt.xpt_flags); >>> set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags); >>> if (sk->sk_state != TCP_ESTABLISHED) >>> set_bit(XPT_CLOSE, &svsk->sk_xprt.xpt_flags); >>> @@ -1143,8 +1116,14 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv, >>> /* Initialize the socket */ >>> if (sock->type == SOCK_DGRAM) >>> svc_udp_init(svsk, serv); >>> - else >>> + else { >>> + /* initialise setting must have enough space to >>> >> >> s/initialise/initial/ >> >> >>> + * receive and respond to one request. >>> + */ >>> + svc_sock_setbufsize(svsk->sk_sock, 4 * serv->sv_max_mesg, >>> + 4 * serv->sv_max_mesg); >>> svc_tcp_init(svsk, serv); >>> + } >>> dprintk("svc: svc_setup_socket created %p (inet %p)\n", >>> svsk, svsk->sk_sk); >>> -- >>> 1.5.0.2 >>> >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >>