mptcp: sched: penalise a slow subflow (#345, first cut for your lab)

[PATCH mptcp-next 3/3] DO-NOT-MERGE: mptcp: sched: penalise counters

Posted by Shardul Bankar 1 month, 2 weeks ago

Instrumentation for validating the two preceding patches; not for merge.

Adds two MPTcpExt SNMP counters:
- CwndPenalized: times a subflow cwnd was actually halved;
- PenalCandidate: times the rate trigger picked a slow subflow.

Together they separate "the guard held the penalty back" (PenalCandidate
high, CwndPenalized ~0) from "the trigger never fired" (both ~0), which is
what the receive-window-limited case needs to be read correctly.

Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
 net/mptcp/mib.c      | 2 ++
 net/mptcp/mib.h      | 2 ++
 net/mptcp/protocol.c | 8 ++++++++
 3 files changed, 12 insertions(+)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index d9bd4f4afcc0..1299613e183b 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -88,6 +88,8 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("BacklogDrop", MPTCP_MIB_BACKLOGDROP),
 	SNMP_MIB_ITEM("RcvPruned", MPTCP_MIB_RCVPRUNED),
 	SNMP_MIB_ITEM("OfoPruned", MPTCP_MIB_OFO_PRUNED),
+	SNMP_MIB_ITEM("CwndPenalized", MPTCP_MIB_CWNDPENALIZED),
+	SNMP_MIB_ITEM("PenalCandidate", MPTCP_MIB_PENALCAND),
 };
 
 /* mptcp_mib_alloc - allocate percpu mib counters
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 18f35f7e0a2d..93b3a7e9584f 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -91,6 +91,8 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_BACKLOGDROP,		/* Backlog over memory limit */
 	MPTCP_MIB_RCVPRUNED,		/* Dropped due to memory constrains */
 	MPTCP_MIB_OFO_PRUNED,		/* MPTCP-level OoO queue pruned */
+	MPTCP_MIB_CWNDPENALIZED,	/* DEBUG: subflow cwnd halved by the scheduler (#345) */
+	MPTCP_MIB_PENALCAND,		/* DEBUG: picker chose a slow (low-rate) subflow */
 	__MPTCP_MIB_MAX
 };
 
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7cbc5aa17e22..80866f09831a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1607,6 +1607,7 @@ static void mptcp_penalise_cwnd(struct sock *ssk)
 	subflow->penalise = false;
 	subflow->last_penalise = tcp_jiffies32;
 	tcp_snd_cwnd_set(tp, max_t(u32, cwnd >> 1, 2));
+	MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_CWNDPENALIZED);
 	if (cwnd >= tp->snd_ssthresh)
 		tp->snd_ssthresh = max_t(u32, tp->snd_ssthresh >> 1, 2);
 }
@@ -1694,6 +1695,13 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 	 * once per RTT.
 	 */
 	subflow = mptcp_subflow_ctx(ssk);
+	/* DEBUG: count how often the trigger picks a slow path, so a gated-off
+	 * run (PenalCandidate high, CwndPenalized 0) is distinguishable from one
+	 * where the trigger never fired.
+	 */
+	if (fastest && ssk != fastest &&
+	    (u64)subflow->avg_pacing_rate * MPTCP_PENALISE_RATE_RATIO < max_pace)
+		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_PENALCAND);
 	subflow->penalise = fastest && ssk != fastest &&
 			    (u64)subflow->avg_pacing_rate * MPTCP_PENALISE_RATE_RATIO < max_pace &&
 			    inet_csk(ssk)->icsk_ca_state == TCP_CA_Open &&

-- 
2.34.1

Re: [PATCH mptcp-next 3/3] DO-NOT-MERGE: mptcp: sched: penalise counters

Posted by Matthieu Baerts 1 month, 2 weeks ago

Hi Shardul,

On 26/07/2026 07:55, Shardul Bankar wrote:
> Instrumentation for validating the two preceding patches; not for merge.
> 
> Adds two MPTcpExt SNMP counters:
> - CwndPenalized: times a subflow cwnd was actually halved;
> - PenalCandidate: times the rate trigger picked a slow subflow.

