[PATCH mptcp-next v2 0/8] mptcp: sched: reinjection and a send limit as scheduler actions

[PATCH mptcp-next v2 0/8] mptcp: sched: reinjection and a send limit as scheduler actions

[Matthieu Baerts (NGI0)]

Note: this cover-letter has been written by Abdul. I only modified the
ChangeLog to add my recent modifications. (The results from the perf lab
were the ones from Abdul, before my modifications.)

This takes over Gregory's "update scheduler API" RFC [1], and redoes the
opportunistic reinjection RFC [2] as a scheduler action, as Matt asked.

Patch 1 is a fix: the send window could go past what the peer offered
after a retransmission, which reinjecting makes a lot more likely.

A scheduler can't do two things today:

- Send less than one dfrag per decision. msk->snd_burst is only checked
  once a whole dfrag is sent, and a dfrag can be 32 KB, so for example
  one MSS per subflow in turn is not possible. Patch 2 adds send_limit,
  a subflow field set in get_send, used for that decision only. This is
  the "limit" of [1], as a struct member checked at load time and not a
  second call with flags, as Mat suggested on [1]. Stopping inside a
  dfrag leaves it the send head, partly sent, as when a subflow runs out
  of space, so first_pending moves as before. Patch 6 lets BPF
  schedulers set it.

- Resend data that a slower subflow holds on an idle one. Patch 3 calls
  get_retrans for that too, so there is still one call per decision.
  msk->sched_retrans says why: the MPTCP RTO as before, or a subflow is
  idle while nothing new can be sent because the receive window is full,
  the send buffer is full, or the application has nothing more to write:
  patch 4 checks this on each DATA_ACK.
  The scheduler marks a subflow and sets reinject_limit there, and the
  core resends the DSN ranges the other subflows hold, which a BPF
  program can't walk. Patch 5 gives the default scheduler the policy of
  [2]: only when the copy arrives at least one RTT of the idle subflow
  sooner. Patch 6 lets BPF schedulers set reinject_limit too.

Shardul asked whether the application limited case should trigger it
[3]. I measured it: it is where most of the latency gain is, so it stays
and the reason is passed to the scheduler, which can ignore it.

Tests, on the current export with Shardul's v5:
- the two new BPF tests and the other mptcp BPF tests, on a debug kernel
  (KASAN, lockdep); the send limit test fails without patch 2 (32728
  bytes in one decision instead of at most 4096);
- mptcp_connect.sh (all modes), mptcp_join.sh, mptcp_sockopt.sh,
  pm_netlink.sh, simult_flows.sh, userspace_pm.sh, the kunit and
  packetdrill tests, on the same debug kernel;
- the perf lab [4], 5 runs of the base above as history, then the
  series (alpha 0.01). 106 tests unchanged, 28 improvements, 5 flagged.
  The largest changes, iperf3 received in Mbit/s:

    weak_secondary                                       20.6 -> 70.1
    bad_secondary                                        22.8 -> 70.5
    bad_secondary bbr                                    39.2 -> 68.0
    bloated_slow_secondary receiver_tcp_mem parallel=99  41.4 -> 92.4
    huge_lat_primary rmem=200                             8.1 -> 17.3

  Of the 5 flagged, 4 are only how the data is split between the paths,
  the total does not go down: huge_lat_primary, clientlim slow_secondary
  bw=5 and slow_primary bw=10 (their totals go up by 2-7%), and
  weak_secondary backup (about 12 bit/s on the backup path in both). The
  one cost is mem-pressure slow_primary both_tcp_mem parallel=10: iperf3
  received is 1-2% lower (51.2-51.7 vs 52.1, 4 runs), the receiver total
  is the same, so these are the copies. I reran the flagged suites to
  check, the results were the same.

[1] https://lore.kernel.org/mptcp/20240527-sched_per_packet-v1-0-09a41d405f7c@gmail.com/
[2] https://lore.kernel.org/mptcp/20261025134643.1537069-1-w453y.me@gmail.com/
[3] https://lore.kernel.org/mptcp/6ac6cd6f5c7d67c598ae34bbf3bcf6a0edd0e983.camel@mpiricsoftware.com/
[4] https://github.com/multipath-tcp/mptcp-upstream-virtme-docker/blob/latest/PERF.md#tracking-regressions-locally

Based-on: <20261028-mptcp_penalise_send_v2-v5-0-32fb379ce117@mpiricsoftware.com>
Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v2 (of [2]):
- a scheduler action through get_retrans, not in the core for every
  scheduler;
- rebased on Shardul's "penalise a slow subflow" v5;
- the reason is passed to the scheduler (Shardul);
- the subflow list is only walked while no process owns the msk, the
  ranges are merged, and the budget is exact (sashiko-bot);
- a scheduler that doesn't implement get_retrans never wakes the worker
  (MPTCP CI, BPF first test);
- fix the send window after a retransmission, new patch 1;
- check for an idle subflow with all schedulers, deferred if msk owned;
- a refusal is only final for a snd_una with an idle subflow;
- joining and closed subflows are no longer seen as gone;
- only copy what slow or gone subflows hold, lowest DSN first;
- clear left-over send_limit and scheduled flags.

---
Abdul Wasey (6):
      mptcp: sched: let get_retrans reinject on an idle subflow
      mptcp: sched: look for an idle subflow on each DATA_ACK
      mptcp: sched: reinject on an idle subflow with the default scheduler
      mptcp: sched: let a BPF scheduler set the send and reinject limits
      selftests: bpf: mptcp: test the send limit of a scheduler
      selftests: bpf: mptcp: test a scheduler reinjecting on an idle subflow

Gregory Detal (1):
      mptcp: sched: let get_send limit what one decision sends

Matthieu Baerts (NGI0) (1):
      mptcp: do not use a raised snd_wnd for the send window

 net/mptcp/bpf.c                                    |   6 +
 net/mptcp/mib.c                                    |   2 +
 net/mptcp/mib.h                                    |   2 +
 net/mptcp/options.c                                |  12 +-
 net/mptcp/protocol.c                               | 122 ++++++-
 net/mptcp/protocol.h                               |  51 +++
 net/mptcp/sched.c                                  | 384 ++++++++++++++++++++-
 tools/testing/selftests/bpf/prog_tests/mptcp.c     | 130 +++++++
 .../testing/selftests/bpf/progs/mptcp_bpf_limit.c  |  94 +++++
 .../selftests/bpf/progs/mptcp_bpf_reinject.c       |  70 ++++
 10 files changed, 868 insertions(+), 5 deletions(-)
---
base-commit: df540f052ed58044886a6f6f98cc98c25172764f
change-id: 20261031-mptcp-sched-opp-retrans-6d9a1dd6cb8f

Best regards,
--  
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-next v2 1/8] mptcp: do not use a raised snd_wnd for the send window

[Matthieu Baerts (NGI0)]

ack_update_msk() sets the right edge of the MPTCP send window from the
DATA_ACK and tcp_sk(ssk)->snd_wnd, but this is not always what the peer
announced: mptcp_check_allowed_size() raises it to
window_end - data_seq, so that TCP lets through what the MPTCP-level
window allows.

That is fine for new data. For data sent again, data_seq can be below
snd_una, and snd_wnd ends up too large by the difference. TCP only
overwrites it when an ACK moves things forward on this subflow, see
tcp_may_update_window(). If a DATA_ACK moved by another subflow comes in
on this subflow before that, wnd_end goes past what the peer offered. It
only moves forward, so it stays there.

The sender then transmits data beyond the peer's window: the peer drops
it (MPTCP_MIB_NODSSWINDOW) after TCP acked it, and only an MPTCP-level
retransmission can recover it, which raises snd_wnd again. The
connection can then stay stuck at a low rate for several seconds, with
nothing in flight on any subflow.

Use the window announced in the packet carrying the DATA_ACK instead.

In my case with BBR, a small TCP rmem, and a second path with a much
higher latency, a lower throughput and some losses, this small change
can double the throughput in the worst cases.

