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

Sabrina Dubroca
2026-03-30T14:43:53Z

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