I think at least the first counter is interesting, probably the second
one as well, no?

Generally, if you need counters during the development, they might be
needed to debug issues. But here with the scheduler, maybe the tracing
are better. Did you use "trace_mptcp_subflow_get_send"?

> Together they separate "the guard held the penalty back" (PenalCandidate
> high, CwndPenalized ~0) from "the trigger never fired" (both ~0), which is
> what the receive-window-limited case needs to be read correctly.
> 
> Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  net/mptcp/mib.c      | 2 ++
>  net/mptcp/mib.h      | 2 ++
>  net/mptcp/protocol.c | 8 ++++++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
> index d9bd4f4afcc0..1299613e183b 100644
> --- a/net/mptcp/mib.c
> +++ b/net/mptcp/mib.c
> @@ -88,6 +88,8 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>  	SNMP_MIB_ITEM("BacklogDrop", MPTCP_MIB_BACKLOGDROP),
>  	SNMP_MIB_ITEM("RcvPruned", MPTCP_MIB_RCVPRUNED),
>  	SNMP_MIB_ITEM("OfoPruned", MPTCP_MIB_OFO_PRUNED),
> +	SNMP_MIB_ITEM("CwndPenalized", MPTCP_MIB_CWNDPENALIZED),
> +	SNMP_MIB_ITEM("PenalCandidate", MPTCP_MIB_PENALCAND),
>  };
>  
>  /* mptcp_mib_alloc - allocate percpu mib counters
> diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
> index 18f35f7e0a2d..93b3a7e9584f 100644
> --- a/net/mptcp/mib.h
> +++ b/net/mptcp/mib.h
> @@ -91,6 +91,8 @@ enum linux_mptcp_mib_field {
>  	MPTCP_MIB_BACKLOGDROP,		/* Backlog over memory limit */
>  	MPTCP_MIB_RCVPRUNED,		/* Dropped due to memory constrains */
>  	MPTCP_MIB_OFO_PRUNED,		/* MPTCP-level OoO queue pruned */
> +	MPTCP_MIB_CWNDPENALIZED,	/* DEBUG: subflow cwnd halved by the scheduler (#345) */
> +	MPTCP_MIB_PENALCAND,		/* DEBUG: picker chose a slow (low-rate) subflow */
>  	__MPTCP_MIB_MAX
>  };
>  
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7cbc5aa17e22..80866f09831a 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1607,6 +1607,7 @@ static void mptcp_penalise_cwnd(struct sock *ssk)
>  	subflow->penalise = false;
>  	subflow->last_penalise = tcp_jiffies32;
>  	tcp_snd_cwnd_set(tp, max_t(u32, cwnd >> 1, 2));
> +	MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_CWNDPENALIZED);
>  	if (cwnd >= tp->snd_ssthresh)
>  		tp->snd_ssthresh = max_t(u32, tp->snd_ssthresh >> 1, 2);
>  }
> @@ -1694,6 +1695,13 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>  	 * once per RTT.
>  	 */
>  	subflow = mptcp_subflow_ctx(ssk);
> +	/* DEBUG: count how often the trigger picks a slow path, so a gated-off
> +	 * run (PenalCandidate high, CwndPenalized 0) is distinguishable from one
> +	 * where the trigger never fired.
> +	 */
> +	if (fastest && ssk != fastest &&
> +	    (u64)subflow->avg_pacing_rate * MPTCP_PENALISE_RATE_RATIO < max_pace)

If this counter is interesting, you could have a variable for penal_cand
and use it below.

> +		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_PENALCAND);
>  	subflow->penalise = fastest && ssk != fastest &&
>  			    (u64)subflow->avg_pacing_rate * MPTCP_PENALISE_RATE_RATIO < max_pace &&
>  			    inet_csk(ssk)->icsk_ca_state == TCP_CA_Open &&
> 

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next 3/3] DO-NOT-MERGE: mptcp: sched: penalise counters

Posted by Shardul Bankar 1 month, 1 week ago