Fixes: b713d0067574 ("mptcp: really share subflow snd_wnd")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index adbf8e0773db..b0e8b1200cfe 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1121,9 +1121,11 @@ static void rwin_update(struct mptcp_sock *msk, struct sock *ssk,
 
 static void ack_update_msk(struct mptcp_sock *msk,
 			   struct sock *ssk,
+			   const struct sk_buff *skb,
 			   struct mptcp_options_received *mp_opt)
 {
 	u64 new_wnd_end, new_snd_una, snd_nxt = READ_ONCE(msk->snd_nxt);
+	u32 win = ntohs(tcp_hdr(skb)->window);
 	struct sock *sk = (struct sock *)msk;
 	u64 old_snd_una;
 
@@ -1140,7 +1142,12 @@ static void ack_update_msk(struct mptcp_sock *msk,
 	if (unlikely(after64(new_snd_una, snd_nxt)))
 		new_snd_una = old_snd_una;
 
-	new_wnd_end = new_snd_una + tcp_sk(ssk)->snd_wnd;
+	/* What the peer announced: tcp_sk(ssk)->snd_wnd can be larger, see
+	 * mptcp_check_allowed_size(), and is not always updated.
+	 */
+	if (likely(!tcp_hdr(skb)->syn))
+		win <<= tcp_sk(ssk)->rx_opt.snd_wscale;
+	new_wnd_end = new_snd_una + win;
 
 	if (after64(new_wnd_end, msk->wnd_end))
 		WRITE_ONCE(msk->wnd_end, new_wnd_end);
@@ -1310,7 +1317,7 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 	 * monodirectional flows will stuck
 	 */
 	if (mp_opt.use_ack)
-		ack_update_msk(msk, sk, &mp_opt);
+		ack_update_msk(msk, sk, skb, &mp_opt);
 	rwin_update(msk, sk, skb);
 
 	/* Zero-data-length packets are dropped by the caller and not

-- 
2.55.0

[PATCH mptcp-next v2 2/8] mptcp: sched: let get_send limit what one decision sends

[Matthieu Baerts (NGI0)]

From: Gregory Detal <gregory.detal@gmail.com>

A scheduler picks the subflows for the pending data in get_send, then
__subflow_push_pending() sends on each of them until msk->snd_burst is
used up, the subflow has no room, or it is not active. snd_burst is only
checked once a whole dfrag is sent, and one dfrag can hold up to a page
fragment, 32 KB, so a scheduler cannot ask for less than that. Sending
one MSS per subflow in turn, for example, is not possible.

Add send_limit to the subflow context, which a scheduler can set in
get_send: at most that many bytes are sent on the subflow for this
decision, then the scheduler is asked again. 0, the default, keeps the
current behaviour. The limit applies to this decision only:
__subflow_push_pending() reads and clears it, and mptcp_sched_get_send()
clears the limits before asking a scheduler: a retransmission can leave
one behind, as it calls get_send when the scheduler has no get_retrans,
without using the limit. Nothing sets it yet: a later patch lets a BPF
scheduler write it.

When the limit is reached inside a dfrag, the dfrag stays the send head,
partly sent, as it already does when the subflow runs out of space in
the middle of it. first_pending then moves on only once the dfrag is
fully sent, as before.

This is based on Gregory Detal's RFC called "mptcp: update scheduler
API" [1] but applying Mat Martineau's suggestion.

Link: https://lore.kernel.org/mptcp/20240527-sched_per_packet-v1-0-09a41d405f7c@gmail.com/ [1]
Signed-off-by: Gregory Detal <gregory.detal@gmail.com>
Co-developed-by: Abdul Wasey <w453y.me@gmail.com>
Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 22 ++++++++++++++++++++--
 net/mptcp/protocol.h |  4 ++++
 net/mptcp/sched.c    |  4 ++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 8f3c6ebd49ee..d960ab9c76b3 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1823,17 +1823,25 @@ void mptcp_check_and_set_pending(struct sock *sk)
 static int __subflow_push_pending(struct sock *sk, struct sock *ssk,
 				  struct mptcp_sendmsg_info *info)
 {
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct mptcp_data_frag *dfrag;
 	int len, copied = 0, err = 0;
+	u32 limit;
 
-	if (mptcp_subflow_ctx(ssk)->penalise)
+	if (subflow->penalise)
 		mptcp_penalise_cwnd(ssk);
 
+	/* the scheduler's limit for this decision only */
+	limit = subflow->send_limit;
+	subflow->send_limit = 0;
+
 	while ((dfrag = mptcp_send_head(sk))) {
 		info->sent = dfrag->already_sent;
 		info->limit = dfrag->data_len;
-		len = dfrag->data_len - dfrag->already_sent;
+		if (limit && dfrag->data_len - info->sent > limit - copied)
+			info->limit = info->sent + limit - copied;
+		len = info->limit - info->sent;
 		while (len > 0) {
 			int ret = 0;
 
@@ -1849,6 +1857,16 @@ static int __subflow_push_pending(struct sock *sk, struct sock *ssk,
 
 			mptcp_update_post_push(msk, dfrag, ret);
 		}
+
+		/* Stopped inside the dfrag: it stays the send head, as when the
+		 * subflow runs out of space, and the scheduler is asked again.
+		 */
+		if (limit && copied >= limit) {
+			if (dfrag->already_sent == dfrag->data_len)
+				msk->first_pending = mptcp_send_next(sk);
+			err = copied;
+			goto out;
+		}
 		msk->first_pending = mptcp_send_next(sk);
 
 		if (msk->snd_burst <= 0 ||
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 1d41ab1da2e0..4e8920747682 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -606,6 +606,10 @@ struct mptcp_subflow_context {
 	bool	data_avail;
 	bool	scheduled;
 	bool	penalise;	    /* scheduler flagged this subflow for cwnd halving */
+	u32	send_limit;	    /* set by the scheduler in get_send: at
+				     * most this many bytes before it is
+				     * asked again; 0 for no limit
+				     */
 	bool	pm_listener;	    /* a listener managed by the kernel PM? */
 	bool	fully_established;  /* path validated */
 	bool	close_event_done;   /* has done the post-closed part,
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 1e59072d478c..c8d193cb0e4c 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -189,6 +189,10 @@ int mptcp_sched_get_send(struct mptcp_sock *msk)
 
 	if (msk->sched == &mptcp_sched_default || !msk->sched)
 		return mptcp_sched_default_get_send(msk);
+
+	/* limits are per decision: a retransmission can leave some behind */
+	mptcp_for_each_subflow(msk, subflow)
+		subflow->send_limit = 0;
 	return msk->sched->get_send(msk);
 }
 

-- 
2.55.0

[PATCH mptcp-next v2 3/8] mptcp: sched: let get_retrans reinject on an idle subflow

[Matthieu Baerts (NGI0)]

From: Abdul Wasey <w453y.me@gmail.com>

With subflows of different speeds, the data sent on a slow subflow holds
back the DATA_ACK while the fast one keeps delivering what comes after
it. Once the receive window or the send buffer is used up, nothing new
can be sent: the fast subflow drains and stays idle until the slow one
delivers, or until the MPTCP-level RTO fires. The same happens when the
application has nothing more to write for now: the end of what it wrote
waits on the slow subflow. The old out-of-tree kernel resent that data
on the idle subflow ("opportunistic retransmission", Raiciu et al., NSDI
2012), upstream has nothing for it [1].

Do it as a scheduler action, through get_retrans, so that there is still
one call per decision: get_retrans is now also called when an active
non-backup subflow has nothing queued or in flight while nothing new can
be sent, and msk->sched_retrans says why it is called:

- MPTCP_SCHED_RETRANS_RTO: the MPTCP-level timer fired, as before;
- MPTCP_SCHED_RETRANS_IDLE_WND: the peer's receive window is nearly
  full;
- MPTCP_SCHED_RETRANS_IDLE_SNDBUF: the application waits for send buffer
  space;
- MPTCP_SCHED_RETRANS_IDLE_APP: the application has nothing more to send
  for now.

On the IDLE reasons, the scheduler marks a subflow and sets the new
reinject_limit there. The core then resends on it, up to that many bytes
and one cwnd, the DSN ranges that the other subflows still hold, taken
from the DSS mappings of the skbs in their queues: a BPF program cannot
walk them. Most of the data after snd_una already reached the peer, so
this is less than resending everything from snd_una. A limit of 0, the
default, sends nothing. This is done at most once per snd_una value.

This is not done with the MPTCP checksum: a re-injection must then spool
a complete DSS, see commit 4e14867d5e91 ("mptcp: tune re-injections for
csum enabled mode").

This runs in the worker, under the msk socket lock, as the MPTCP-level
retransmission does. If no subflow is idle there, or if a send is
pending, nothing is decided for this snd_una. Otherwise, the answer of
the scheduler is final for this snd_una. Nothing wakes the worker yet:
that is the next patch.

Only a scheduler with its own get_retrans is asked: the default one does
not reinject yet, see a later patch. A BPF scheduler cannot set
reinject_limit yet either: a later patch allows it. One that does not
want to reinject can ignore the IDLE reasons.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/332 [1]
Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/mib.c      |   2 +
 net/mptcp/mib.h      |   2 +
 net/mptcp/protocol.c |  89 +++++++++++++++++++
 net/mptcp/protocol.h |  44 +++++++++
 net/mptcp/sched.c    | 245 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 382 insertions(+)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index b39b5511345d..7fe0fd71cb43 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -97,6 +97,8 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("OFOPruned", MPTCP_MIB_OFOPRUNED),
 	SNMP_MIB_ITEM("CwndPenalised", MPTCP_MIB_CWNDPENALISED),
 	SNMP_MIB_ITEM("PenalCandidate", MPTCP_MIB_PENALCAND),
+	SNMP_MIB_ITEM("OppReinject", MPTCP_MIB_OPPREINJ),
+	SNMP_MIB_ITEM("OppReinjectBytes", MPTCP_MIB_OPPREINJBYTES),
 };
 
 /* mptcp_mib_alloc - allocate percpu mib counters
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index a7b5bf684af7..615666050f51 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -100,6 +100,8 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_OFOPRUNED,		/* MPTCP-level OoO queue pruned */
 	MPTCP_MIB_CWNDPENALISED,	/* subflow cwnd halved by the scheduler */
 	MPTCP_MIB_PENALCAND,		/* scheduler picked a slow (low-rate) subflow */
+	MPTCP_MIB_OPPREINJ,		/* Opportunistic reinjections on an idle subflow */
+	MPTCP_MIB_OPPREINJBYTES,	/* Bytes sent by opportunistic reinjections */
 	__MPTCP_MIB_MAX
 };
 
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d960ab9c76b3..7117786e5251 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3057,6 +3057,92 @@ static int __mptcp_push_retrans(struct sock *sk, struct mptcp_data_frag *dfrag,
 	return len;
 }
 
+/* resend [start, end) on @ssk from the msk rtx queue; < 0 on a send error */
+static int mptcp_oppr_send_range(struct sock *sk, struct sock *ssk, u64 start,
+				 u64 end, int budget,
+				 struct mptcp_sendmsg_info *info)
+{
+	struct mptcp_data_frag *dfrag;
+	int copied = 0, ret;
+
+	list_for_each_entry(dfrag, &mptcp_sk(sk)->rtx_queue, list) {
+		if (!dfrag->already_sent || !before64(dfrag->data_seq, end))
+			break;
+		if (!after64(dfrag->data_seq + dfrag->already_sent, start))
+			continue;
+
+		info->sent = before64(dfrag->data_seq, start) ?
+			     start - dfrag->data_seq : 0;
+		info->limit = min_t(u64, dfrag->already_sent,
+				    end - dfrag->data_seq);
+		info->limit = min_t(u64, info->limit,
+				    info->sent + budget - copied);
+		while (info->sent < info->limit && copied < budget) {
+			ret = mptcp_sendmsg_frag(sk, ssk, dfrag, info);
+			if (ret <= 0)
+				return copied ? : -1;
+			copied += ret;
+			info->sent += ret;
+		}
+		if (copied >= budget)
+			break;
+	}
+	return copied;
+}
+
+/* Opportunistic reinjection: the scheduling part is in sched.c */
+static void __mptcp_oppr(struct sock *sk)
+{
+	struct mptcp_sendmsg_info info = { .data_lock_held = true, };
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	int copied = 0, budget, ret, i;
+	struct mptcp_sched_oppr oppr;
+	struct sock *ssk;
+
+	mptcp_data_lock(sk);
+	__mptcp_clean_una_wakeup(sk);
+	oppr.una = msk->snd_una;
+	mptcp_data_unlock(sk);
+
+	if (!mptcp_sched_oppr_get(msk, &oppr))
+		return;
+
+	ssk = oppr.ssk;
+	lock_sock(ssk);
+	/* the scheduler's limit, and no more than one cwnd */
+	budget = min_t(u64, oppr.limit,
+		       tcp_snd_cwnd(tcp_sk(ssk)) * tcp_sk(ssk)->mss_cache);
+
+	spin_lock_bh(&msk->fallback_lock);
+	if (__mptcp_check_fallback(msk) || !msk->allow_subflows)
+		goto unlock;
+
+	for (i = 0; i < oppr.n && copied < budget; i++) {
+		ret = mptcp_oppr_send_range(sk, ssk, oppr.r[i].start,
+					    oppr.r[i].end, budget - copied,
+					    &info);
+		if (ret < 0)
+			break;
+		copied += ret;
+	}
+
+	if (copied) {
+		/* once per snd_una value; after a failed send, the next
+		 * DATA_ACK tries again
+		 */
+		WRITE_ONCE(msk->oppr_una, oppr.una);
+		tcp_push(ssk, 0, info.mss_now, tcp_sk(ssk)->nonagle,
+			 info.size_goal);
+		msk->allow_infinite_fallback = false;
+		msk->bytes_retrans += copied;
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OPPREINJ);
+		MPTCP_ADD_STATS(sock_net(sk), MPTCP_MIB_OPPREINJBYTES, copied);
+	}
+unlock:
+	spin_unlock_bh(&msk->fallback_lock);
+	release_sock(ssk);
+}
+
 static void __mptcp_retrans(struct sock *sk)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
@@ -3295,6 +3381,9 @@ static void mptcp_worker(struct work_struct *work)
 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
 		__mptcp_retrans(sk);
 
+	if (test_and_clear_bit(MPTCP_WORK_OPPR, &msk->flags))
+		__mptcp_oppr(sk);
+
 	fail_tout = msk->first ? READ_ONCE(mptcp_subflow_ctx(msk->first)->fail_tout) : 0;
 	if (fail_tout && time_after(jiffies, fail_tout))
 		mptcp_mp_fail_no_response(msk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 4e8920747682..39dd56d882ce 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -117,6 +117,7 @@
 #define MPTCP_FALLBACK_DONE	2
 #define MPTCP_WORK_CLOSE_SUBFLOW 3
 #define MPTCP_RTX_ENABLED	4
+#define MPTCP_WORK_OPPR		5
 
 /* MPTCP socket release cb flags */
 #define MPTCP_PUSH_PENDING	1
@@ -294,6 +295,40 @@ struct mptcp_data_frag {
  */
 #define MPTCP_RTT_SAMPLES	5
 
+/* Why get_retrans is called, in msk->sched_retrans */
+enum mptcp_sched_retrans {
+	/* the MPTCP-level retransmission timer fired */
+	MPTCP_SCHED_RETRANS_RTO,
+	/* A subflow is idle, nothing new can be sent, and other subflows
+	 * hold data the peer is waiting for. The core resends that data on
+	 * the subflow the scheduler marks, up to the reinject_limit bytes it
+	 * sets there: 0, the default, sends nothing. Nothing new can be sent
+	 * because:
+	 */
+	/* the peer's receive window is nearly full */
+	MPTCP_SCHED_RETRANS_IDLE_WND,
+	/* the application waits for send buffer space */
+	MPTCP_SCHED_RETRANS_IDLE_SNDBUF,
+	/* the application has nothing more to send for now */
+	MPTCP_SCHED_RETRANS_IDLE_APP,
+};
+
+#define MPTCP_OPPR_MAX_RANGES	32
+
+struct mptcp_oppr_range {
+	u64 start;
+	u64 end;
+};
+
+/* What __mptcp_oppr() resends: see mptcp_sched_oppr_get() */
+struct mptcp_sched_oppr {
+	struct sock *ssk;
+	u32 limit;
+	u64 una;
+	int n;
+	struct mptcp_oppr_range r[MPTCP_OPPR_MAX_RANGES];
+};
+
 /* MPTCP connection sock */
 struct mptcp_sock {
 	/* inet_connection_sock must be the first member */
@@ -310,6 +345,10 @@ struct mptcp_sock {
 	atomic64_t	rcv_wnd_sent;
 	u64		rcv_data_fin_seq;
 	u64		bytes_retrans;
+	u64		oppr_una;	/* snd_una of the last opportunistic
+					 * reinjection, or of its refusal
+					 */
+	u8		sched_retrans;	/* enum mptcp_sched_retrans */
 	u64		bytes_consumed;
 	int		snd_burst;
 	int		old_wspace;
@@ -610,6 +649,9 @@ struct mptcp_subflow_context {
 				     * most this many bytes before it is
 				     * asked again; 0 for no limit
 				     */
+	u32	reinject_limit;	    /* set by the scheduler, on an
+				     * MPTCP_SCHED_RETRANS_IDLE_* reason
+				     */
 	bool	pm_listener;	    /* a listener managed by the kernel PM? */
 	bool	fully_established;  /* path validated */
 	bool	close_event_done;   /* has done the post-closed part,
@@ -888,6 +930,8 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk);
 struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk);
 int mptcp_sched_get_send(struct mptcp_sock *msk);
 int mptcp_sched_get_retrans(struct mptcp_sock *msk);
+bool mptcp_sched_oppr_get(struct mptcp_sock *msk,
+			  struct mptcp_sched_oppr *oppr);
 
 static inline u64 mptcp_data_avail(const struct mptcp_sock *msk)
 {
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index c8d193cb0e4c..da5fa8c0fb12 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -28,6 +28,31 @@ static int mptcp_sched_default_get_send(struct mptcp_sock *msk)
 	return 0;
 }
 
+/* the active subflow with nothing to send or retransmit and the lowest RTT */
+static struct sock *mptcp_sched_oppr_idle_ssk(struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow;
+	struct sock *pick = NULL;
+	u32 best = U32_MAX;
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+		u32 rtt = READ_ONCE(tcp_sk(ssk)->srtt_us);
+
+		if (!__mptcp_subflow_active(subflow) || subflow->backup ||
+		    subflow->request_bkup || subflow->stale || !rtt)
+			continue;
+		/* racy on a DATA_ACK, only a hint: the worker checks again */
+		if (!data_race(tcp_rtx_and_write_queues_empty(ssk)))
+			continue;
+		if (rtt < best) {
+			best = rtt;
+			pick = ssk;
+		}
+	}
+	return pick;
+}
+
 static int mptcp_sched_default_get_retrans(struct mptcp_sock *msk)
 {
 	struct sock *ssk;
@@ -217,3 +242,223 @@ int mptcp_sched_get_retrans(struct mptcp_sock *msk)
 		return msk->sched->get_retrans(msk);
 	return msk->sched->get_send(msk);
 }
+
+/* Opportunistic reinjection, "opportunistic retransmission" in section 4.2
+ * of Raiciu et al., NSDI 2012.
+ *
+ * With paths of different speeds, data sent on a slow or lossy path holds
+ * back the DATA_ACK while the fast path keeps delivering what comes after
+ * it. Once the receive window or the send buffer is used up, nothing new
+ * can be sent: the fast path drains and stays idle until the slow path
+ * delivers, or until the MPTCP-level RTO fires. The same happens when the
+ * application has nothing more to send for now: the end of what it wrote
+ * waits on the slow path, and so does the application if it expects an
+ * answer.
+ *
+ * That state can be seen when a DATA_ACK comes in: no new data can go out,
+ * and an active subflow has nothing queued or in flight. The data at
+ * snd_una is then held by another subflow, so resend on the idle subflow
+ * the ranges that the other subflows still hold, taken from the DSS
+ * mappings in their queues, instead of everything from snd_una: most of
+ * that already reached the peer.
+ *
+ * The scheduler decides on which subflow, and how much, through get_retrans
+ * with the reason in msk->sched_retrans. This is done at most once per
+ * snd_una value.
+ */
+#define MPTCP_OPPR_MIN		(16 * 1024)
+
+/* can the scheduler be asked to reinject: see mptcp_sched_get_reinject() */
+static bool mptcp_sched_has_reinject(const struct mptcp_sock *msk)
+{
+	const struct mptcp_sched_ops *sched = READ_ONCE(msk->sched);
+
+	return sched && sched != &mptcp_sched_default && sched->get_retrans;
+}
+
+/* Ask the scheduler where to resend the data other subflows hold, because a
+ * subflow is idle while nothing new can be sent, for @reason: see
+ * MPTCP_SCHED_RETRANS_IDLE_WND. Only a scheduler with its own get_retrans is
+ * asked; the default one does not reinject.
+ */
+static int mptcp_sched_get_reinject(struct mptcp_sock *msk, u8 reason)
+{
+	struct mptcp_subflow_context *subflow;
+	int err;
+
+	msk_owned_by_me(msk);
+
+	if (__mptcp_check_fallback(msk))
+		return -EINVAL;
+
+	mptcp_for_each_subflow(msk, subflow) {
+		if (READ_ONCE(subflow->scheduled))
+			return -EBUSY;
+		subflow->reinject_limit = 0;
+	}
+
+	if (!mptcp_sched_has_reinject(msk))
+		return -EOPNOTSUPP;
+
+	msk->sched_retrans = reason;
+	err = msk->sched->get_retrans(msk);
+	msk->sched_retrans = MPTCP_SCHED_RETRANS_RTO;
+	return err;
+}
+
+/* Why nothing new can go out while the peer waits for data: an
+ * MPTCP_SCHED_RETRANS_IDLE_* reason, or 0 when new data can still go out
+ * (the scheduler will then use the idle subflow) or nothing is waiting.
+ * Read without the msk socket lock when called from a DATA_ACK.
+ */
+static u8 mptcp_sched_oppr_reason(struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	u64 snd_una = READ_ONCE(msk->snd_una);
+	struct socket *sock;
+
+	/* with csum, a resend must spool a complete DSS */
+	if (snd_una == READ_ONCE(msk->snd_nxt) ||
+	    snd_una == READ_ONCE(msk->oppr_una) ||
+	    READ_ONCE(msk->csum_enabled) || __mptcp_check_fallback(msk))
+		return 0;
+
+	/* only the DATA_FIN is waiting */
+	if (snd_una + 1 == READ_ONCE(msk->snd_nxt) &&
+	    mptcp_data_fin_enabled(msk))
+		return 0;
+
+	if (mptcp_send_head(sk)) {
+		if (mptcp_wnd_end(msk) - READ_ONCE(msk->snd_nxt) >= MPTCP_OPPR_MIN)
+			return 0;
+		return MPTCP_SCHED_RETRANS_IDLE_WND;
+	}
+
+	/* everything written so far was handed to a subflow */
+	sock = READ_ONCE(sk->sk_socket);
+	if (sock && test_bit(SOCK_NOSPACE, &sock->flags))
+		return MPTCP_SCHED_RETRANS_IDLE_SNDBUF;
+	return MPTCP_SCHED_RETRANS_IDLE_APP;
+}
+
+/* Add what @skb holds to @r, sorted by start. When @r is full, keep the lowest
+ * ones: the data at snd_una is the one the peer is waiting for.
+ */
+static int mptcp_sched_oppr_add_skb(const struct sk_buff *skb, u64 una,
+				    u64 *last, struct mptcp_oppr_range *r,
+				    int n)
+{
+	const struct mptcp_ext *mpext = mptcp_get_ext(skb);
+	u64 start, end;
+	int i;
+
+	if (!mpext || !mpext->use_map || !mpext->data_len ||
+	    mpext->data_seq == *last)
+		return n;
+
+	/* one mapping can span several skbs after a TCP split */
+	*last = mpext->data_seq;
+	start = mpext->data_seq;
+	end = start + mpext->data_len;
+	if (!after64(end, una))
+		return n;
+	if (before64(start, una))
+		start = una;
+
+	if (n == MPTCP_OPPR_MAX_RANGES) {
+		if (!before64(start, r[n - 1].start))
+			return n;
+		n--;
+	}
+
+	for (i = n; i > 0 && after64(r[i - 1].start, start); i--)
+		r[i] = r[i - 1];
+	r[i].start = start;
+	r[i].end = end;
+	return n + 1;
+}
+
+/* The DSN ranges at or after @una that the subflows other than @idle still
+ * hold, sent or not: the data the peer is waiting for. Sorted by start, and
+ * merged: data resent before can sit on more than one subflow.
+ */
+static int mptcp_sched_oppr_collect(struct mptcp_sock *msk, struct sock *idle,
+				    u64 una, struct mptcp_oppr_range *r)
+{
+	struct mptcp_subflow_context *subflow;
+	int n = 0, i, k;
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+		struct sk_buff *skb;
+		u64 last = 0;
+
+		if (ssk == idle)
+			continue;
+
+		lock_sock(ssk);
+		skb_rbtree_walk(skb, &ssk->tcp_rtx_queue)
+			n = mptcp_sched_oppr_add_skb(skb, una, &last, r, n);
+		skb_queue_walk(&ssk->sk_write_queue, skb)
+			n = mptcp_sched_oppr_add_skb(skb, una, &last, r, n);
+		release_sock(ssk);
+	}
+
+	for (i = 1, k = 0; i < n; i++) {
+		if (!after64(r[i].start, r[k].end)) {
+			if (after64(r[i].end, r[k].end))
+				r[k].end = r[i].end;
+		} else {
+			r[++k] = r[i];
+		}
+	}
+	return n ? k + 1 : 0;
+}
+
+/* What to resend from @oppr->una for an opportunistic reinjection, if anything:
+ * ask the scheduler, then take the ranges from the subflows. Called from the
+ * worker.
+ */
+bool mptcp_sched_oppr_get(struct mptcp_sock *msk,
+			  struct mptcp_sched_oppr *oppr)
+{
+	struct mptcp_subflow_context *subflow;
+	struct sock *sk = (struct sock *)msk;
+	u8 reason;
+	int err;
+
+	/* no subflow to use yet: the next DATA_ACK checks again */
+	reason = mptcp_sched_oppr_reason(sk);
+	if (!reason || !mptcp_sched_oppr_idle_ssk(msk))
+		return false;
+
+	/* same with a send pending: new data is about to go out */
+	err = mptcp_sched_get_reinject(msk, reason);
+	if (err == -EBUSY)
+		return false;
+
+	oppr->ssk = NULL;
+	oppr->limit = 0;
+	oppr->n = 0;
+	mptcp_for_each_subflow(msk, subflow) {
+		if (!READ_ONCE(subflow->scheduled))
+			continue;
+		if (!err && !oppr->ssk && subflow->reinject_limit) {
+			oppr->ssk = mptcp_subflow_tcp_sock(subflow);
+			oppr->limit = subflow->reinject_limit;
+		}
+		mptcp_subflow_set_scheduled(subflow, false);
+	}
+
+	/* the scheduler declined, or nothing to resend: not again for this
+	 * snd_una
+	 */
+	if (oppr->ssk)
+		oppr->n = mptcp_sched_oppr_collect(msk, oppr->ssk, oppr->una,
+						   oppr->r);
+	if (!oppr->n) {
+		WRITE_ONCE(msk->oppr_una, oppr->una);
+		return false;
+	}
+	return true;
+}

-- 
2.55.0

[PATCH mptcp-next v2 4/8] mptcp: sched: look for an idle subflow on each DATA_ACK

[Matthieu Baerts (NGI0)]

From: Abdul Wasey <w453y.me@gmail.com>

The reinjection added by the previous patch runs in the worker, which
takes the msk socket lock and delays the receive path. Only wake it when
it can do something: when a DATA_ACK leaves nothing new to send, with a
subflow idle.

Checking the subflows on a DATA_ACK needs a stable subflow list, so only
while no process owns the msk. Otherwise, defer the check to the release
of the msk socket lock, as MPTCP_CLEAN_UNA does. Waking the worker
instead would do it for most DATA_ACKs, even with only one subflow, and
often before a subflow is idle.

Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c  |  1 +
 net/mptcp/protocol.c | 11 +++++++++++
 net/mptcp/protocol.h |  3 +++
 net/mptcp/sched.c    | 17 +++++++++++++++++
 4 files changed, 32 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index b0e8b1200cfe..626a1e2fc4de 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1161,6 +1161,7 @@ static void ack_update_msk(struct mptcp_sock *msk,
 		__mptcp_data_acked(sk);
 	}
 	msk->last_ack_recv = tcp_jiffies32;
+	__mptcp_check_oppr(sk);
 	mptcp_data_unlock(sk);
 
 	trace_ack_update_msk(mp_opt->data_ack,
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7117786e5251..800c2a4ae3a0 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -4057,6 +4057,15 @@ void __mptcp_check_push(struct sock *sk, struct sock *ssk)
 		__set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->cb_flags);
 }
 
+/* the subflow list is only stable while no process owns the msk */
+void __mptcp_check_oppr(struct sock *sk)
+{
+	if (!sock_owned_by_user(sk))
+		mptcp_sched_oppr_check(sk);
+	else
+		__set_bit(MPTCP_OPPR_CHECK, &mptcp_sk(sk)->cb_flags);
+}
+
 #define MPTCP_FLAGS_PROCESS_CTX_NEED (BIT(MPTCP_PUSH_PENDING) | \
 				      BIT(MPTCP_RETRANSMIT) | \
 				      BIT(MPTCP_FLUSH_JOIN_LIST))
@@ -4121,6 +4130,8 @@ static void mptcp_release_cb(struct sock *sk)
 			__mptcp_error_report(sk);
 		if (__test_and_clear_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags))
 			__mptcp_sync_sndbuf(sk);
+		if (__test_and_clear_bit(MPTCP_OPPR_CHECK, &msk->cb_flags))
+			mptcp_sched_oppr_check(sk);
 	}
 }
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 39dd56d882ce..32985f74ce35 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -127,6 +127,7 @@
 #define MPTCP_FLUSH_JOIN_LIST	5
 #define MPTCP_SYNC_STATE	6
 #define MPTCP_SYNC_SNDBUF	7
+#define MPTCP_OPPR_CHECK	8
 
 struct mptcp_skb_cb {
 	u64 map_seq;
@@ -930,6 +931,7 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk);
 struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk);
 int mptcp_sched_get_send(struct mptcp_sock *msk);
 int mptcp_sched_get_retrans(struct mptcp_sock *msk);
+void mptcp_sched_oppr_check(struct sock *sk);
 bool mptcp_sched_oppr_get(struct mptcp_sock *msk,
 			  struct mptcp_sched_oppr *oppr);
 
@@ -1053,6 +1055,7 @@ static inline u64 mptcp_expand_seq(u64 old_seq, u64 cur_seq, bool use_64bit)
 }
 void __mptcp_check_push(struct sock *sk, struct sock *ssk);
 void __mptcp_data_acked(struct sock *sk);
