From: Matthieu Baerts <matttbe@kernel.org>
To: Eric Dumazet <edumazet@google.com>,
"David S . Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Cc: Simon Horman <horms@kernel.org>,
Neal Cardwell <ncardwell@google.com>,
Willem de Bruijn <willemb@google.com>,
Kuniyuki Iwashima <kuniyu@google.com>,
Mat Martineau <martineau@kernel.org>,
Geliang Tang <geliang@kernel.org>,
netdev@vger.kernel.org, eric.dumazet@gmail.com
Subject: Re: [PATCH v2 net 2/3] tcp: add newval parameter to tcp_rcvbuf_grow()
Date: Mon, 27 Oct 2025 15:50:00 +0100 [thread overview]
Message-ID: <d4d71883-d249-4fbd-a703-930e62a16b96@kernel.org> (raw)
In-Reply-To: <20251027073809.2112498-3-edumazet@google.com>
Hi Eric,
On 27/10/2025 08:38, Eric Dumazet wrote:
> This patch has no functional change, and prepares the following one.
>
> tcp_rcvbuf_grow() will need to have access to tp->rcvq_space.space
> old and new values.
>
> Change mptcp_rcvbuf_grow() in a similar way.
Thank you for the v2, and for having adapted MPTCP as well.
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> include/net/tcp.h | 2 +-
> net/ipv4/tcp_input.c | 15 ++++++++-------
> net/mptcp/protocol.c | 16 ++++++++--------
> 3 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 5ca230ed526ae02711e8d2a409b91664b73390f2..ab20f549b8f9143671b75ed0a3f87d64b9e73583 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -370,7 +370,7 @@ void tcp_delack_timer_handler(struct sock *sk);
> int tcp_ioctl(struct sock *sk, int cmd, int *karg);
> enum skb_drop_reason tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb);
> void tcp_rcv_established(struct sock *sk, struct sk_buff *skb);
> -void tcp_rcvbuf_grow(struct sock *sk);
> +void tcp_rcvbuf_grow(struct sock *sk, u32 newval);
> void tcp_rcv_space_adjust(struct sock *sk);
> int tcp_twsk_unique(struct sock *sk, struct sock *sktw, void *twp);
> void tcp_twsk_destructor(struct sock *sk);
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 31ea5af49f2dc8a6f95f3f8c24065369765b8987..600b733e7fb554c36178e432996ecc7d4439268a 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -891,18 +891,21 @@ static inline void tcp_rcv_rtt_measure_ts(struct sock *sk,
> }
> }
>
> -void tcp_rcvbuf_grow(struct sock *sk)
> +void tcp_rcvbuf_grow(struct sock *sk, u32 newval)
> {
> const struct net *net = sock_net(sk);
> struct tcp_sock *tp = tcp_sk(sk);
> - int rcvwin, rcvbuf, cap;
> + u32 rcvwin, rcvbuf, cap, oldval;
> +
> + oldval = tp->rcvq_space.space;
Even if the series as a whole is OK, NIPA (and the MPTCP CI) are
complaining about this line, because in this patch, 'oldval' is set but
not used. It is used in the next patch.
I guess we want to fix this to prevent issues with 'git bisect'. If yes,
do you mind moving the declaration to the next patch please?
> + tp->rcvq_space.space = newval;
>
> if (!READ_ONCE(net->ipv4.sysctl_tcp_moderate_rcvbuf) ||
> (sk->sk_userlocks & SOCK_RCVBUF_LOCK))
> return;
>
> /* slow start: allow the sender to double its rate. */
> - rcvwin = tp->rcvq_space.space << 1;
> + rcvwin = newval << 1;
>
> if (!RB_EMPTY_ROOT(&tp->out_of_order_queue))
> rcvwin += TCP_SKB_CB(tp->ooo_last_skb)->end_seq - tp->rcv_nxt;
> @@ -943,9 +946,7 @@ void tcp_rcv_space_adjust(struct sock *sk)
>
> trace_tcp_rcvbuf_grow(sk, time);
>
> - tp->rcvq_space.space = copied;
> -
> - tcp_rcvbuf_grow(sk);
> + tcp_rcvbuf_grow(sk, copied);
>
> new_measure:
> tp->rcvq_space.seq = tp->copied_seq;
> @@ -5270,7 +5271,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
> }
> /* do not grow rcvbuf for not-yet-accepted or orphaned sockets. */
> if (sk->sk_socket)
> - tcp_rcvbuf_grow(sk);
> + tcp_rcvbuf_grow(sk, tp->rcvq_space.space);
> }
>
> static int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb,
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0292162a14eedffde166cc2a2d4eaa7c3aa6760d..f12c5806f1c861ca74d2375914073abc37c940d6 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -194,17 +194,19 @@ static bool mptcp_ooo_try_coalesce(struct mptcp_sock *msk, struct sk_buff *to,
> * - mptcp does not maintain a msk-level window clamp
> * - returns true when the receive buffer is actually updated
> */
> -static bool mptcp_rcvbuf_grow(struct sock *sk)
> +static bool mptcp_rcvbuf_grow(struct sock *sk, u32 newval)
> {
> struct mptcp_sock *msk = mptcp_sk(sk);
> const struct net *net = sock_net(sk);
> - int rcvwin, rcvbuf, cap;
> + u32 rcvwin, rcvbuf, cap, oldval;
>
> + oldval = msk->rcvq_space.space;
Same here in MPTCP:
> net/mptcp/protocol.c: In function 'mptcp_rcvbuf_grow':
> net/mptcp/protocol.c:201:34: error: variable 'oldval' set but not used [-Werror=unused-but-set-variable]
> 201 | u32 rcvwin, rcvbuf, cap, oldval;
> | ^~~~~~
Apart from this small detail, the rest looks good to me.
> + msk->rcvq_space.space = newval;
> if (!READ_ONCE(net->ipv4.sysctl_tcp_moderate_rcvbuf) ||
> (sk->sk_userlocks & SOCK_RCVBUF_LOCK))
> return false;
>
> - rcvwin = msk->rcvq_space.space << 1;
> + rcvwin = newval << 1;
>
> if (!RB_EMPTY_ROOT(&msk->out_of_order_queue))
> rcvwin += MPTCP_SKB_CB(msk->ooo_last_skb)->end_seq - msk->ack_seq;
> @@ -334,7 +336,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
> skb_set_owner_r(skb, sk);
> /* do not grow rcvbuf for not-yet-accepted or orphaned sockets. */
> if (sk->sk_socket)
> - mptcp_rcvbuf_grow(sk);
> + mptcp_rcvbuf_grow(sk, msk->rcvq_space.space);
> }
>
> static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
> @@ -2049,8 +2051,7 @@ static void mptcp_rcv_space_adjust(struct mptcp_sock *msk, int copied)
> if (msk->rcvq_space.copied <= msk->rcvq_space.space)
> goto new_measure;
>
> - msk->rcvq_space.space = msk->rcvq_space.copied;
> - if (mptcp_rcvbuf_grow(sk)) {
> + if (mptcp_rcvbuf_grow(sk, msk->rcvq_space.copied)) {
>
> /* Make subflows follow along. If we do not do this, we
> * get drops at subflow level if skbs can't be moved to
> @@ -2063,8 +2064,7 @@ static void mptcp_rcv_space_adjust(struct mptcp_sock *msk, int copied)
>
> ssk = mptcp_subflow_tcp_sock(subflow);
> slow = lock_sock_fast(ssk);
> - tcp_sk(ssk)->rcvq_space.space = msk->rcvq_space.copied;
> - tcp_rcvbuf_grow(ssk);
> + tcp_rcvbuf_grow(ssk, msk->rcvq_space.copied);
> unlock_sock_fast(ssk, slow);
> }
> }
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
next prev parent reply other threads:[~2025-10-27 14:50 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-27 7:38 [PATCH v2 net 0/3] tcp: fix receive autotune again Eric Dumazet
2025-10-27 7:38 ` [PATCH v2 net 1/3] trace: tcp: add three metrics to trace_tcp_rcvbuf_grow() Eric Dumazet
2025-10-27 7:38 ` [PATCH v2 net 2/3] tcp: add newval parameter to tcp_rcvbuf_grow() Eric Dumazet
2025-10-27 14:50 ` Matthieu Baerts [this message]
2025-10-27 15:37 ` Eric Dumazet
2025-10-27 17:22 ` Matthieu Baerts
2025-10-27 7:38 ` [PATCH v2 net 3/3] tcp: fix too slow tcp_rcvbuf_grow() action Eric Dumazet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d4d71883-d249-4fbd-a703-930e62a16b96@kernel.org \
--to=matttbe@kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=eric.dumazet@gmail.com \
--cc=geliang@kernel.org \
--cc=horms@kernel.org \
--cc=kuba@kernel.org \
--cc=kuniyu@google.com \
--cc=martineau@kernel.org \
--cc=ncardwell@google.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=willemb@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).