2008-06-10 19:03:43

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH 2/2] SUNRPC: Ensure all transports set rq_xtime consistently

On Fri, 2008-06-06 at 10:22 -0700, Chuck Lever wrote:
> The RPC client uses the rq_xtime field in each RPC request to
> determine the
> round-trip time of the request. Currently, the rq_xtime field is
> initialized by each transport just before it starts enqueing a request
> to
> be sent. However, transports do not handle initializing this value
> consistently; sometimes they don't initialize it at all.
>
> To make the measurement of request round-trip time consistent for all
> RPC client transport capabilities, pull rq_xtime initialization into
> the
> RPC client's generic transport logic. Now all transports will get a
> standardized RTT measure automatically, from:
>
> xprt_transmit()
>
> to
>
> xprt_complete_rqst()
>
> This makes round-trip time calculation more accurate for the TCP
> transport.
> The socket ->sendmsg() method can return "-EAGAIN" if the socket's
> output
> buffer is full, so the TCP transport's ->send_request() method may
> call
> the ->sendmsg() method repeatedly until it gets all of the request's
> bytes
> queued in the socket's buffer.
>
> Currently, the TCP transport sets the rq_xtime field every time
> through
> that loop so the final value is the timestamp just before the *last*
> call
> to the underlying socket's ->sendmsg() method. After this patch, the
> rq_xtime field contains a timestamp that reflects the time just before
> the
> *first* call to ->sendmsg().
>
> This is consequential under heavy workloads because large requests
> often
> take multiple ->sendmsg() calls to get all the bytes of a request
> queued.
> The TCP transport causes the request to sleep until the remote end of
> the
> socket has received enough bytes to clear space in the socket's local
> output buffer. This delay can be quite significant.
>
> The method introduced by this patch is a more accurate measure of RTT
> for stream transports, since the server can cause enough back pressure
> to delay (ie increase the latency of) requests from the client.
>
> Additionally, this patch corrects the behavior of the RDMA transport,
> which
> entirely neglected to initialize the rq_xtime field. RPC performance
> metrics for RDMA transports now display correct RPC request round trip
> times.
>
> Signed-off-by: Chuck Lever <[email protected]>
> Acked-by: Tom Talpey <[email protected]>
> ---
>
> net/sunrpc/xprt.c | 1 +
> net/sunrpc/xprtsock.c | 2 --
> 2 files changed, 1 insertions(+), 2 deletions(-)
>
>
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index 67996bd..99a52aa 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -872,6 +872,7 @@ void xprt_transmit(struct rpc_task *task)
> return;
>
> req->rq_connect_cookie = xprt->connect_cookie;
> + req->rq_xtime = jiffies;
> status = xprt->ops->send_request(task);
> if (status == 0) {
> dprintk("RPC: %5u xmit complete\n", task->tk_pid);
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index ddbe981..4486c59 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -579,7 +579,6 @@ static int xs_udp_send_request(struct rpc_task
> *task)
> req->rq_svec->iov_base,
> req->rq_svec->iov_len);
>
> - req->rq_xtime = jiffies;
> status = xs_sendpages(transport->sock,
> xs_addr(xprt),
> xprt->addrlen, xdr,
> @@ -671,7 +670,6 @@ static int xs_tcp_send_request(struct rpc_task
> *task)
> * to cope with writespace callbacks arriving _after_ we have
> * called sendmsg(). */
> while (1) {
> - req->rq_xtime = jiffies;
> status = xs_sendpages(transport->sock,
> NULL, 0, xdr,
> req->rq_bytes_sent);
>

Please resend as a _text_ patch. base64-encoded HTML isn't an acceptable
format for patches...

--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com


2008-06-10 19:39:43

by Chuck Lever III

[permalink] [raw]
Subject: Re: [PATCH 2/2] SUNRPC: Ensure all transports set rq_xtime consistently

On Tue, Jun 10, 2008 at 3:03 PM, Trond Myklebust
<[email protected]> wrote:
> On Fri, 2008-06-06 at 10:22 -0700, Chuck Lever wrote:
>> The RPC client uses the rq_xtime field in each RPC request to
>> determine the
>> round-trip time of the request. Currently, the rq_xtime field is
>> initialized by each transport just before it starts enqueing a request
>> to
>> be sent. However, transports do not handle initializing this value
>> consistently; sometimes they don't initialize it at all.
>>
>> To make the measurement of request round-trip time consistent for all
>> RPC client transport capabilities, pull rq_xtime initialization into
>> the
>> RPC client's generic transport logic. Now all transports will get a
>> standardized RTT measure automatically, from:
>>
>> xprt_transmit()
>>
>> to
>>
>> xprt_complete_rqst()
>
> Please resend as a _text_ patch. base64-encoded HTML isn't an acceptable
> format for patches...

Sorry, I didn't do that on purpose. I posted this via 'stg mail', but
it looks like the gmail.com MTA converts these. I will try a
different outgoing mail service.

--
Chuck Lever