+void __mptcp_check_oppr(struct sock *sk);
 void __mptcp_error_report(struct sock *sk);
 bool mptcp_update_rcv_data_fin(struct mptcp_sock *msk, u64 data_fin_seq, bool use_64bit);
 static inline bool mptcp_data_fin_enabled(const struct mptcp_sock *msk)
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index da5fa8c0fb12..087b36381e7a 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -341,6 +341,23 @@ static u8 mptcp_sched_oppr_reason(struct sock *sk)
 	return MPTCP_SCHED_RETRANS_IDLE_APP;
 }
 
+/* called on each DATA_ACK, or when the msk socket lock is released if a process
+ * owned it: see __mptcp_check_oppr(). Under the msk data lock, with a stable
+ * subflow list. The worker takes the msk socket lock, which delays the receive
+ * path, so wake it only when the scheduler may reinject: if a subflow is idle.
+ */
+void mptcp_sched_oppr_check(struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+
+	if (!mptcp_sched_has_reinject(msk) || !mptcp_sched_oppr_reason(sk) ||
+	    !mptcp_sched_oppr_idle_ssk(msk))
+		return;
+
+	if (!test_and_set_bit(MPTCP_WORK_OPPR, &msk->flags))
+		mptcp_schedule_work(sk);
+}
+
 /* Add what @skb holds to @r, sorted by start. When @r is full, keep the lowest
  * ones: the data at snd_una is the one the peer is waiting for.
  */

