RE: [PATCH net 2/4] tls: Fix write space handling

[Vakul Garg <vakul.garg@nxp.com>]

> -----Original Message-----
> From: Boris Pismenny <borisp@mellanox.com>
> Sent: Monday, March 11, 2019 9:29 PM
> To: Vakul Garg <vakul.garg@nxp.com>; Aviad Yehezkel
> <aviadye@mellanox.com>; davejwatson@fb.com;
> john.fastabend@gmail.com; daniel@iogearbox.net; netdev@vger.kernel.org
> Cc: Eran Ben Elisha <eranbe@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);
> >>>>    }
> >>>>