[v3] bpf: drop duplicate check_app_limited in tcp_bpf_push

[PATCH bpf-next v3] bpf: drop duplicate check_app_limited in tcp_bpf_push

Posted by Geliang Tang 1 week, 2 days ago

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 perform
the check on every path that queues data. The outer call became redundant
but was left in place.

The site changed here, tcp_bpf_push(), holds the socket lock and invokes
tcp_sendmsg_locked() on every iteration. The early-return paths in
tcp_sendmsg_locked() that skip tcp_rate_check_app_limited() -- the
MSG_ZEROCOPY allocation failure and MSG_FASTOPEN branches -- return
without queueing any MSG_SPLICE_PAGES data, so there is no functional
consequence from omitting the outer check.

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>
---
Note:

This 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.

v3:
 - send this patch separately to bpf as Jakub suggested.
 - update the commit log as bot+bpf-ci suggested.

v2:
 - drop the "Fixes" tags and update the prefixes and commit logs as Jiayuan
   suggested.
 - https://patchwork.kernel.org/project/netdevbpf/cover/cover.1789469930.git.tanggeliang@kylinos.cn/

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
---
 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

Re: [PATCH bpf-next v3] bpf: drop duplicate check_app_limited in tcp_bpf_push

Posted by MPTCP CI 1 week, 2 days ago

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): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- 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/35203046251

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


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)

Re: [PATCH bpf-next v3] bpf: drop duplicate check_app_limited in tcp_bpf_push

Posted by bot+bpf-ci@kernel.org 1 week, 2 days ago

> bpf: drop duplicate check_app_limited in tcp_bpf_push
>
> 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 perform
> the check on every path that queues data. The outer call became redundant
> but was left in place.

This isn't a bug, but would it be clearer to cite ebf2e8860eea ("tcp_bpf:
Inline do_tcp_sendpages as it's now a wrapper around tcp_sendmsg") as the
commit that left the outer tcp_rate_check_app_limited() in tcp_bpf_push(),
with c5c37af6ecad9 mentioned as the commit that moved the check inside
do_tcp_sendpages()?

Looking at the git history, c5c37af6ecad9 only touched net/ipv4/tcp.c and
turned do_tcp_sendpages() into a wrapper that calls tcp_sendmsg_locked().
The direct tcp_sendmsg_locked() call in tcp_bpf_push() was introduced by
ebf2e8860eea, which kept the outer tcp_rate_check_app_limited(sk) while
inlining the wrapper.


---
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/35203307255

Re: [PATCH bpf-next v3] bpf: drop duplicate check_app_limited in tcp_bpf_push

Posted by Jiayuan Chen 4 days, 23 hours ago

On 9/17/26 5:45 PM, bot+bpf-ci@kernel.org wrote:
>> bpf: drop duplicate check_app_limited in tcp_bpf_push
>>
>> 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 perform
>> the check on every path that queues data. The outer call became redundant
>> but was left in place.
> This isn't a bug, but would it be clearer to cite ebf2e8860eea ("tcp_bpf:
> Inline do_tcp_sendpages as it's now a wrapper around tcp_sendmsg") as the
> commit that left the outer tcp_rate_check_app_limited() in tcp_bpf_push(),
> with c5c37af6ecad9 mentioned as the commit that moved the check inside
> do_tcp_sendpages()?
>
> Looking at the git history, c5c37af6ecad9 only touched net/ipv4/tcp.c and
> turned do_tcp_sendpages() into a wrapper that calls tcp_sendmsg_locked().
> The direct tcp_sendmsg_locked() call in tcp_bpf_push() was introduced by
> ebf2e8860eea, which kept the outer tcp_rate_check_app_limited(sk) while
> inlining the wrapper.
>

CI is right. ebf2e8860eea should be accurate.


> ---
> 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/35203307255

Re: [PATCH bpf-next v3] bpf: drop duplicate check_app_limited in tcp_bpf_push

Posted by Geliang Tang 3 days, 23 hours ago

Hi Jiayuan,

On Mon, 2026-09-21 at 19:46 +0800, Jiayuan Chen wrote:
> 
> On 9/17/26 5:45 PM, bot+bpf-ci@kernel.org wrote:
> > > bpf: drop duplicate check_app_limited in tcp_bpf_push
> > > 
> > > 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
> > > perform
> > > the check on every path that queues data. The outer call became
> > > redundant
> > > but was left in place.
> > This isn't a bug, but would it be clearer to cite ebf2e8860eea
> > ("tcp_bpf:
> > Inline do_tcp_sendpages as it's now a wrapper around tcp_sendmsg")
> > as the
> > commit that left the outer tcp_rate_check_app_limited() in
> > tcp_bpf_push(),
> > with c5c37af6ecad9 mentioned as the commit that moved the check
> > inside
> > do_tcp_sendpages()?
> > 
> > Looking at the git history, c5c37af6ecad9 only touched
> > net/ipv4/tcp.c and
> > turned do_tcp_sendpages() into a wrapper that calls
> > tcp_sendmsg_locked().
> > The direct tcp_sendmsg_locked() call in tcp_bpf_push() was
> > introduced by
> > ebf2e8860eea, which kept the outer tcp_rate_check_app_limited(sk)
> > while
> > inlining the wrapper.
> > 
> 
> CI is right. ebf2e8860eea should be accurate.

You're right, thanks for the careful review. ebf2e8860eea is indeed the
commit that inlined the wrapper and left the outer check in place.
Updated in v4.

Thanks,
-Geliang

> 
> 
> > ---
> > 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/35203307255