Re: [PATCH mptcp-next v3 0/3] send() fails with EAGAIN in blocking IO mode #487

[Geliang Tang <geliang@kernel.org>]

Hi Mat, Matt,

On Mon, 2025-06-16 at 14:34 +0800, Geliang Tang wrote:
> Hi Matt, Mat,
> 
> On Sun, 2025-06-15 at 23:29 +0200, Matthieu Baerts wrote:
> > Hi Mat, Geliang,
> > 
> > On 14/06/2025 01:11, Mat Martineau wrote:
> > > On Thu, 29 May 2025, Geliang Tang wrote:
> > > 
> > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > 
> > > > Good news! I finally solved the unstable issue of MPTCP BPF
> > > > sched
> > > > selftests
> > > > I reported a year ago, #487 "send() fails with EAGAIN in
> > > > blocking
> > > > IO
> > > > mode".
> > > > 
> > > > The fix is simple, it can be solved by explicitly setting
> > > > SO_SNDBUF
> > > > sockopt, but be sure not to set SO_RCVBUF at the same time
> > > > (see sk->sk_userlocks & SOCK_RCVBUF_LOCK in
> > > > mptcp_rcv_space_adjust()).
> > > > 
> > > > With this fix, BPF sched selftests are now very stable, I run
> > > > loop
> > > > testing
> > > > using mptcp-upstream-virtme-docker (run_loop run_bpftest_all),
> > > > and can
> > > > run
> > > > it normally for hundreds of times without error:
> > > > 
> > > 
> > > Hi Geliang -
> > > 
> > > I can see how changing SO_SNDBUF on the sending socket side would
> > > shift
> > > timing behavior in a way that affect the test outcome, but it
> > > doesn't
> > > address the root issue with bug #487:
> > > 
> > > It is either OK to get an EAGAIN from a blocking send(), or it's
> > > not OK.
> > > 
> > > 
> > > If it's not ok to ever return EAGAIN from a blocking send, the
> > > existing
> > > test code is a reproducer for a bug, and changing the test is
> > > hiding
> > > that bug.
> > > 
> > > If EAGAIN is ok, then we should change the code in
> > > send_recv_server() to
> > > allow it.
> 
> I did try to handle EAGAIN in send_recv_server() but it didn't work.
> MPTCP BPF sched selftests still fail. Test code and results are
> attached.
> 
> I added this in send_recv_server():
> 
> 	if (errno == EAGAIN && again < 5) {
> 		again++;
> 		continue;
> 	}
> 
> And still got the EAGAIN error:
> 
> # (network_helpers.c:728: errno: Resource temporarily unavailable)
> send
> 7867500 expected 10485760
> # (network_helpers.c:782: errno: Resource temporarily unavailable)
> recv
> 3469500 ​​expected 10485760
> # (network_helpers.c:790: errno: Resource temporarily unavailable)
> Failed in thread_ret -11
> # send_data_and_verify:FAIL:send_recv_data unexpected error: -11
> (errno
> 11)
> 
> In addition, BPF selftests adds a new mechanism that does not allow
> any
> test item to run for more than 10 seconds. Otherwise, the following
> error will be reported:
> 
> # WATCHDOG: test case mptcp/default executes for 10 seconds...
> 
> In my testing, I have not found any other solution besides limiting
> the
> send buffer. This allows data to be sent at a constant rate, which
> ensures the stability of MPTCP BPF sched selftests.
> 
> In order to avoid hiding this bug, we can add a test item for this in
> mptcp selftest in the future, like in [1].

Last week I debugged this issue further and found something (Thanks to
Gang Yan for his help): when mptcp bpf sched selftests fail, the memory
limit check (if (sk_rmem_alloc_get(sk) > sk->sk_rcvbuf)) is always true
in __mptcp_move_skbs_from_subflow(), at this time sk->sk_receive_queue
is empty, but no skb is moved from this subflow to sk-
>sk_receive_queue, which causes the transmission to fail.

One fix is ​​to also consider the case where sk->sk_receive_queue is
empty when doing the memory limit check:

  -               if (sk_rmem_alloc_get(sk) > sk->sk_rcvbuf)
  +               if (sk_rmem_alloc_get(sk) > sk->sk_rcvbuf &&
  +                   !skb_queue_empty(&sk->sk_receive_queue))
                          break;

In addition, this memory limit check was moved from the end of do {}
while (more_data_avail) to the front in the commit e0ca4057e0ec
("mptcp: micro-optimize __mptcp_move_skb()"), so another better fix is
​​to restore this check to the end of do {} while () so that move skbs
from this subflow to sk->sk_receive_queue always has a chance to do at
least once:

@@ -587,9 +587,6 @@ static bool __mptcp_move_skbs_from_subflow(struct
mptcp_sock *msk,
                struct sk_buff *skb;
                bool fin;
 
-               if (sk_rmem_alloc_get(sk) > sk->sk_rcvbuf)
-                       break;
-
                /* try to move as much data as available */
                map_remaining = subflow->map_data_len -
                                mptcp_subflow_get_map_offset(subflow);
@@ -634,6 +631,8 @@ static bool __mptcp_move_skbs_from_subflow(struct
mptcp_sock *msk,
                WRITE_ONCE(tp->copied_seq, seq);
                more_data_avail = mptcp_subflow_data_available(ssk);
 
+               if (sk_rmem_alloc_get(sk) > sk->sk_rcvbuf)
+                       break;
        } while (more_data_avail);

I think this is the root cause of #487, I would like to hear your
opinions on which fix is ​​better.

Thanks,
-Geliang

> 
> WDYT?
> 
> Thanks,
> -Geliang
> 
> [1]
> https://patchwork.kernel.org/project/mptcp/cover/cover.1722502941.git.tanggeliang@kylinos.cn/
> 
> > 
> > It is now a bit hidden in the middle of #487, but if I'm not
> > mistaken,
> > it is OK to get EAGAIN with a blocking send() **if** SO_SNDTIMEO is
> > used, and in case of timeout.
> > 
> > See:
> > https://github.com/multipath-tcp/mptcp_net-next/issues/487#issuecomment-2485577676
> > 
> > So I think the question should be: is it normal to block for longer
> > than
> > the timeout period (which is a "long" period, no?)? If yes, then
> > limiting the send buffer might be a solution, but as Mat said, it
> > looks
> > better to understand the root cause than hiding a bug :)
> > 
> > Cheers,
> > Matt