Re: [PATCH PATCH net-next v4 5/8] tls: Suppress spurious saved_data_ready on all receive paths

[Sabrina Dubroca <sd@queasysnail.net>]

2026-03-17, 11:04:18 -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Each record release via tls_strp_msg_done() triggers
> tls_strp_check_rcv(), which calls tls_rx_msg_ready() and
> fires saved_data_ready(). During a multi-record receive,
> the first N-1 wakeups are pure overhead: the caller is
> already running and will pick up subsequent records on
> the next loop iteration. The same waste occurs on the
> recvmsg and splice_read paths.

nit: splice_read is less of a problem since it doesn't loop over
records?


[...]
> +void tls_strp_check_rcv_quiet(struct tls_strparser *strp)
> +{
> +	if (unlikely(strp->stopped) || strp->msg_ready)
> +		return;
> +
> +	if (tls_strp_read_sock(strp) == -ENOMEM)
> +		queue_work(tls_strp_wq, &strp->work);
> +}

c/p of tls_strp_check_rcv isn't nice. Add a 'bool wake_up' argument
instead? [but see the comment about recvmsg]


>  void tls_strp_check_rcv(struct tls_strparser *strp)
>  {
>  	if (unlikely(strp->stopped) || strp->msg_ready)
> @@ -551,6 +566,8 @@ void tls_strp_check_rcv(struct tls_strparser *strp)
>  
>  	if (tls_strp_read_sock(strp) == -ENOMEM)
>  		queue_work(tls_strp_wq, &strp->work);
> +	else if (strp->msg_ready)
> +		tls_rx_msg_ready(strp);

Since that's now the only caller of tls_rx_msg_ready, and all that
does is call saved_data_ready, inline it here?

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 07f4a3d1a6f854acc7762608cc7741b3de95c195..381a723b6cacc669e333752af34f051f296d6f52 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1384,7 +1384,10 @@ tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock,
>  			return ret;
>  
>  		if (!skb_queue_empty(&sk->sk_receive_queue)) {
> -			tls_strp_check_rcv(&ctx->strp);
> +			/* tls_strp_check_rcv() is called at each receive
> +			 * path's exit before the socket lock is released.
> +			 */

I'm not convinced this comment will make sense to someone reading the
code outside of reviewing this series.

> +			tls_strp_check_rcv_quiet(&ctx->strp);
>  			if (tls_strp_msg_ready(ctx))
>  				break;
>  		}
> @@ -1867,9 +1870,9 @@ static int tls_record_content_type(struct msghdr *msg, struct tls_msg *tlm,
>  	return 1;
>  }
>  
> -static void tls_rx_rec_done(struct tls_sw_context_rx *ctx)
> +static void tls_rx_rec_release(struct tls_sw_context_rx *ctx)
>  {
> -	tls_strp_msg_done(&ctx->strp);
> +	tls_strp_msg_release(&ctx->strp);
>  }
>  
>  /* This function traverses the rx_list in tls receive context to copies the
> @@ -2150,7 +2153,7 @@ int tls_sw_recvmsg(struct sock *sk,
>  		err = tls_record_content_type(msg, tls_msg(darg.skb), &control);
>  		if (err <= 0) {
>  			DEBUG_NET_WARN_ON_ONCE(darg.zc);
> -			tls_rx_rec_done(ctx);
> +			tls_rx_rec_release(ctx);
>  put_on_rx_list_err:
>  			__skb_queue_tail(&ctx->rx_list, darg.skb);
>  			goto recv_end;
> @@ -2164,7 +2167,8 @@ int tls_sw_recvmsg(struct sock *sk,
>  		/* TLS 1.3 may have updated the length by more than overhead */
>  		rxm = strp_msg(darg.skb);
>  		chunk = rxm->full_len;
> -		tls_rx_rec_done(ctx);
> +		tls_rx_rec_release(ctx);
> +		tls_strp_check_rcv_quiet(&ctx->strp);

This one worries me: if tls_strp_check_rcv_quiet() sets msg_ready=1
without calling saved_data_ready. If we break out of the loop after
this, the final tls_strp_check_rcv() just before returning from
tls_sw_recvmsg() will do:

void tls_strp_check_rcv(struct tls_strparser *strp, bool wake_up)
{
	if (unlikely(strp->stopped) || strp->msg_ready)
		return;
[...]

and not call saved_data_ready?

-- 
Sabrina