-- 
2.55.0

[PATCH mptcp-next v2 5/8] mptcp: sched: reinject on an idle subflow with the default scheduler

[Matthieu Baerts (NGI0)]

From: Abdul Wasey <w453y.me@gmail.com>

Let the default scheduler use the reinjection added by the previous
patches. On an IDLE reason, it picks the active non-backup subflow with
nothing queued or in flight and the lowest RTT, and resends there only
the data held by a subflow that is gone, or slower than the idle one
with an RTT long enough for a copy to arrive at least one RTT of the
idle subflow sooner. Below that, the copies cost the idle subflow more
than they save, and on subflows of similar RTT nothing is resent. The
limit is what the idle subflow can send in the time the copies save, at
most 256 KB, or 256 KB when the slow subflow is gone.

A subflow that had a retransmission timeout without progress since is
not counted as slow: it may be broken, and the stale detection needs the
MPTCP-level timeouts that copies would avoid.

A subflow is gone when it cannot send anymore but still holds data: not
when it is joining, as it only holds a SYN, nor when it is closed, even
if its queues do not look empty after a tcp_disconnect().

With more than two subflows, only what these subflows hold is resent,
and the check on each DATA_ACK uses the same condition: the worker is
only woken when there is something worth resending.

Two subflows in netns, netem on both directions, one download, at least
5 runs each, tcp_rmem max 6 MB:

