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: AQHXAkZmunoZe40LXUmxO15yQAmlIKpWoMoAgAAEs4CAABZCgIABGpyAgAASgACAAAO3AIAABO2A
Date:   Sun, 14 Feb 2021 17:58:39 +0000
Message-ID: <BE094630-AA8E-4C7E-ACF8-B153AECA2EDA@oracle.com>
References: <20210213202532.23146-1-trondmy@kernel.org>
 <952C605B-C072-4C6B-B9C0-88C25A3B891E@oracle.com>
 <f025fa709f923255b9cb8e76a9b5ad4cca9355c4.camel@hammerspace.com>
 <4CD2739A-D39B-48C9-BCCF-A9DF1047D507@oracle.com>
 <8ce015080f2f4eb57a81e10041947ad7e6f31499.camel@hammerspace.com>
 <C3A48B63-63AF-4308-A499-15665AB2FF9C@oracle.com>
 <6f49449343dc7ee4efda1c9d9cc56d272c984502.camel@hammerspace.com>
In-Reply-To: <6f49449343dc7ee4efda1c9d9cc56d272c984502.camel@hammerspace.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <82C8B654732E19458A0390DEBB1CF832@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 96f06196-0e2b-4058-69e1-08d8d11228e6
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Feb 2021 17:58:39.7993
 (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: elrjlCZRUY15V6rv1/60VSVtSQeCwX8DfCcHAX3Ng0wiJvXSZEtb4gtzQXZOCf9107XmgDdEd9saSr+KkGSzMw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4814
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9895 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 spamscore=0 mlxscore=0
 phishscore=0 adultscore=0 bulkscore=0 mlxlogscore=999 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2102140155
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9895 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 priorityscore=1501
 lowpriorityscore=0 bulkscore=0 impostorscore=0 mlxlogscore=999
 adultscore=0 malwarescore=0 phishscore=0 clxscore=1015 mlxscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2102140155
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Feb 14, 2021, at 12:41 PM, Trond Myklebust <trondmy@hammerspace.com> w=
rote:
>=20
> On Sun, 2021-02-14 at 17:27 +0000, Chuck Lever wrote:
>>=20
>>=20
>>> On Feb 14, 2021, at 11:21 AM, Trond Myklebust
>>> <trondmy@hammerspace.com> wrote:
>>>=20
>>> On Sat, 2021-02-13 at 23:30 +0000, Chuck Lever wrote:
>>>>=20
>>>>=20
>>>>> On Feb 13, 2021, at 5:10 PM, Trond Myklebust <
>>>>> trondmy@hammerspace.com> wrote:
>>>>>=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.
>>>>=20
>>>> The description above suggests the patch is just a
>>>> clean-up, but a forced push has potential to change
>>>> the server's behavior.
>>>=20
>>> Well, yes. That's very much the point.
>>>=20
>>> Right now, the TCP_NODELAY/Nagle setting means that we're doing
>>> that
>>> forced push at the end of _every_ RPC reply, whether or not there
>>> is
>>> more stuff that can be queued up in the socket. The MSG_MORE is the
>>> only thing that keeps us from doing the forced push on every
>>> sendpage()
>>> call.
>>> So the TCP_CORK is there to _further delay_ that forced push until
>>> we
>>> think the queue is empty.
>>=20
>> My concern is that waiting for the queue to empty before pushing
>> could
>> improve throughput at the cost of increased average round-trip
>> latency.
>> That concern is based on experience I've had attempting to batch
>> sends
>> in the RDMA transport.
>>=20
>>=20
>>> IOW: it attempts to optimise the scheduling of that push until
>>> we're
>>> actually done pushing more stuff into the socket.
>>=20
>> Yep, clear, thanks. It would help a lot if the above were included in
>> the patch description.
>>=20
>> And, I presume that the TCP layer will push anyway if it needs to
>> reclaim resources to handle more queued sends.
>>=20
>> Let's also consider starvation; ie, that the server will continue
>> queuing replies such that it never uncorks. The logic in the patch
>> appears to depend on the client stopping at some point to wait for
>> the
>> server to catch up. There probably should be a trap door that uncorks
>> after a few requests (say, 8) or certain number of bytes are pending
>> without a break.
>=20
> So, firstly, the TCP layer will still push every time a frame is full,
> so traffic does not stop altogether while TCP_CORK is set.

OK.


> Secondly, TCP will also always push on send errors (e.g. when out of free=
 socket
> buffer).

As I presumed. OK.


> Thirdly, it will always push after hitting the 200ms ceiling,
> as described in the tcp(7) manpage.

That's the trap door we need to ensure no starvation or deadlock,
assuming there are no bugs in the TCP layer's logic.

200ms seems a long time to wait, though, when average round trip
latencies are usually under 1ms on typical Ethernet. It would be
good to know how often sending has to wait this long.


> IOW: The TCP_CORK feature is not designed to block the socket forever.
> It is there to allow the application to hint to the TCP layer what it
> needs to do in exactly the same way that MSG_MORE does.

As long as it is only a hint, then we're good.

Out of interest, why not use only MSG_MORE, or remove the use
of MSG_MORE in favor of only cork? If these are essentially the
same mechanism, seems like we need only one or the other.


--
Chuck Lever