Re: [PATCH v1] xprtrdma: Use xprt_pin_rqst in rpcrdma_reply_handler

[Chuck Lever <chuck.lever@oracle.com>]

> On Sep 5, 2017, at 6:31 PM, Trond Myklebust <trondmy@primarydata.com> wrote:
> 
> On Tue, 2017-09-05 at 17:06 -0400, Chuck Lever wrote:
>>> On Sep 5, 2017, at 4:33 PM, Trond Myklebust <trondmy@primarydata.co
>>> m> wrote:
>>> 
>>> On Thu, 2017-08-24 at 14:18 -0400, Anna Schumaker wrote:
>>>> 
>>>> On 08/23/2017 05:40 PM, Trond Myklebust wrote:
>>>>> On Wed, 2017-08-23 at 17:05 -0400, Chuck Lever wrote:
>>>>>> Adopt the use of xprt_pin_rqst to eliminate contention
>>>>>> between
>>>>>> Call-side users of rb_lock and the use of rb_lock in
>>>>>> rpcrdma_reply_handler.
>>>>>> 
>>>>>> This replaces the mechanism introduced in 431af645cf66
>>>>>> ("xprtrdma:
>>>>>> Fix client lock-up after application signal fires").
>>>>>> 
>>>>>> Use recv_lock to quickly find the completing rqst, pin it,
>>>>>> then
>>>>>> drop the lock. At that point invalidation and pull-up of the
>>>>>> Reply
>>>>>> XDR can be done. Both are often expensive operations.
>>>>>> 
>>>>>> Finally, take recv_lock again to signal completion to the RPC
>>>>>> layer. It also protects adjustment of "cwnd".
>>>>>> 
>>>>>> This greatly reduces the amount of time a lock is held by the
>>>>>> reply handler. Comparing lock_stat results shows a marked
>>>>>> decrease
>>>>>> in contention on rb_lock and recv_lock.
>>>>>> 
>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>>> ---
>>>>>> Hi-
>>>>>> 
>>>>>> If Trond's lock contention series is going into v4.14, I'd
>>>>>> like
>>>>>> you
>>>>>> to consider this one (applied at the end of that series) as
>>>>>> well.
>>>>>> Without it, NFS/RDMA performance regresses a bit after the
>>>>>> first xprt_pin_rqst patch is applied. Thanks!
>>>>>> 
>>>>> 
>>>>> If Anna is OK with it, then I can apply this patch once she's
>>>>> sent
>>>>> me
>>>>> the pull request for the other RDMA client patches.
>>>> 
>>>> Works for me.  Thanks!
>>> 
>>> Please can you both check http://git.linux-nfs.org/?p=trondmy/linux
>>> -nfs
>>> .git;a=commit;h=b1da5d90857ee4b8f5acee1143705412b16fce56 and see if
>>> all
>>> is well?
>>> 
>>> Do note that I did remove the call to rpcrdma_buffer_put() which
>>> was
>>> being called with an uninitialised argument.
>> 
>> Looking at that again. rpcrdma_buffer_put() does look unnecessary,
>> since out_norqst is being called now before "req" is initialized.
>> 
>> On further inspection, "return;" should be replaced with
>> "goto repost;". This is similar to "out_nomatch" and
>> "out_duplicate", which are both being replaced in this patch.
>> 
>> So, out_norqst should look something like this:
>> 
>> 
>> out_norqst:
>>        spin_unlock(&xprt->recv_lock);
>>        dprintk("RPC:       %s: no match for incoming xid 0x%08x\n",
>>                __func__, be32_to_cpu(xid));
>>        goto repost;
>> 
>> 
>> Otherwise, I don't see any obvious problems.
>> 
> 
> Patch updated. Please see http://git.linux-nfs.org/?p=trondmy/linux-nfs
> .git;a=commit;h=9590d083c1bb1419b7992609d1a0a3e3517d3893

out_norqst looks correct.

Note that for out_shortreply:


+out_shortreply:
+       dprintk("RPC:       %s: short/invalid reply\n", __func__);
+       goto repost;


The "goto repost;" can be dropped, since this code drops
right into "repost:". That's harmless, though.


--
Chuck Lever