- 40M/10ms + 2M/250ms with 5% loss: BBR 27-30 -> 37-38 Mbit/s,
  CUBIC 9-35 -> 37-38.
- 400M/10ms + 15M/120ms: BBR 67-82 -> 320-332 Mbit/s, CUBIC 30-142 ->
  322-345.
- 1 MB request/response, p95 completion time in ms:
  40M/10 + 2M/250 5%: 2396 -> 317; 400M/10 + 15M/120: 530 -> 104.
  The copies are 1 to 3% of the bytes sent.
- 50M/10 + 50M/10: nothing resent, results unchanged.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/332
Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/sched.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 128 insertions(+), 12 deletions(-)

diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 087b36381e7a..b70e4a690817 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -28,6 +28,9 @@ static int mptcp_sched_default_get_send(struct mptcp_sock *msk)
 	return 0;
 }
 
+#define MPTCP_OPPR_BUDGET	(256 * 1024)
+#define MPTCP_OPPR_GONE		U32_MAX
+
 /* the active subflow with nothing to send or retransmit and the lowest RTT */
 static struct sock *mptcp_sched_oppr_idle_ssk(struct mptcp_sock *msk)
 {
@@ -53,11 +56,98 @@ static struct sock *mptcp_sched_oppr_idle_ssk(struct mptcp_sock *msk)
 	return pick;
 }
 
