[PATCH v2 0/2] Drop redundant tcp_rate_check_app_limited calls

[PATCH v2 0/2] Drop redundant tcp_rate_check_app_limited calls

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

These two patches drop the redundant tcp_rate_check_app_limited() calls
before tcp_sendmsg_locked() in tcp_bpf_push() and tls_push_sg(), since
tcp_rate_check_app_limited() is already called inside tcp_sendmsg_locked().

Note:

The first patch was originally part of my ongoing "MPTCP sockmap support"
series [1] (patch 5). Matthieu suggested converting it to a fix and sending
it directly to netdev. Removing the redundant tcp_rate_check_app_limited()
call benefits my subsequent MPTCP work: in patch 3 of that series, I
implement an MPTCP-specific mptcp_rate_check_app_limited() function and
call it in mptcp_sendmsg_locked(). This allows me to reuse tcp_bpf_push()
by simply replacing tcp_sendmsg_locked() with

		sk->sk_socket->ops->sendmsg_locked(),

without carrying protocol-specific assumptions.

The second patch similarly benefits my ongoing "MPTCP KTLS support" work
[2]. In patch 9 of that series, I had defined a check_app_limited()
interface in struct tls_prot_ops, but this interface is unnecessary if we
can drop this redundant call.

v2:
 - drop the "Fixes" tags and update the prefixes and commit logs as Jiayuan
   suggested.

v1:
 - https://patchwork.kernel.org/project/netdevbpf/cover/cover.1789368148.git.tanggeliang@kylinos.cn/

[1]
MPTCP sockmap support
https://lore.kernel.org/mptcp/b5f9e8d7-b738-1df6-3b5e-1d54cbbc663c@gmail.com/T/#t

[2]
MPTCP KTLS support
https://lore.kernel.org/netdev/cover.1782123118.git.tanggeliang@kylinos.cn/

Geliang Tang (2):
  bpf: drop duplicate check_app_limited in tcp_bpf_push
  tls: drop duplicate check_app_limited in tls_push_sg

 net/ipv4/tcp_bpf.c | 1 -
 net/tls/tls_main.c | 2 --
 2 files changed, 3 deletions(-)

-- 
2.53.0

