> -----Original Message-----
> From: Boris Pismenny <bor...@mellanox.com>
> Sent: Monday, March 11, 2019 9:29 PM
> To: Vakul Garg <vakul.g...@nxp.com>; Aviad Yehezkel
> <avia...@mellanox.com>; davejwat...@fb.com;
> john.fastab...@gmail.com; dan...@iogearbox.net; netdev@vger.kernel.org
> Cc: Eran Ben Elisha <era...@mellanox.com>
> Subject: Re: [PATCH net 2/4] tls: Fix write space handling
>
> >>>> a/net/tls/tls_main.c b/net/tls/tls_main.c index
> >>>> 7e05af75536d..11c1980a75cb 100644
> >>>> --- a/net/tls/tls_main.c
> >>>> +++ b/net/tls/tls_main.c
> >>>> @@ -212,7 +212,7 @@ int tls_push_partial_record(struct sock *sk,
> >>>> struct tls_context *ctx,
> >>>> static void tls_write_space(struct sock *sk)
> >>>> {
> >>>> struct tls_context *ctx = tls_get_ctx(sk);
> >>>> - struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
> >>>> + int rc;
> >>>>
> >>>> /* If in_tcp_sendpages call lower protocol write space handler
> >>>> * to ensure we wake up any waiting operations there. For
> >>>> example @@ -223,14 +223,15 @@ static void tls_write_space(struct
> sock *sk)
> >>>> return;
> >>>> }
> >>>>
> >>>> - /* Schedule the transmission if tx list is ready */
> >>>> - if (is_tx_ready(tx_ctx) && !sk->sk_write_pending) {
> >>>> - /* Schedule the transmission */
> >>>> - if (!test_and_set_bit(BIT_TX_SCHEDULED, &tx_ctx-
> >>>>> tx_bitmask))
> >>>> - schedule_delayed_work(&tx_ctx->tx_work.work, 0);
> >>>> - }
> >>>> +#ifdef CONFIG_TLS_DEVICE
> >>>> + if (ctx->tx_conf == TLS_HW)
> >>>> + rc = tls_device_write_space(sk, ctx);
> >>>> + else
> >>>> +#endif
> >>>> + rc = tls_sw_write_space(sk, ctx);
> >>>>
> >>>> - ctx->sk_write_space(sk);
> >>>> + if (!rc)
> >>>
> >>> Why do we need to check 'rc'?
> >>>
> >>> If it is required, then ' ctx->sk_write_space(sk)' can move to
> >>> tls_device_write_space() since tls_sw_write_space() always returns '0'.
> >>>
> >>
> >> It is not necessary in the software code path due to the delayed work
> >> that is there. But, we need in the device flow. I'll move it there.
> >>
> >
> > Removal of ctx->sk_write_space(sk) has broken software code flow.
> > The ktls send stops and user space application waits infinitely.
> > When tls_write_space() gets invoked tcp has been able to transmit some
> data.
> > Shouldn't we unconditionally call ctx->sk_write_space() in order to
> > inform user space application about availability of buffer space?
> >
> > Please advise. I would submit the patch.
>
> AFAIU, the code in the software path calls ctx->sk_write_space in its
> schedule work which eventually calls tls_push_sg. Since this flow is
> asynchronous, I thought it was best to postpone the notification and let the
> work handle it.
>
As per my code reading, sk->sk_write_space() is called from tcp code itself
after
transmitting socket data.
sk->sk_write_space() is mapped to tls_write_space() which then internally calls
tls_sw_write_space() or tls_device_write_space(). Inside tls_sw_write_space(),
we may not schedule delayed work, but still we need to inform user space about
availability of buffer space by way for invoking of ctx->sk_write_space().
On the other hand, calling ctx->sk_write_space() from tls_push_sg() seems
superfluous
& should be removed. For both device and software flows, ctx->sk_write_space()
should
be invoked like before from tls_write_space().
> >
> >>
> >>>> + ctx->sk_write_space(sk);
> >>>> }
> >>>>
