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

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 29 Apr 2026 17:48:11 -0400 Chuck Lever wrote:
> -void tls_strp_check_rcv(struct tls_strparser *strp)
> +/**
> + * tls_strp_check_rcv - parse queued data and optionally notify
> + * @strp: TLS stream parser instance
> + * @wake: if true, fire consumer notification when a record is newly
> + *        parsed by this call
> + *
> + * Returns immediately when a record is already ready; the wake fires
> + * only on transitions from no-record to record-ready. Callers that
> + * need to notify a waiter about a record parsed by another path
> + * should invoke tls_rx_msg_ready() directly.
> + */

nit/reminder: no kdoc

> +void tls_strp_check_rcv(struct tls_strparser *strp, bool wake)

I wonder if it'd make sense to s/wake/announce/ for consistency?

>  {
>  	if (unlikely(strp->stopped) || strp->msg_ready)
>  		return;
>  
>  	if (tls_strp_read_sock(strp) == -ENOMEM)
>  		queue_work(tls_strp_wq, &strp->work);
> +	else if (wake && strp->msg_ready)
> +		tls_rx_msg_ready(strp);
>  }

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index c58d3b0b0a8a..cbb068266bab 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1383,7 +1383,11 @@ 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);
> +			/* Defer notification to the exit point;
> +			 * this thread will consume the record
> +			 * directly.

this line could be un-wrapped AFAICT

> +			 */
> +			tls_strp_check_rcv(&ctx->strp, false);
>  			if (tls_strp_msg_ready(ctx))
>  				break;
>  		}
> @@ -1869,9 +1873,17 @@ 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)
> +/* Parse any data left in the lower socket and hand off a single
> + * notification to the next reader. tls_rx_msg_ready() is a no-op
> + * when the current record has already been announced, so paths
> + * that drained ctx->rx_list without touching the strparser do
> + * not re-fire saved_data_ready() for a record BH or the worker
> + * already announced.
> + */
> +static void tls_rx_handoff(struct tls_sw_context_rx *ctx)
>  {
> -	tls_strp_msg_done(&ctx->strp);
> +	tls_strp_check_rcv(&ctx->strp, false);
> +	tls_rx_msg_ready(&ctx->strp);
>  }
>  
>  /* This function traverses the rx_list in tls receive context to copies the
> @@ -2152,7 +2164,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_strp_msg_release(&ctx->strp);
>  put_on_rx_list_err:
>  			__skb_queue_tail(&ctx->rx_list, darg.skb);
>  			goto recv_end;
> @@ -2166,7 +2178,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_strp_msg_release(&ctx->strp);
> +		tls_strp_check_rcv(&ctx->strp, false);
>  
>  		if (!darg.zc) {
>  			bool partially_consumed = chunk > len;
> @@ -2260,6 +2273,7 @@ int tls_sw_recvmsg(struct sock *sk,
>  	copied += decrypted;
>  
>  end:
> +	tls_rx_handoff(ctx);
>  	tls_rx_reader_unlock(sk, ctx);
>  	if (psock)
>  		sk_psock_put(sk, psock);
> @@ -2300,7 +2314,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  		if (err < 0)
>  			goto splice_read_end;
>  
> -		tls_rx_rec_done(ctx);
> +		tls_strp_msg_release(&ctx->strp);
>  		skb = darg.skb;
>  	}
>  
> @@ -2327,6 +2341,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  	consume_skb(skb);
>  
>  splice_read_end:
> +	tls_rx_handoff(ctx);

I don't get why the tls_rx_handoff() thing exists, TBH
Maybe it made sense in the context of the code which is no longer part
of the series. But without it it looks pretty odd. Fells like you should
simply be suppressing notifications when tls_strp_check_rcv() is called
by tls_rx_rec_done() and then tls_rx_reader_release() should make sure
it flushes the ->data_ready if it's exiting on a read && !announced
condition. In rcvmsg() path you're now calling tls_strp_check_rcv()
twice.

>  	tls_rx_reader_unlock(sk, ctx);
>  	return copied ? : err;
>  
> @@ -2392,7 +2407,7 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
>  			tlm = tls_msg(skb);
>  			decrypted += rxm->full_len;
>  
> -			tls_rx_rec_done(ctx);
> +			tls_strp_msg_release(&ctx->strp);
>  		}
>  
>  		/* read_sock does not support reading control messages */
> @@ -2420,6 +2435,7 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
>  	}
>  
>  read_sock_end:
> +	tls_rx_handoff(ctx);
>  	tls_rx_reader_release(sk, ctx);
>  	return copied ? : err;
>  
> @@ -2504,10 +2520,18 @@ int tls_rx_msg_size(struct tls_strparser *strp, struct sk_buff *skb)
>  	return ret;
>  }
>  
> +/* Fire saved_data_ready() at most once per parsed record.
> + * msg_announced is cleared by tls_strp_msg_release() when the
> + * current record is consumed, arming the next announcement.
> + */
>  void tls_rx_msg_ready(struct tls_strparser *strp)

Given the change to the semantics maybe this should be named
tls_rx_msg_maybe_announce() or _flush_announce() now ?

>  {
>  	struct tls_sw_context_rx *ctx;
>  
> +	if (!READ_ONCE(strp->msg_ready) || strp->msg_announced)
> +		return;
> +	strp->msg_announced = 1;
> +
>  	ctx = container_of(strp, struct tls_sw_context_rx, strp);
>  	ctx->saved_data_ready(strp->sk);
>  }
>