[PATCH bpf-next v2 1/2] bpf: drop duplicate check_app_limited in tcp_bpf_push

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Before commit c5c37af6ecad9 ("tcp: Convert do_tcp_sendpages() to use
MSG_SPLICE_PAGES"), do_tcp_sendpages() did not call
tcp_rate_check_app_limited() internally, so callers needed an explicit
tcp_rate_check_app_limited() to cover it. That commit replaced
do_tcp_sendpages() with direct tcp_sendmsg_locked() calls, which always
provide the check internally. The outer call became redundant but was
left in place.

The site changed here, tcp_bpf_push(), is a MSG_SPLICE_PAGES loop that
holds the socket lock and only iterates when size > 0; tcp_sendmsg_locked()
is invoked on every iteration with state identical to what the outer call
sees, so dropping the outer call is safe and behavior-preserving.

A potential benefit of this change is that it facilitates future reuse of
tcp_bpf_push() for sockmap support in protocols beyond TCP, such as MPTCP.
Since tcp_rate_check_app_limited() is TCP-specific while sendmsg_locked()
is a generic interface in struct proto_ops, this change allows us to switch
to different protocols via sk->sk_socket->ops->sendmsg_locked() without
carrying protocol-specific assumptions.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/ipv4/tcp_bpf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index 2e234d155b5e..d5fcf3ce4861 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -108,7 +108,6 @@ static int tcp_bpf_push(struct sock *sk, struct sk_msg *msg, u32 apply_bytes,
 		off  = sge->offset;
 		page = sg_page(sge);
 
-		tcp_rate_check_app_limited(sk);
 retry:
 		msghdr.msg_flags = flags | MSG_SPLICE_PAGES;
 		has_tx_ulp = tls_sw_has_ctx_tx(sk);
-- 
2.53.0

[PATCH net-next v2 2/2] tls: drop duplicate check_app_limited in tls_push_sg

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Before commit c5c37af6ecad9 ("tcp: Convert do_tcp_sendpages() to use
MSG_SPLICE_PAGES"), do_tcp_sendpages() did not call
tcp_rate_check_app_limited() internally, so callers needed an explicit
tcp_rate_check_app_limited() to cover it. That commit replaced
do_tcp_sendpages() with direct tcp_sendmsg_locked() calls, which always
provide the check internally. The outer call became redundant but was
left in place.

The site changed here, tls_push_sg(), is a MSG_SPLICE_PAGES loop that holds
the socket lock and only iterates when size > 0; tcp_sendmsg_locked() is
invoked on every iteration with state identical to what the outer call
sees, so dropping the outer call is safe and behavior-preserving.

A potential benefit of this change is that it facilitates future reuse of
tls_push_sg() for KTLS support in protocols beyond TCP, such as MPTCP.
Since tcp_rate_check_app_limited() is TCP-specific while sendmsg_locked()
is a generic interface in struct proto_ops, this change allows us to switch
to different protocols via sk->sk_socket->ops->sendmsg_locked() without
carrying protocol-specific assumptions.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/tls/tls_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index fbb274287aa5..8bdb78718a8d 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -185,8 +185,6 @@ int tls_push_sg(struct sock *sk,
 
 	ctx->splicing_pages = true;
 	while (1) {
-		/* is sending application-limited? */
-		tcp_rate_check_app_limited(sk);
 		p = sg_page(sg);
 retry:
 		bvec_set_page(&bvec, p, size, offset);
-- 
2.53.0

Re: [PATCH bpf-next v2 1/2] bpf: drop duplicate check_app_limited in tcp_bpf_push

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3000 bytes --]

> Before commit c5c37af6ecad9 ("tcp: Convert do_tcp_sendpages() to use
> MSG_SPLICE_PAGES"), do_tcp_sendpages() did not call
> tcp_rate_check_app_limited() internally, so callers needed an explicit
> tcp_rate_check_app_limited() to cover it. That commit replaced
> do_tcp_sendpages() with direct tcp_sendmsg_locked() calls, which always
> provide the check internally. The outer call became redundant but was
> left in place.
>
> The site changed here, tcp_bpf_push(), is a MSG_SPLICE_PAGES loop that
> holds the socket lock and only iterates when size > 0; tcp_sendmsg_locked()
> is invoked on every iteration with state identical to what the outer call
> sees, so dropping the outer call is safe and behavior-preserving.
>
> A potential benefit of this change is that it facilitates future reuse of
> tcp_bpf_push() for sockmap support in protocols beyond TCP, such as MPTCP.
> Since tcp_rate_check_app_limited() is TCP-specific while sendmsg_locked()
> is a generic interface in struct proto_ops, this change allows us to switch
> to different protocols via sk->sk_socket->ops->sendmsg_locked() without
> carrying protocol-specific assumptions.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

This isn't a bug, but the state-equivalence argument here looks right.
Would it be worth tightening "which always provide the check internally"
to something like "which performs the check on every path that queues
data"?

Looking at tcp_sendmsg_locked(), it reaches tcp_rate_check_app_limited()
only after the MSG_ZEROCOPY and MSG_FASTOPEN early returns:

net/ipv4/tcp.c:tcp_sendmsg_locked() {
    ...
		uarg = msg_zerocopy_realloc(...);
		if (!uarg) {
			err = -ENOBUFS;
			goto out_err;
		}
    ...
	err = tcp_sendmsg_fastopen(sk, msg, &copied_syn, size, uarg);
	if (err == -EINPROGRESS && copied_syn > 0)
		goto out;
	else if (err)
		goto out_err;
	}

	timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);

	tcp_rate_check_app_limited(sk);  /* is sending application-limited? */
    ...
}

Those branches are reachable from tcp_bpf_push(), since tcp_bpf_sendmsg()
forwards the user's flags almost verbatim and tcp_bpf_push() passes them
on as `msghdr.msg_flags = flags | MSG_SPLICE_PAGES`, so MSG_ZEROCOPY and
MSG_FASTOPEN both reach tcp_sendmsg_locked(). On those paths the old code
updated tp->app_limited and the new code does not. Every one of them
returns without queueing SPLICE_PAGES data, so there is no functional
consequence -- but "always" is doing load-bearing work in the
justification, and a narrower phrasing would match the code.

Also, is the "only iterates when size > 0" clause needed? The internal
check at net/ipv4/tcp.c:1190 is called unconditionally regardless of
size, so the size > 0 property is not what makes the removal safe.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34964239423

Re: [PATCH v2 0/2] Drop redundant tcp_rate_check_app_limited calls

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Unstable: 1 failed test(s): selftest_simult_flows ⚠️ 
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Unstable: 1 failed test(s): selftest_mptcp_join ⚠️ 
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/34964839210

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/80478bdef8a8
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1165425


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)