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

2026-03-28, 11:17:11 -0400, Chuck Lever wrote:
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 5fdd43a55f1e..8fb2f2a93846 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1373,7 +1373,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.
> +			 */

That's a really nice improvement over the comment you had here
before. Thanks.

> +			tls_strp_check_rcv(&ctx->strp, false);
>  			if (tls_strp_msg_ready(ctx))
>  				break;
>  		}

[...]
> @@ -2142,7 +2146,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;
> @@ -2156,7 +2160,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(&ctx->strp, false);

Not strictly an objection against your patch, but after those changes,
calling tls_strp_check_rcv() at this point in tls_sw_recvmsg() is
starting to look like a leftover from the transition from generic strp
to custom strp. We're not going to do anything with the next record
until we call tls_rx_rec_wait(), so it seems it would fit better
before the loop in tls_rx_rec_wait().


[...]
> @@ -2290,7 +2296,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_rx_rec_release(ctx);
>  		skb = darg.skb;
>  	}
>  
> @@ -2317,6 +2323,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  	consume_skb(skb);
>  
>  splice_read_end:
> +	tls_strp_check_rcv(&ctx->strp, true);

This is in effect adding a

if (strp->msg_ready)
    tls_rx_msg_ready(strp);

[a bit more than that]
in case we dequeue from the rx_list but don't use the record (or only
part of it).

I wonder if that should be seen as a problem (another spurious wakeup)
or an improvement (wake up because there's data ready)? Or if we
should wake up anyway if we exit with rx_list non-empty, regardless or
the parser's state, since there is data to read (tls_sk_poll looks at
both rx_list and msg_ready).

[this applies to all 3 RX functions, but this one is the simplest,
which makes it more visible]

>  	tls_rx_reader_unlock(sk, ctx);
>  	return copied ? : err;
>  
> @@ -2382,7 +2389,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_rx_rec_release(ctx);

With this, there's no tls_strp_check_rcv() call inside the
tls_sw_read_sock() loop, so in the next iteration, tls_rx_rec_wait()
will have to go for at least one round of its own loop. [this points
back to the recvmsg comment above, and a bit to the "cold path"
discussion we had earlier]


(Sorry that I'm only thinking about all this at v7...)

>  		}
>  
>  		/* read_sock does not support reading control messages */
> @@ -2412,6 +2419,7 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
>  	}
>  
>  read_sock_end:
> +	tls_strp_check_rcv(&ctx->strp, true);
>  	tls_rx_reader_release(sk, ctx);
>  	return copied ? : err;


-- 
Sabrina