Hi Matt,

On Wed, 2026-07-29 at 13:50 +0200, Matthieu Baerts wrote:
> Hi Shardul,
> 
> On 26/07/2026 07:55, Shardul Bankar wrote:
> > Instrumentation for validating the two preceding patches; not for
> > merge.
> > 
> > Adds two MPTcpExt SNMP counters:
> > - CwndPenalized: times a subflow cwnd was actually halved;
> > - PenalCandidate: times the rate trigger picked a slow subflow.
> 
> I think at least the first counter is interesting, probably the
> second
> one as well, no?
> 
> Generally, if you need counters during the development, they might be
> needed to debug issues. But here with the scheduler, maybe the
> tracing
> are better. Did you use "trace_mptcp_subflow_get_send"?
> 

I did not. Looking at it now, it fires before the penalise decision and
does not see the halving, so it would need a small extension (a new
field, or a tracepoint at the halving point) to show what these
counters show. Which would you prefer for v2: keep the DO-NOT-MERGE
counters, or add that tracepoint? I will go with your call.

> > 
> >  
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 7cbc5aa17e22..80866f09831a 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1607,6 +1607,7 @@ static void mptcp_penalise_cwnd(struct sock
> > *ssk)
> >         subflow->penalise = false;
> >         subflow->last_penalise = tcp_jiffies32;
> >         tcp_snd_cwnd_set(tp, max_t(u32, cwnd >> 1, 2));
> > +       MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_CWNDPENALIZED);
> >         if (cwnd >= tp->snd_ssthresh)
> >                 tp->snd_ssthresh = max_t(u32, tp->snd_ssthresh >>
> > 1, 2);
> >  }
> > @@ -1694,6 +1695,13 @@ struct sock *mptcp_subflow_get_send(struct
> > mptcp_sock *msk)
> >          * once per RTT.
> >          */
> >         subflow = mptcp_subflow_ctx(ssk);
> > +       /* DEBUG: count how often the trigger picks a slow path, so
> > a gated-off
> > +        * run (PenalCandidate high, CwndPenalized 0) is
> > distinguishable from one
> > +        * where the trigger never fired.
> > +        */
> > +       if (fastest && ssk != fastest &&
> > +           (u64)subflow->avg_pacing_rate *
> > MPTCP_PENALISE_RATE_RATIO < max_pace)
> 
> If this counter is interesting, you could have a variable for
> penal_cand
> and use it below.
> 

Got it, a single penal_cand local will now feed both the counter and
the penalise decision.

Thanks,
Shardul

Re: [PATCH mptcp-next 3/3] DO-NOT-MERGE: mptcp: sched: penalise counters

Posted by Matthieu Baerts 1 month, 1 week ago

Hi Shardul,

Thank you for your reply!

On 07/08/2026 17:19, Shardul Bankar wrote:
> Hi Matt,
> 
> On Wed, 2026-07-29 at 13:50 +0200, Matthieu Baerts wrote:
>> Hi Shardul,
>>
>> On 26/07/2026 07:55, Shardul Bankar wrote:
>>> Instrumentation for validating the two preceding patches; not for
>>> merge.
>>>
>>> Adds two MPTcpExt SNMP counters:
>>> - CwndPenalized: times a subflow cwnd was actually halved;
>>> - PenalCandidate: times the rate trigger picked a slow subflow.
>>
>> I think at least the first counter is interesting, probably the
>> second
>> one as well, no?
>>
>> Generally, if you need counters during the development, they might be
>> needed to debug issues. But here with the scheduler, maybe the
>> tracing
>> are better. Did you use "trace_mptcp_subflow_get_send"?
>>
> 
> I did not. Looking at it now, it fires before the penalise decision and
> does not see the halving, so it would need a small extension (a new
> field, or a tracepoint at the halving point) to show what these
> counters show. Which would you prefer for v2: keep the DO-NOT-MERGE
> counters, or add that tracepoint? I will go with your call.

Maybe both?

If you can add a new field for the tracing, that would help with the
development. The MIB counter would help with the debugging.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.