+/* How much earlier, in us, a copy sent now on @idle would reach the peer
+ * than the data held by @subflow: about half an RTT of each, the current one
+ * of @subflow and the base one of @idle, whose queue is empty. 0 if @subflow
+ * holds nothing, is not slower than @idle, or if the copy would not save at
+ * least one RTT of @idle: with a smaller gain, the copies cost more than they
+ * save. MPTCP_OPPR_GONE if @subflow is gone.
+ */
+static u32 mptcp_sched_oppr_gain(struct mptcp_subflow_context *subflow,
+				 const struct sock *idle)
+{
+	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+	u32 srtt, min_rtt;
+
+	/* racy on a DATA_ACK, only a hint: the worker checks again */
+	if (ssk == idle || data_race(tcp_rtx_and_write_queues_empty(ssk)))
+		return 0;
+
+	/* A joining subflow only holds its SYN, and a closed one nothing, even
+	 * if its queues look used: tcp_disconnect() moves the write_seq.
+	 */
+	if ((1 << inet_sk_state_load(ssk)) &
+	    (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_CLOSE))
+		return 0;
+
+	if (!__mptcp_subflow_active(subflow))
+		return MPTCP_OPPR_GONE;
+
+	/* A retransmission timeout without progress since: the subflow may
+	 * be broken, which the stale detection handles (it needs the
+	 * MPTCP-level timeouts that copies would avoid).
+	 */
+	if (READ_ONCE(inet_csk(ssk)->icsk_retransmits))
+		return 0;
+
+	srtt = READ_ONCE(tcp_sk(ssk)->srtt_us) >> 3;
+	/* same here, @idle is not locked */
+	min_rtt = data_race(tcp_min_rtt(tcp_sk(idle)));
+	if (READ_ONCE(ssk->sk_pacing_rate) >= READ_ONCE(idle->sk_pacing_rate) ||
+	    min_rtt == ~0U || srtt <= 3 * min_rtt)
+		return 0;
+	return (srtt - min_rtt) / 2;
+}
+
+/* The largest gain of the subflows, 0 if nothing is worth resending on @idle */
+static u32 mptcp_sched_oppr_max_gain(struct mptcp_sock *msk,
+				     const struct sock *idle)
+{
+	struct mptcp_subflow_context *subflow;
+	u32 gain = 0;
+
+	mptcp_for_each_subflow(msk, subflow)
+		gain = max(gain, mptcp_sched_oppr_gain(subflow, idle));
+	return gain;
+}
+
+/* The default scheduler's choice on an MPTCP_SCHED_RETRANS_IDLE_* reason,
+ * whichever it is: the idle subflow with the lowest RTT, if another subflow
+ * holds data worth resending on it. The limit is what the idle subflow can
+ * send in the time the copies save; a subflow that is gone saves an MPTCP
+ * RTO.
+ */
+static struct sock *mptcp_sched_oppr_default(struct mptcp_sock *msk)
+{
+	struct sock *idle;
+	u64 limit;
+	u32 gain;
+
+	idle = mptcp_sched_oppr_idle_ssk(msk);
+	if (!idle)
+		return NULL;
+
+	gain = mptcp_sched_oppr_max_gain(msk, idle);
+	if (!gain)
+		return NULL;
+
+	limit = MPTCP_OPPR_BUDGET;
+	if (gain != MPTCP_OPPR_GONE)
+		limit = min_t(u64, limit,
+			      div_u64((u64)READ_ONCE(idle->sk_pacing_rate) * gain,
+				      USEC_PER_SEC));
+	mptcp_subflow_ctx(idle)->reinject_limit = limit;
+	return idle;
+}
+
 static int mptcp_sched_default_get_retrans(struct mptcp_sock *msk)
 {
 	struct sock *ssk;
 
-	ssk = mptcp_subflow_get_retrans(msk);
+	if (msk->sched_retrans != MPTCP_SCHED_RETRANS_RTO)
+		ssk = mptcp_sched_oppr_default(msk);
+	else
+		ssk = mptcp_subflow_get_retrans(msk);
 	if (!ssk)
 		return -EINVAL;
 
@@ -263,23 +353,36 @@ int mptcp_sched_get_retrans(struct mptcp_sock *msk)
  * that already reached the peer.
  *
  * The scheduler decides on which subflow, and how much, through get_retrans
- * with the reason in msk->sched_retrans. This is done at most once per
- * snd_una value.
+ * with the reason in msk->sched_retrans. The default one resends only data
+ * held by a subflow that is gone, or that is slower than the idle one and
+ * whose RTT is long enough for a copy to arrive at least one RTT of the
+ * idle subflow sooner. Below that, the copies cost the idle subflow more
+ * than they save. This is done at most once per snd_una value, and at most
+ * what the idle subflow can send in the time saved, or MPTCP_OPPR_BUDGET for
+ * a subflow that is gone.
  */
 #define MPTCP_OPPR_MIN		(16 * 1024)
 
+/* the in-kernel default scheduler */
+static bool mptcp_sched_is_default(const struct mptcp_sock *msk)
+{
+	const struct mptcp_sched_ops *sched = READ_ONCE(msk->sched);
+
+	return !sched || sched == &mptcp_sched_default;
+}
+
 /* can the scheduler be asked to reinject: see mptcp_sched_get_reinject() */
 static bool mptcp_sched_has_reinject(const struct mptcp_sock *msk)
 {
 	const struct mptcp_sched_ops *sched = READ_ONCE(msk->sched);
 
-	return sched && sched != &mptcp_sched_default && sched->get_retrans;
+	return !sched || sched->get_retrans;
 }
 
 /* Ask the scheduler where to resend the data other subflows hold, because a
  * subflow is idle while nothing new can be sent, for @reason: see
- * MPTCP_SCHED_RETRANS_IDLE_WND. Only a scheduler with its own get_retrans is
- * asked; the default one does not reinject.
+ * MPTCP_SCHED_RETRANS_IDLE_WND. Only the default scheduler and one with its
+ * own get_retrans are asked.
  */
 static int mptcp_sched_get_reinject(struct mptcp_sock *msk, u8 reason)
 {
@@ -301,7 +404,10 @@ static int mptcp_sched_get_reinject(struct mptcp_sock *msk, u8 reason)
 		return -EOPNOTSUPP;
 
 	msk->sched_retrans = reason;
-	err = msk->sched->get_retrans(msk);
+	if (mptcp_sched_is_default(msk))
+		err = mptcp_sched_default_get_retrans(msk);
+	else
+		err = msk->sched->get_retrans(msk);
 	msk->sched_retrans = MPTCP_SCHED_RETRANS_RTO;
 	return err;
 }
@@ -344,14 +450,20 @@ static u8 mptcp_sched_oppr_reason(struct sock *sk)
 /* called on each DATA_ACK, or when the msk socket lock is released if a process
  * owned it: see __mptcp_check_oppr(). Under the msk data lock, with a stable
  * subflow list. The worker takes the msk socket lock, which delays the receive
- * path, so wake it only when the scheduler may reinject: if a subflow is idle.
+ * path, so wake it only when the scheduler may reinject: when a subflow is idle
+ * and, for the default one, when mptcp_sched_oppr_default() would pick it.
  */
 void mptcp_sched_oppr_check(struct sock *sk)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct sock *idle;
 
-	if (!mptcp_sched_has_reinject(msk) || !mptcp_sched_oppr_reason(sk) ||
-	    !mptcp_sched_oppr_idle_ssk(msk))
+	if (!mptcp_sched_has_reinject(msk) || !mptcp_sched_oppr_reason(sk))
+		return;
+
+	idle = mptcp_sched_oppr_idle_ssk(msk);
+	if (!idle || (mptcp_sched_is_default(msk) &&
+		      !mptcp_sched_oppr_max_gain(msk, idle)))
 		return;
 
 	if (!test_and_set_bit(MPTCP_WORK_OPPR, &msk->flags))
@@ -396,8 +508,9 @@ static int mptcp_sched_oppr_add_skb(const struct sk_buff *skb, u64 una,
 }
 
 /* The DSN ranges at or after @una that the subflows other than @idle still
- * hold, sent or not: the data the peer is waiting for. Sorted by start, and
- * merged: data resent before can sit on more than one subflow.
+ * hold, sent or not: the data the peer is waiting for. With the default
+ * scheduler, only from the subflows this is worth it for. Sorted by start,
+ * and merged: data resent before can sit on more than one subflow.
  */
 static int mptcp_sched_oppr_collect(struct mptcp_sock *msk, struct sock *idle,
 				    u64 una, struct mptcp_oppr_range *r)
@@ -412,6 +525,9 @@ static int mptcp_sched_oppr_collect(struct mptcp_sock *msk, struct sock *idle,
 
 		if (ssk == idle)
 			continue;
+		if (mptcp_sched_is_default(msk) &&
+		    !mptcp_sched_oppr_gain(subflow, idle))
+			continue;
 
 		lock_sock(ssk);
 		skb_rbtree_walk(skb, &ssk->tcp_rtx_queue)

-- 
2.55.0

[PATCH mptcp-next v2 6/8] mptcp: sched: let a BPF scheduler set the send and reinject limits

[Matthieu Baerts (NGI0)]

From: Abdul Wasey <w453y.me@gmail.com>

A scheduler can limit what is sent for one decision with send_limit, and
what is resent on an idle subflow with reinject_limit, but a BPF scheduler
cannot write these fields of the subflow context yet. Allow it, so that
it can set them in get_send and get_retrans.

Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
---
 net/mptcp/bpf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 53111d7a9eee..832dd807f672 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -63,6 +63,12 @@ static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log,
 		case offsetof(struct mptcp_subflow_context, avg_pacing_rate):
 			end = offsetofend(struct mptcp_subflow_context, avg_pacing_rate);
 			break;
+		case offsetof(struct mptcp_subflow_context, send_limit):
+			end = offsetofend(struct mptcp_subflow_context, send_limit);
+			break;
+		case offsetof(struct mptcp_subflow_context, reinject_limit):
+			end = offsetofend(struct mptcp_subflow_context, reinject_limit);
+			break;
 		default:
 			bpf_log(log, "no write support to mptcp_subflow_context at off %d\n",
 				off);

-- 
2.55.0

[PATCH mptcp-next v2 7/8] selftests: bpf: mptcp: test the send limit of a scheduler

[Matthieu Baerts (NGI0)]

From: Abdul Wasey <w453y.me@gmail.com>

Add bpf_limit, a round robin scheduler that sets send_limit to 4 KB on
the subflow it picks, and a test checking that no subflow gets more than
that between two calls of get_send. The scheduler measures it from the
subflow's write_seq, at each call, for the subflow it picked last time.
An MPTCP-level retransmission is not counted: it does not come from
get_send.

The data is written in 64 KB chunks, so that a dfrag is larger than the
limit. With 1.5 KB writes, as send_recv_data() does, no decision is given
more than 4 KB anyway, and the limit is not tested. Without the limit
applied in __subflow_push_pending(), a subflow got 32728 bytes in one
decision.

Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/mptcp.c     | 106 +++++++++++++++++++++
 .../testing/selftests/bpf/progs/mptcp_bpf_limit.c  |  94 ++++++++++++++++++
 2 files changed, 200 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index d77c9f8c53c7..4302c16cb6bb 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -18,6 +18,7 @@
 #include "mptcp_bpf_rr.skel.h"
 #include "mptcp_bpf_red.skel.h"
 #include "mptcp_bpf_burst.skel.h"
+#include "mptcp_bpf_limit.skel.h"
 
 #define NS_TEST "mptcp_ns"
 #define ADDR_1	"10.0.1.1"
@@ -820,6 +821,109 @@ static void test_burst(void)
 	mptcp_bpf_burst__destroy(skel);
 }
 
+struct big_send_arg {
+	int lfd;
+	int err;
+};
+
+/* send total_bytes in 64 KB writes, so that data fragments get larger than
+ * the scheduler's limit
+ */
+static void *big_send(void *data)
+{
+	struct big_send_arg *a = data;
+	static char buf[64 * 1024];
+	unsigned int sent = 0;
+	ssize_t ret;
+	int fd;
+
+	fd = accept(a->lfd, NULL, NULL);
+	if (fd < 0) {
+		a->err = -errno;
+		return NULL;
+	}
+	while (sent < total_bytes) {
+		ret = send(fd, buf, MIN(total_bytes - sent, sizeof(buf)), 0);
+		if (ret < 0 && errno == EINTR)
+			continue;
+		if (ret < 0) {
+			a->err = -errno;
+			break;
+		}
+		sent += ret;
+	}
+	close(fd);
+	return NULL;
+}
+
+/* At most 4 KB per decision, also inside a data fragment: no subflow gets
+ * more than that between two calls of the scheduler.
+ */
+static void test_limit(void)
+{
+	struct big_send_arg arg = {};
+	struct mptcp_bpf_limit *skel;
+	int server_fd = -1, client_fd = -1;
+	struct netns_obj *netns = NULL;
+	unsigned int recvd = 0;
+	struct bpf_link *link;
+	pthread_t thread;
+	char buf[64 * 1024];
+	ssize_t ret;
+
+	skel = mptcp_bpf_limit__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load: limit"))
+		return;
+
+	link = bpf_map__attach_struct_ops(skel->maps.limit);
+	if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
+		goto destroy;
+
+	netns = netns_new(NS_TEST, true);
+	if (!ASSERT_OK_PTR(netns, "netns_new"))
+		goto detach;
+	if (!ASSERT_OK(sched_init("subflow", "bpf_limit"), "sched_init"))
+		goto close_netns;
+
+	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
+	if (!ASSERT_OK_FD(server_fd, "start_mptcp_server"))
+		goto close_netns;
+	client_fd = connect_to_fd(server_fd, 0);
+	if (!ASSERT_OK_FD(client_fd, "connect_to_fd"))
+		goto close_server;
+
+	arg.lfd = server_fd;
+	if (!ASSERT_OK(pthread_create(&thread, NULL, big_send, &arg),
+		       "pthread_create"))
+		goto close_client;
+	while (recvd < total_bytes) {
+		ret = recv(client_fd, buf, sizeof(buf), 0);
+		if (ret < 0 && errno == EINTR)
+			continue;
+		if (ret <= 0)
+			break;
+		recvd += ret;
+	}
+	pthread_join(thread, NULL);
+
+	ASSERT_OK(arg.err, "send");
+	ASSERT_EQ(recvd, total_bytes, "recv");
+	CHECK(has_bytes_sent(ADDR_2), "limit", "should have bytes_sent on addr2\n");
+	ASSERT_GT(skel->bss->max_given, 0, "bytes given per decision");
+	ASSERT_LE(skel->bss->max_given, 4096, "bytes given per decision");
+
+close_client:
+	close(client_fd);
+close_server:
+	close(server_fd);
+close_netns:
+	netns_free(netns);
+detach:
+	bpf_link__destroy(link);
+destroy:
+	mptcp_bpf_limit__destroy(skel);
+}
+
 void test_mptcp(void)
 {
 	if (test__start_subtest("base"))
@@ -842,4 +946,6 @@ void test_mptcp(void)
 		test_red();
 	if (test__start_subtest("burst"))
 		test_burst();
+	if (test__start_subtest("limit"))
+		test_limit();
 }
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_limit.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_limit.c
new file mode 100644
index 000000000000..431f07d6b669
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_limit.c
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "mptcp_bpf.h"
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+#define LIMIT	4096
+
+__u64 calls = 0;
+/* the most bytes a subflow was given for one decision */
+__u32 max_given = 0;
+
+/* the subflow picked last time, by position, and its write_seq then */
+int last_pick = -1;
+__u32 last_seq = 0;
+
+SEC("struct_ops")
+void BPF_PROG(mptcp_sched_limit_init, struct mptcp_sock *msk)
+{
+}
+
+SEC("struct_ops")
+void BPF_PROG(mptcp_sched_limit_release, struct mptcp_sock *msk)
+{
+}
+
+/* round robin, at most LIMIT bytes per decision */
+SEC("struct_ops")
+int BPF_PROG(bpf_limit_get_send, struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow, *first = NULL, *pick = NULL;
+	const struct tcp_sock *tp;
+	__u64 n = calls++;
+	struct sock *ssk;
+	int i = 0, p = 0;
+
+	bpf_for_each(mptcp_subflow, subflow, (struct sock *)msk) {
+		if (i == last_pick) {
+			ssk = bpf_mptcp_subflow_tcp_sock(subflow);
+			tp = ssk ? bpf_skc_to_tcp_sock(ssk) : NULL;
+			if (tp && tp->write_seq - last_seq > max_given)
+				max_given = tp->write_seq - last_seq;
+		}
+		if (!first)
+			first = subflow;
+		if (!pick && i == (n & 1)) {
+			pick = subflow;
+			p = i;
+		}
+		i++;
+	}
+	/* a single subflow so far */
+	if (!pick) {
+		pick = first;
+		p = 0;
+	}
+	if (!pick)
+		return -1;
+
+	ssk = bpf_mptcp_subflow_tcp_sock(pick);
+	tp = ssk ? bpf_skc_to_tcp_sock(ssk) : NULL;
+	last_pick = tp ? p : -1;
+	last_seq = tp ? tp->write_seq : 0;
+
+	pick->send_limit = LIMIT;
+	mptcp_subflow_set_scheduled(pick, true);
+	return 0;
+}
+
+/* An MPTCP-level retransmission is not one of our decisions: do not count
+ * what it sends.
+ */
+SEC("struct_ops")
+int BPF_PROG(bpf_limit_get_retrans, struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow;
+
+	last_pick = -1;
+	bpf_for_each(mptcp_subflow, subflow, (struct sock *)msk) {
+		mptcp_subflow_set_scheduled(subflow, true);
+		return 0;
+	}
+	return -1;
+}
+
+SEC(".struct_ops.link")
+struct mptcp_sched_ops limit = {
+	.init		= (void *)mptcp_sched_limit_init,
+	.release	= (void *)mptcp_sched_limit_release,
+	.get_send	= (void *)bpf_limit_get_send,
+	.get_retrans	= (void *)bpf_limit_get_retrans,
+	.name		= "bpf_limit",
+};

-- 
2.55.0

[PATCH mptcp-next v2 8/8] selftests: bpf: mptcp: test a scheduler reinjecting on an idle subflow

[Matthieu Baerts (NGI0)]

From: Abdul Wasey <w453y.me@gmail.com>

Add bpf_reinject, a scheduler that sends new data on the first subflow
only, and on an IDLE reason of get_retrans marks the other subflow with a
reinject_limit of 64 KB. The second address then only carries data
resent by the core. The test checks that it did, and that get_retrans
was called with an IDLE reason; the scheduler counts its calls by reason.

Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/mptcp.c     | 24 ++++++++
 .../selftests/bpf/progs/mptcp_bpf_reinject.c       | 70 ++++++++++++++++++++++
 2 files changed, 94 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 4302c16cb6bb..723f42b45827 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -18,6 +18,7 @@
 #include "mptcp_bpf_rr.skel.h"
 #include "mptcp_bpf_red.skel.h"
 #include "mptcp_bpf_burst.skel.h"
+#include "mptcp_bpf_reinject.skel.h"
 #include "mptcp_bpf_limit.skel.h"
 
 #define NS_TEST "mptcp_ns"
@@ -821,6 +822,27 @@ static void test_burst(void)
 	mptcp_bpf_burst__destroy(skel);
 }
 
