Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Bruce Fields <bfields@redhat.com>
Subject: Re: [PATCH] SUNRPC: Use TCP_CORK to optimise send performance on the
 server
Thread-Topic: [PATCH] SUNRPC: Use TCP_CORK to optimise send performance on the
 server
Thread-Index: AQHXAkZmunoZe40LXUmxO15yQAmlIKpWoMoAgAAEs4CAABZCgA==
Date:   Sat, 13 Feb 2021 23:30:02 +0000
Message-ID: <4CD2739A-D39B-48C9-BCCF-A9DF1047D507@oracle.com>
References: <20210213202532.23146-1-trondmy@kernel.org>
 <952C605B-C072-4C6B-B9C0-88C25A3B891E@oracle.com>
 <f025fa709f923255b9cb8e76a9b5ad4cca9355c4.camel@hammerspace.com>
In-Reply-To: <f025fa709f923255b9cb8e76a9b5ad4cca9355c4.camel@hammerspace.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <BFBD76DF31B89C40BD09948E371C4CA2@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 77dab3f3-e186-419a-7de2-08d8d077497b
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Feb 2021 23:30:02.4783
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: nbJyQt3NandKu9HdwfQxTnjkoweviDSkrM8uS4IRkGRifapAhMkEMlfRzVjkta/tGNwVX+63b8cFqZOGglu6Wg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2645
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9894 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 suspectscore=0
 mlxscore=0 adultscore=0 spamscore=0 mlxlogscore=999 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2102130216
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9894 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 clxscore=1015 impostorscore=0
 mlxscore=0 phishscore=0 mlxlogscore=999 spamscore=0 bulkscore=0
 priorityscore=1501 malwarescore=0 suspectscore=0 adultscore=0
 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2102130215
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Feb 13, 2021, at 5:10 PM, Trond Myklebust <trondmy@hammerspace.com> wr=
ote:
>=20
> On Sat, 2021-02-13 at 21:53 +0000, Chuck Lever wrote:
>> Hi Trond-
>>=20
>>> On Feb 13, 2021, at 3:25 PM, trondmy@kernel.org wrote:
>>>=20
>>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>>=20
>>> Use a counter to keep track of how many requests are queued behind
>>> the
>>> xprt->xpt_mutex, and keep TCP_CORK set until the queue is empty.
>>=20
>> I'm intrigued, but IMO, the patch description needs to explain
>> why this change should be made. Why abandon Nagle?
>>=20
>=20
> This doesn't change the Nagle/TCP_NODELAY settings. It just switches to
> using the new documented kernel interface.
>=20
> The only change is to use TCP_CORK so that we don't send out partially
> filled TCP frames, when we can see that there are other RPC replies
> that are queued up for transmission.
>=20
> Note the combination TCP_CORK+TCP_NODELAY is common, and the main
> effect of the latter is that when we turn off the TCP_CORK, then there
> is an immediate forced push of the TCP queue.

The description above suggests the patch is just a
clean-up, but a forced push has potential to change
the server's behavior.

I'm trying to assess what kind of additional testing
would be valuable.


>> If you expect a performance impact, the description should
>> provide metrics to show it.
>=20
> I don't have a performance system to measure the improvement
> accurately.

Then let's have Daire try it out, if possible.


> However I am seeing an notable improvement with the
> equivalent client change. Specifically, xfstests generic/127 shows a
> ~20% improvement on my VM based test rig.

>> (We should have Daire try this change with his multi-client
>> setup).
>>=20
>>=20
>>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> ---
>>> include/linux/sunrpc/svcsock.h | 2 ++
>>> net/sunrpc/svcsock.c           | 8 +++++++-
>>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>>=20
>>> diff --git a/include/linux/sunrpc/svcsock.h
>>> b/include/linux/sunrpc/svcsock.h
>>> index b7ac7fe68306..bcc555c7ae9c 100644
>>> --- a/include/linux/sunrpc/svcsock.h
>>> +++ b/include/linux/sunrpc/svcsock.h
>>> @@ -35,6 +35,8 @@ struct svc_sock {
>>>         /* Total length of the data (not including fragment
>>> headers)
>>>          * received so far in the fragments making up this rpc: */
>>>         u32                     sk_datalen;
>>> +       /* Number of queued send requests */
>>> +       atomic_t                sk_sendqlen;
>>=20
>> Can you take advantage of xpt_mutex: update this field
>> only in the critical section, and make it a simple
>> integer type?

I see why atomic_inc(&sk_sendqlen); has to happen outside
the mutex. However, it seems a little crazy/costly to have
both of these serialization primitives.

I haven't found a mutex_unlock() that will report that there
are no waiters to wake up, though.


>>>         struct page *           sk_pages[RPCSVC_MAXPAGES];      /*
>>> received data */
>>> };
>>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>>> index 5a809c64dc7b..231f510a4830 100644
>>> --- a/net/sunrpc/svcsock.c
>>> +++ b/net/sunrpc/svcsock.c
>>> @@ -1171,18 +1171,23 @@ static int svc_tcp_sendto(struct svc_rqst
>>> *rqstp)
>>>=20
>>>         svc_tcp_release_rqst(rqstp);
>>>=20
>>> +       atomic_inc(&svsk->sk_sendqlen);
>>>         mutex_lock(&xprt->xpt_mutex);
>>>         if (svc_xprt_is_dead(xprt))
>>>                 goto out_notconn;
>>> +       tcp_sock_set_cork(svsk->sk_sk, true);
>>>         err =3D svc_tcp_sendmsg(svsk->sk_sock, &msg, xdr, marker,
>>> &sent);
>>>         xdr_free_bvec(xdr);
>>>         trace_svcsock_tcp_send(xprt, err < 0 ? err : sent);
>>>         if (err < 0 || sent !=3D (xdr->len + sizeof(marker)))
>>>                 goto out_close;
>>> +       if (atomic_dec_and_test(&svsk->sk_sendqlen))
>>> +               tcp_sock_set_cork(svsk->sk_sk, false);
>>>         mutex_unlock(&xprt->xpt_mutex);
>>>         return sent;
>>>=20
>>> out_notconn:
>>> +       atomic_dec(&svsk->sk_sendqlen);
>>>         mutex_unlock(&xprt->xpt_mutex);
>>>         return -ENOTCONN;
>>> out_close:
>>> @@ -1192,6 +1197,7 @@ static int svc_tcp_sendto(struct svc_rqst
>>> *rqstp)
>>>                   (err < 0) ? err : sent, xdr->len);
>>>         set_bit(XPT_CLOSE, &xprt->xpt_flags);
>>>         svc_xprt_enqueue(xprt);
>>> +       atomic_dec(&svsk->sk_sendqlen);
>>>         mutex_unlock(&xprt->xpt_mutex);
>>>         return -EAGAIN;
>>> }
>>> @@ -1261,7 +1267,7 @@ static void svc_tcp_init(struct svc_sock
>>> *svsk, struct svc_serv *serv)
>>>                 svsk->sk_datalen =3D 0;
>>>                 memset(&svsk->sk_pages[0], 0, sizeof(svsk-
>>>> sk_pages));
>>>=20
>>> -               tcp_sk(sk)->nonagle |=3D TCP_NAGLE_OFF;
>>> +               tcp_sock_set_nodelay(sk);
>>>=20
>>>                 set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags);
>>>                 switch (sk->sk_state) {
>>> --=20
>>> 2.29.2
>>>=20
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
>=20
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com

--
Chuck Lever