On Mon, 2022-03-14 at 21:35 +0000, Chuck Lever III wrote:
> Hey Trond-
>
> I've made some progress getting RPC-with-TLS working in
> the Linux NFS client, but I recently hit an interesting
> snag and could use a little advice.
>
> The software kTLS infrastructure uses do_tcp_sendpages()
> under the covers, and that function is clearing
> SOCKWQ_ASYNC_NOSPACE from under xs_nospace(). That
> prevents xs_run_error_worker() from waking up xprt->sending,
> stalling an RPC transport waiting for more socket write
> space. I'm not sure how to address this, and I'm interested
> in your opinion.
>
How is it achieving this? We only set SOCKWQ_ASYNC_NOSPACE after the
call to xprt_sock_sendmsg().
> For example, why check that flag rather than just waking
> up xprt->sending unconditionally?
The socket code calls ->write_space() in all sorts of situations, so we
need to distinguish between the cases where we are actually waiting for
buffer memory, and the situations where we are not. Otherwise, we'd be
calling xs_run_error_worker() all the time.
>
> Also just for my own understanding of how the write_space
> mechanism is supposed to work for RPC, I instrumented the
> code that bumps and decrements sk_write_pending, and found
> that under normal workloads, the value of that field goes
> negative and stays there. I'm not sure that's intended...?
>
It is not intended, no. Looks like there have been various refactorings
that have mangled that code. From what I can see, the socket code
assumes that sk_write_pending should always be bumped with the
sock_lock() held.
Let me write a fix for that...
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
> On Mar 14, 2022, at 8:06 PM, Trond Myklebust <[email protected]> wrote:
>
> On Mon, 2022-03-14 at 21:35 +0000, Chuck Lever III wrote:
>> Hey Trond-
>>
>> I've made some progress getting RPC-with-TLS working in
>> the Linux NFS client, but I recently hit an interesting
>> snag and could use a little advice.
>>
>> The software kTLS infrastructure uses do_tcp_sendpages()
>> under the covers, and that function is clearing
>> SOCKWQ_ASYNC_NOSPACE from under xs_nospace(). That
>> prevents xs_run_error_worker() from waking up xprt->sending,
>> stalling an RPC transport waiting for more socket write
>> space. I'm not sure how to address this, and I'm interested
>> in your opinion.
>>
>
> How is it achieving this? We only set SOCKWQ_ASYNC_NOSPACE after the
> call to xprt_sock_sendmsg().
A kworker is clearing NOSPACE between the time xs_tcp_send_request()
sets it and the time xs_write_space() runs.
kworker/u128:2-33 [003] 155.723869: rpc_socket_nospace: task:000006cb@00000003 total=262380 remaining=131308
kworker/u128:2-33 [003] 155.723870: bprint: xs_nospace: sk=0xffff88810a8f0a00 setting SOCKWQ_ASYNC_NOSPACE
kworker/u128:2-33 [003] 155.723879: xprt_transmit: task:000006cb@00000003 xid=0x8ab69e2e seqno=0 status=-11
kworker/u128:2-33 [003] 155.723881: xprt_release_xprt: task:000006cc@00000003 snd_task:ffffffff
kworker/3:2-116 [003] 155.723885: bprint: do_tcp_sendpages: sk=0xffff88810a8f0a00 clearing SOCKWQ_ASYNC_NOSPACE
kworker/u128:2-33 [003] 155.723888: rpc_task_run_action: task:000006cc@00000003 flags=ASYNC|MOVEABLE|NORTO|CRED_NOREF runstate=RUNNING|ACTIVE|NEED_XMIT|NEED_RECV status=-11 action=call_transmit_status
kworker/u128:2-33 [003] 155.723889: rpc_task_run_action: task:000006cc@00000003 flags=ASYNC|MOVEABLE|NORTO|CRED_NOREF runstate=RUNNING|ACTIVE|NEED_XMIT|NEED_RECV status=0 action=call_transmit
kworker/u128:2-33 [003] 155.723890: rpc_task_sleep: task:000006cc@00000003 flags=ASYNC|MOVEABLE|NORTO|CRED_NOREF runstate=RUNNING|ACTIVE|NEED_XMIT|NEED_RECV status=-11 timeout=0 queue=xprt_sending
kworker/1:2-115 [001] 155.733398: bprint: do_tcp_sendpages: sk=0xffff88810a8f0a00 clearing SOCKWQ_ASYNC_NOSPACE
kworker/1:2-115 [001] 155.733418: bprint: do_tcp_sendpages: sk=0xffff88810a8f0a00 clearing SOCKWQ_ASYNC_NOSPACE
openvpn-914 [001] 155.750263: bprint: xs_write_space: sk=0xffff88810a8f0a00 SOCKWQ_ASYNC_NOSPACE was clear
>> For example, why check that flag rather than just waking
>> up xprt->sending unconditionally?
>
> The socket code calls ->write_space() in all sorts of situations, so we
> need to distinguish between the cases where we are actually waiting for
> buffer memory, and the situations where we are not. Otherwise, we'd be
> calling xs_run_error_worker() all the time.
On my (admittedly limited) workloads, sk_stream_is_writeable()
does a good job of avoiding spurious wake-ups. However, to be
absolutely certain of our wake-up accounting, using a flag that
is local to the rpc_xprt and not overloaded might be wise?
--
Chuck Lever