+/* New data only goes on the first subflow, so the second one only carries
+ * what the scheduler asks the core to reinject when it goes idle.
+ */
+static void test_reinject(void)
+{
+	struct mptcp_bpf_reinject *skel;
+	__u64 idle;
+
+	skel = mptcp_bpf_reinject__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load: reinject"))
+		return;
+
+	test_bpf_sched(skel->maps.reinject, "reinject", WITH_DATA, WITH_DATA);
+
+	/* calls[0] counts the MPTCP RTOs, the others the idle reasons */
+	idle = skel->bss->calls[1] + skel->bss->calls[2] + skel->bss->calls[3];
+	ASSERT_GT(idle, 0, "get_retrans called on an idle subflow");
+
+	mptcp_bpf_reinject__destroy(skel);
+}
+
 struct big_send_arg {
 	int lfd;
 	int err;
@@ -946,6 +968,8 @@ void test_mptcp(void)
 		test_red();
 	if (test__start_subtest("burst"))
 		test_burst();
+	if (test__start_subtest("reinject"))
+		test_reinject();
 	if (test__start_subtest("limit"))
 		test_limit();
 }
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_reinject.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_reinject.c
new file mode 100644
index 000000000000..3e315c7ead67
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_reinject.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "mptcp_bpf.h"
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+/* get_retrans calls, by msk->sched_retrans */
+__u64 calls[4] = {};
+
+SEC("struct_ops")
+void BPF_PROG(mptcp_sched_reinject_init, struct mptcp_sock *msk)
+{
+}
+
+SEC("struct_ops")
+void BPF_PROG(mptcp_sched_reinject_release, struct mptcp_sock *msk)
+{
+}
+
+/* new data only on the first subflow, as bpf_first */
+SEC("struct_ops")
+int BPF_PROG(bpf_reinject_get_send, struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow;
+
+	bpf_for_each(mptcp_subflow, subflow, (struct sock *)msk) {
+		if (bpf_mptcp_subflow_tcp_sock(subflow) == msk->first) {
+			mptcp_subflow_set_scheduled(subflow, true);
+			return 0;
+		}
+	}
+	return -1;
+}
+
+/* On an MPTCP RTO, resend on the first subflow. When the core asks because
+ * a subflow is idle, resend up to 64 KB of what the first subflow holds on
+ * another one, so the second address only carries reinjected data.
+ */
+SEC("struct_ops")
+int BPF_PROG(bpf_reinject_get_retrans, struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow;
+	__u8 reason = msk->sched_retrans;
+	bool idle = reason != MPTCP_SCHED_RETRANS_RTO;
+
+	if (reason < 4)
+		__sync_fetch_and_add(&calls[reason], 1);
+
+	bpf_for_each(mptcp_subflow, subflow, (struct sock *)msk) {
+		bool first = bpf_mptcp_subflow_tcp_sock(subflow) == msk->first;
+
+		if (first == idle)
+			continue;
+		if (idle)
+			subflow->reinject_limit = 64 * 1024;
+		mptcp_subflow_set_scheduled(subflow, true);
+		return 0;
+	}
+	return -1;
+}
+
+SEC(".struct_ops.link")
+struct mptcp_sched_ops reinject = {
+	.init		= (void *)mptcp_sched_reinject_init,
+	.release	= (void *)mptcp_sched_reinject_release,
+	.get_send	= (void *)bpf_reinject_get_send,
+	.get_retrans	= (void *)bpf_reinject_get_retrans,
+	.name		= "bpf_reinject",
+};

-- 
2.55.0

Re: [PATCH mptcp-next v2 8/8] selftests: bpf: mptcp: test a scheduler reinjecting on an idle subflow

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] selftests/bpf: multi-line comments in test_reinject() and bpf_reinject_get_retrans() violate BPF comment style
--

commit 85b159ecf48bf639151eb32d4b50cba4bfb3a107
Author: Abdul Wasey <w453y.me@gmail.com>

selftests: bpf: mptcp: test a scheduler reinjecting on an idle subflow

This commit adds a bpf mptcp scheduler selftest (bpf_reinject) that sends
new data only on the first subflow and reinjects up to 64 KB on another
subflow when get_retrans is invoked for an idle subflow.

> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 4302c16cb6bb4..723f42b458270 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
[ ... ]
> @@ -821,6 +822,27 @@ static void test_burst(void)
>  	mptcp_bpf_burst__destroy(skel);
>  }
>  
> +/* New data only goes on the first subflow, so the second one only carries
> + * what the scheduler asks the core to reinject when it goes idle.
> + */
> +static void test_reinject(void)
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_reinject.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_reinject.c
> new file mode 100644
> index 0000000000000..3e315c7ead67c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_reinject.c
> @@ -0,0 +1,70 @@
[ ... ]
> +/* On an MPTCP RTO, resend on the first subflow. When the core asks because
> + * a subflow is idle, resend up to 64 KB of what the first subflow holds on
> + * another one, so the second address only carries reinjected data.
> + */
> +SEC("struct_ops")
> +int BPF_PROG(bpf_reinject_get_retrans, struct mptcp_sock *msk)

[Severity: Low]
This isn't a bug, but the new multi-line comments above test_reinject() in
tools/testing/selftests/bpf/prog_tests/mptcp.c and above
bpf_reinject_get_retrans() in
tools/testing/selftests/bpf/progs/mptcp_bpf_reinject.c place comment text on
the same line as the opening /* delimiter. Could these be formatted with the
opening /* on its own line to match the bpf coding style?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-mptcp-sched-opp-retrans-v2-0-e9c4a30f6841@kernel.org?part=8

Re: [PATCH mptcp-next v2 3/8] mptcp: sched: let get_retrans reinject on an idle subflow

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mptcp: 32-bit overflow in __mptcp_oppr() and mptcp_oppr_send_range() budget math
- [Medium] mptcp: __mptcp_oppr() continues to later DSN ranges after partial send failure
--

commit be2c71f8537cecaea5b15cddbe5f79d5a824718f
Author: Abdul Wasey <w453y.me@gmail.com>

mptcp: sched: let get_retrans reinject on an idle subflow

This commit extends the MPTCP scheduler get_retrans callback to support
opportunistic reinjection on an idle subflow when no new data can be sent.
The core collects the unacknowledged DSN ranges held by other subflows and
resends them on the selected subflow up to reinject_limit and one cwnd.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/332 [1]

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index d960ab9c76b32..7117786e52513 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3057,6 +3057,92 @@ static int __mptcp_push_retrans(struct sock *sk, struct mptcp_data_frag *dfrag,
>  	return len;
>  }
>  
> +/* resend [start, end) on @ssk from the msk rtx queue; < 0 on a send error */
> +static int mptcp_oppr_send_range(struct sock *sk, struct sock *ssk, u64 start,
> +				 u64 end, int budget,
> +				 struct mptcp_sendmsg_info *info)
> +{
> +	struct mptcp_data_frag *dfrag;
> +	int copied = 0, ret;
> +
> +	list_for_each_entry(dfrag, &mptcp_sk(sk)->rtx_queue, list) {
> +		if (!dfrag->already_sent || !before64(dfrag->data_seq, end))
> +			break;
> +		if (!after64(dfrag->data_seq + dfrag->already_sent, start))
> +			continue;
> +
> +		info->sent = before64(dfrag->data_seq, start) ?
> +			     start - dfrag->data_seq : 0;
> +		info->limit = min_t(u64, dfrag->already_sent,
> +				    end - dfrag->data_seq);
> +		info->limit = min_t(u64, info->limit,
> +				    info->sent + budget - copied);
> +		while (info->sent < info->limit && copied < budget) {
> +			ret = mptcp_sendmsg_frag(sk, ssk, dfrag, info);
> +			if (ret <= 0)
> +				return copied ? : -1;
> +			copied += ret;
> +			info->sent += ret;
> +		}
> +		if (copied >= budget)
> +			break;
> +	}
> +	return copied;
> +}
> +
> +/* Opportunistic reinjection: the scheduling part is in sched.c */
> +static void __mptcp_oppr(struct sock *sk)
> +{
> +	struct mptcp_sendmsg_info info = { .data_lock_held = true, };
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	int copied = 0, budget, ret, i;
> +	struct mptcp_sched_oppr oppr;
> +	struct sock *ssk;
> +
> +	mptcp_data_lock(sk);
> +	__mptcp_clean_una_wakeup(sk);
> +	oppr.una = msk->snd_una;
> +	mptcp_data_unlock(sk);
> +
> +	if (!mptcp_sched_oppr_get(msk, &oppr))
> +		return;
> +
> +	ssk = oppr.ssk;
> +	lock_sock(ssk);
> +	/* the scheduler's limit, and no more than one cwnd */
> +	budget = min_t(u64, oppr.limit,
> +		       tcp_snd_cwnd(tcp_sk(ssk)) * tcp_sk(ssk)->mss_cache);

[Severity: Medium]
Can the budget calculation in __mptcp_oppr() and mptcp_oppr_send_range()
overflow 32-bit integers when a scheduler sets a large subflow->reinject_limit
on a subflow with a large cwnd?

In __mptcp_oppr(), tcp_snd_cwnd(tcp_sk(ssk)) and tcp_sk(ssk)->mss_cache are
both u32, so their multiplication is evaluated in 32-bit unsigned arithmetic
before min_t(u64, ...) promotes the product to u64, wrapping if the product
reaches 2^32.

In addition, oppr.limit is u32 while budget and copied are signed int. If both
oppr.limit and the cwnd * mss_cache product are >= 0x80000000U (for example
when a scheduler sets reinject_limit = U32_MAX on a high-BDP path), assigning
the min_t(u64, ...) result to signed int budget makes budget negative. The
loop condition copied < budget in __mptcp_oppr() is then immediately false,
skipping reinjection without updating msk->oppr_una.

Similarly, in mptcp_oppr_send_range():

	info->limit = min_t(u64, info->limit,
			    info->sent + budget - copied);

info->sent + budget - copied is evaluated as a signed 32-bit int before
min_t(u64, ...) casts it to u64. If budget is near INT_MAX and info->sent >
copied (such as when start > dfrag->data_seq), the signed addition overflows
to a negative int, which sign-extends to a large u64 and fails to clamp
info->limit to the remaining budget.

> +
> +	spin_lock_bh(&msk->fallback_lock);
> +	if (__mptcp_check_fallback(msk) || !msk->allow_subflows)
> +		goto unlock;
> +
> +	for (i = 0; i < oppr.n && copied < budget; i++) {
> +		ret = mptcp_oppr_send_range(sk, ssk, oppr.r[i].start,
> +					    oppr.r[i].end, budget - copied,
> +					    &info);
> +		if (ret < 0)
> +			break;
> +		copied += ret;
> +	}

[Severity: Medium]
If mptcp_sendmsg_frag() fails partway through sending a DSN range in
mptcp_oppr_send_range(), should __mptcp_oppr() stop rather than continuing to
subsequent ranges in oppr.r[]?

In mptcp_oppr_send_range(), when mptcp_sendmsg_frag() returns <= 0 (for
example -ENOMEM from sk_wmem_schedule() or 0 when hitting mptcp_wnd_end(msk))
after an earlier fragment in the same range already sent data (copied > 0),
it returns the positive copied count via:

	if (ret <= 0)
		return copied ? : -1;

Because the loop in __mptcp_oppr() only breaks on ret < 0, a partial send on
oppr.r[i] causes __mptcp_oppr() to advance to oppr.r[i + 1]. If a later,
smaller range succeeds, higher-DSN data is transmitted while leaving an unsent
gap in the earlier range that holds back snd_una, and msk->oppr_una is still
recorded as oppr.una so the failed portion is not retried for this snd_una.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-mptcp-sched-opp-retrans-v2-0-e9c4a30f6841@kernel.org?part=3

Re: [PATCH mptcp-next v2 7/8] selftests: bpf: mptcp: test the send limit of a scheduler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] selftests/bpf: deprecated CHECK() macro used in test_limit()
- [Low] selftests/bpf: non-standard multi-line comment style in mptcp limit test
--

commit fb18275d8cce4c512f78d41262ea65272c01b932
Author: Abdul Wasey <w453y.me@gmail.com>

selftests: bpf: mptcp: test the send limit of a scheduler

This commit adds a bpf_limit round-robin MPTCP scheduler that sets send_limit
to 4 KB on the selected subflow and a selftest verifying that no subflow
receives more than 4 KB between two get_send calls when sending 64 KB chunks.

> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index d77c9f8c53c75..4302c16cb6bb4 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
[ ... ]
> @@ -820,6 +821,109 @@ static void test_burst(void)
>  	mptcp_bpf_burst__destroy(skel);
>  }
>  
> +struct big_send_arg {
> +	int lfd;
> +	int err;
> +};
> +
> +/* send total_bytes in 64 KB writes, so that data fragments get larger than
> + * the scheduler's limit
> + */
> +static void *big_send(void *data)
[ ... ]
> +/* At most 4 KB per decision, also inside a data fragment: no subflow gets
> + * more than that between two calls of the scheduler.
> + */
> +static void test_limit(void)

[Severity: Low]
This isn't a bug, but in BPF code and selftests, should multi-line comments
above big_send() and test_limit() place the opening /* on its own line with
the comment text starting on the next line?

> +{
[ ... ]
> +	ASSERT_OK(arg.err, "send");
> +	ASSERT_EQ(recvd, total_bytes, "recv");
> +	CHECK(has_bytes_sent(ADDR_2), "limit", "should have bytes_sent on addr2\n");

[Severity: Low]
Since CHECK() is deprecated in BPF selftests, could test_limit() use
ASSERT_OK(has_bytes_sent(ADDR_2), "has_bytes_sent addr2") here instead,
matching the other ASSERT_*() calls in this function?

> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_limit.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_limit.c
> new file mode 100644
> index 0000000000000..431f07d6b6696
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_limit.c
> @@ -0,0 +1,94 @@
[ ... ]
> +/* An MPTCP-level retransmission is not one of our decisions: do not count
> + * what it sends.
> + */
> +SEC("struct_ops")
> +int BPF_PROG(bpf_limit_get_retrans, struct mptcp_sock *msk)

[Severity: Low]
Similarly, should the multi-line comment above bpf_limit_get_retrans() have
the opening /* on its own line to follow BPF coding style?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-mptcp-sched-opp-retrans-v2-0-e9c4a30f6841@kernel.org?part=7

Re: [PATCH mptcp-next v2 5/8] mptcp: sched: reinject on an idle subflow with the default scheduler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mptcp: sched: u32 overflow and underflow in mptcp_sched_oppr_gain()
- [Medium] mptcp: sched: false MPTCP_OPPR_GONE in mptcp_sched_oppr_gain()
--

commit dddb76c1077214e20753883f9b0151980b6f6ff0
Author: Abdul Wasey <w453y.me@gmail.com>

mptcp: sched: reinject on an idle subflow with the default scheduler

This commit enables opportunistic reinjection on an idle subflow in the
default MPTCP scheduler when another subflow is gone or slow enough that a
copy would arrive at least one RTT of the idle subflow earlier, up to a
256 KB budget.

> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index 087b36381e7a4..b70e4a6908177 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -28,6 +28,9 @@ static int mptcp_sched_default_get_send(struct mptcp_sock *msk)
>  	return 0;
>  }
>  
> +#define MPTCP_OPPR_BUDGET	(256 * 1024)
> +#define MPTCP_OPPR_GONE		U32_MAX
> +
>  /* the active subflow with nothing to send or retransmit and the lowest RTT */
>  static struct sock *mptcp_sched_oppr_idle_ssk(struct mptcp_sock *msk)
>  {
> @@ -53,11 +56,98 @@ static struct sock *mptcp_sched_oppr_idle_ssk(struct mptcp_sock *msk)
>  	return pick;
>  }
>  
> +/* How much earlier, in us, a copy sent now on @idle would reach the peer
> + * than the data held by @subflow: about half an RTT of each, the current one
> + * of @subflow and the base one of @idle, whose queue is empty. 0 if @subflow
> + * holds nothing, is not slower than @idle, or if the copy would not save at
> + * least one RTT of @idle: with a smaller gain, the copies cost more than they
> + * save. MPTCP_OPPR_GONE if @subflow is gone.
> + */
> +static u32 mptcp_sched_oppr_gain(struct mptcp_subflow_context *subflow,
> +				 const struct sock *idle)
> +{
> +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +	u32 srtt, min_rtt;
> +
> +	/* racy on a DATA_ACK, only a hint: the worker checks again */
> +	if (ssk == idle || data_race(tcp_rtx_and_write_queues_empty(ssk)))
> +		return 0;
> +
> +	/* A joining subflow only holds its SYN, and a closed one nothing, even
> +	 * if its queues look used: tcp_disconnect() moves the write_seq.
> +	 */
> +	if ((1 << inet_sk_state_load(ssk)) &
> +	    (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_CLOSE))
> +		return 0;
> +
> +	if (!__mptcp_subflow_active(subflow))
> +		return MPTCP_OPPR_GONE;

[Severity: Medium]
Can mptcp_sched_oppr_gain() return a false MPTCP_OPPR_GONE for a subflow
that holds no MPTCP data?

When the initial subflow msk->first is closed while the MPTCP connection
stays open on other subflows (for example via mptcp_pm_rm_addr_or_subflow(),
mptcp_nl_cmd_sf_destroy(), or __mptcp_close_subflow() on a peer FIN),
__mptcp_close_ssk() leaves msk->first in msk->conn_list when
!msk->free_first and calls __mptcp_subflow_disconnect(). If msk->first is
in TCP_ESTABLISHED or TCP_CLOSE_WAIT, __mptcp_subflow_disconnect() calls
tcp_shutdown(ssk, SEND_SHUTDOWN), moving ssk to TCP_FIN_WAIT1 or
TCP_LAST_ACK and queuing a TCP FIN skb on ssk->tcp_rtx_queue.

Even when msk->first holds only that unacknowledged TCP FIN and no MPTCP
data, tcp_rtx_and_write_queues_empty(ssk) is false, its state is not in
TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_CLOSE, and __mptcp_subflow_active()
is false. Similarly, on the lockless DATA_ACK path in
mptcp_sched_oppr_check(), an outgoing MP_JOIN subflow whose SYN was in
tcp_rtx_queue when tcp_rtx_and_write_queues_empty(ssk) ran can transition
to TCP_ESTABLISHED before inet_sk_state_load(ssk) while
__mptcp_subflow_active(subflow) is still false waiting for the 4th ACK.

When another active subflow has data in flight alongside an idle subflow,
wouldn't this cause mptcp_sched_oppr_max_gain() to return MPTCP_OPPR_GONE,
waking mptcp_worker on every DATA_ACK in mptcp_sched_oppr_check() and
forcing reinject_limit to the full MPTCP_OPPR_BUDGET in
mptcp_sched_oppr_default()?

> +
> +	/* A retransmission timeout without progress since: the subflow may
> +	 * be broken, which the stale detection handles (it needs the
> +	 * MPTCP-level timeouts that copies would avoid).
> +	 */
> +	if (READ_ONCE(inet_csk(ssk)->icsk_retransmits))
> +		return 0;
> +
> +	srtt = READ_ONCE(tcp_sk(ssk)->srtt_us) >> 3;
> +	/* same here, @idle is not locked */
> +	min_rtt = data_race(tcp_min_rtt(tcp_sk(idle)));
> +	if (READ_ONCE(ssk->sk_pacing_rate) >= READ_ONCE(idle->sk_pacing_rate) ||
> +	    min_rtt == ~0U || srtt <= 3 * min_rtt)
> +		return 0;
> +	return (srtt - min_rtt) / 2;
> +}

[Severity: Medium]
Can 3 * min_rtt overflow u32 and cause (srtt - min_rtt) / 2 to underflow
in mptcp_sched_oppr_gain()?

In net/ipv4/tcp_input.c, tcp_ack_update_rtt() -> tcp_rtt_tsopt_us()
accepts TCP Timestamp deltas up to INT_MAX / (USEC_PER_SEC / TCP_TS_HZ)
(or INT_MAX us with microsecond timestamps), and tcp_update_rtt_min()
stores the sample in tp->rtt_min without capping it to TCP_RTO_MAX.

If idle's min_rtt is in [0x55555556, 0x5fffffff], 3 * min_rtt wraps around
in 32-bit unsigned arithmetic to a small value. Any ssk whose srtt is
larger than that wrapped value but smaller than min_rtt will bypass the
srtt <= 3 * min_rtt check, and (srtt - min_rtt) / 2 will underflow u32
into ~1.43 billion microseconds, saturating reinject_limit to
MPTCP_OPPR_BUDGET in mptcp_sched_oppr_default().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-mptcp-sched-opp-retrans-v2-0-e9c4a30f6841@kernel.org?part=5