:p
atchew
Login
Addressing comments reported by Clashiko [1], but not detected before with the AI review tools using a different model version. Note: the last comment from the 3rd patch has not been addressed, as it is not clear to me whether this comment is valid: ---------------------------------- 8< ---------------------------------- > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 72b1fa3ca71c..5f7d8340a3d9 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c [ ... ] > @@ -682,6 +693,16 @@ static void __mptcp_add_backlog(struct sock *sk, > return; > } > > + /* Similar additional allowance as plain TCP. */ > + limit = READ_ONCE(sk->sk_rcvbuf); > + limit += (limit >> 1) + 64 * 1024; > + limit = min_t(u64, limit, UINT_MAX); > + if (msk->backlog_len > limit && !__mptcp_check_fallback(msk)) { > + __MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_BACKLOGDROP); > + kfree_skb_reason(skb, SKB_DROP_REASON_SOCKET_BACKLOG); > + return; > + } > + [Low] The check compares the current backlog_len against limit before the incoming skb's truesize is added (the truesize is folded in at the account: label further down). Was that intentional? TCP's sk_add_backlog() analog compares queue_size + skb->truesize against the limit, so a single skb can push msk->backlog_len past limit here and the drop only fires on the next admission. The overshoot is bounded by one skb's truesize and the commit message already notes "This is not a complete fix for the stall issue, as the drop strategy needs refinements that will come in the next patches.", so perhaps this is one of the follow-up items? ---------------------------------- 8< ---------------------------------- Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260724-net-next-mptcp-oooq-pruning-v1-0-5dd4dec63a54%40kernel.org Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Matthieu Baerts (NGI0) (2): Squash to "mptcp: explicitly drop over memory limits" Squash to "mptcp: implemented OoO queue pruning" net/mptcp/mib.c | 2 +- net/mptcp/mib.h | 4 ++-- net/mptcp/options.c | 6 +++++- net/mptcp/protocol.c | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) --- base-commit: 23c7cb4262d954b810468b0e661d48af82b418d5 change-id: 20260730-mptcp-squash-ooo-pruning-53d668d4c2aa Best regards, -- Matthieu Baerts (NGI0) <matttbe@kernel.org>
Address comments from Clashiko [1]: - mib: typo: "constrains" -> "constraints". - mptcp_over_limit: precise it is not only 0-win, but retrans, dup or old acks. - mptcp_over_limit: bump LINUX_MIB_TCPRCVQDROP, as previously discussed in [2]. Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260724-net-next-mptcp-oooq-pruning-v1-0-5dd4dec63a54%40kernel.org?part=3 [1] Link: https://lore.kernel.org/b5244dd4-205d-4abd-8ea5-fd879a97038f@redhat.com Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/mib.h | 2 +- net/mptcp/options.c | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/mib.h +++ b/net/mptcp/mib.h @@ -XXX,XX +XXX,XX @@ enum linux_mptcp_mib_field { MPTCP_MIB_FALLBACKFAILED, /* Can't fallback due to msk status */ MPTCP_MIB_WINPROBE, /* MPTCP-level zero window probe */ MPTCP_MIB_BACKLOGDROP, /* Backlog over memory limit */ - MPTCP_MIB_RCVPRUNED, /* Dropped due to memory constrains */ + MPTCP_MIB_RCVPRUNED, /* Dropped due to memory constraints */ MPTCP_MIB_OFO_PRUNED, /* MPTCP-level OoO queue pruned */ __MPTCP_MIB_MAX }; diff --git a/net/mptcp/options.c b/net/mptcp/options.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -XXX,XX +XXX,XX @@ static bool mptcp_over_limit(struct sock *sk, struct sock *ssk, if (likely(mem <= READ_ONCE(sk->sk_rcvbuf))) return false; - /* Avoid silently dropping pure acks, fin or zero win probes. */ + /* Avoid silently dropping pure acks, fin or already-acked segments. */ if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq || TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN || !after(TCP_SKB_CB(skb)->end_seq, tcp_sk(ssk)->rcv_nxt)) @@ -XXX,XX +XXX,XX @@ static bool mptcp_over_limit(struct sock *sk, struct sock *ssk, /* Dropped due to memory constraints, schedule an ack. */ inet_csk(ssk)->icsk_ack.pending |= ICSK_ACK_NOMEM | ICSK_ACK_NOW; inet_csk_schedule_ack(ssk); + + /* In fallback mode: skb is dropped before the TCP recv queue. */ + MPTCP_INC_STATS(sock_net(sk), LINUX_MIB_TCPRCVQDROP); + return true; } -- 2.53.0
Uniform the new counter with the other OFO ones. Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260724-net-next-mptcp-oooq-pruning-v1-0-5dd4dec63a54%40kernel.org?part=5 Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/mib.c | 2 +- net/mptcp/mib.h | 2 +- net/mptcp/protocol.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/mib.c +++ b/net/mptcp/mib.c @@ -XXX,XX +XXX,XX @@ static const struct snmp_mib mptcp_snmp_list[] = { SNMP_MIB_ITEM("WinProbe", MPTCP_MIB_WINPROBE), 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("OFOPruned", MPTCP_MIB_OFOPRUNED), }; /* mptcp_mib_alloc - allocate percpu mib counters diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/mib.h +++ b/net/mptcp/mib.h @@ -XXX,XX +XXX,XX @@ enum linux_mptcp_mib_field { MPTCP_MIB_WINPROBE, /* MPTCP-level zero window probe */ MPTCP_MIB_BACKLOGDROP, /* Backlog over memory limit */ MPTCP_MIB_RCVPRUNED, /* Dropped due to memory constraints */ - MPTCP_MIB_OFO_PRUNED, /* MPTCP-level OoO queue pruned */ + MPTCP_MIB_OFOPRUNED, /* MPTCP-level OoO queue pruned */ __MPTCP_MIB_MAX }; diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -XXX,XX +XXX,XX @@ static bool mptcp_prune_ofo_queue(struct sock *sk, u64 seq) } while (node); if (pruned) - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFO_PRUNED); + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOPRUNED); out: mem = (unsigned int)sk_rmem_alloc_get(sk); -- 2.53.0
Addressing comments reported by Clashiko [1], but not detected before with the AI review tools using a different model version. Note: the last comment from the 3rd patch has not been addressed, as it is not clear to me whether this comment is valid: ---------------------------------- 8< ---------------------------------- > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 72b1fa3ca71c..5f7d8340a3d9 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c [ ... ] > @@ -682,6 +693,16 @@ static void __mptcp_add_backlog(struct sock *sk, > return; > } > > + /* Similar additional allowance as plain TCP. */ > + limit = READ_ONCE(sk->sk_rcvbuf); > + limit += (limit >> 1) + 64 * 1024; > + limit = min_t(u64, limit, UINT_MAX); > + if (msk->backlog_len > limit && !__mptcp_check_fallback(msk)) { > + __MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_BACKLOGDROP); > + kfree_skb_reason(skb, SKB_DROP_REASON_SOCKET_BACKLOG); > + return; > + } > + [Low] The check compares the current backlog_len against limit before the incoming skb's truesize is added (the truesize is folded in at the account: label further down). Was that intentional? TCP's sk_add_backlog() analog compares queue_size + skb->truesize against the limit, so a single skb can push msk->backlog_len past limit here and the drop only fires on the next admission. The overshoot is bounded by one skb's truesize and the commit message already notes "This is not a complete fix for the stall issue, as the drop strategy needs refinements that will come in the next patches.", so perhaps this is one of the follow-up items? ---------------------------------- 8< ---------------------------------- Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260724-net-next-mptcp-oooq-pruning-v1-0-5dd4dec63a54%40kernel.org Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Changes in v2: - patch 1: s/MPTCP_INC_STATS/NET_INC_STATS/ - Link to v1: https://patch.msgid.link/20260731-mptcp-squash-ooo-pruning-v1-0-a5f5115e1e24@kernel.org --- Matthieu Baerts (NGI0) (2): Squash to "mptcp: explicitly drop over memory limits" Squash to "mptcp: implemented OoO queue pruning" net/mptcp/mib.c | 2 +- net/mptcp/mib.h | 4 ++-- net/mptcp/options.c | 6 +++++- net/mptcp/protocol.c | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) --- base-commit: 23c7cb4262d954b810468b0e661d48af82b418d5 change-id: 20260730-mptcp-squash-ooo-pruning-53d668d4c2aa Best regards, -- Matthieu Baerts (NGI0) <matttbe@kernel.org>
Address comments from Clashiko [1]: - mib: typo: "constrains" -> "constraints". - mptcp_over_limit: precise it is not only 0-win, but retrans, dup or old acks. - mptcp_over_limit: bump LINUX_MIB_TCPRCVQDROP, as previously discussed in [2]. Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260724-net-next-mptcp-oooq-pruning-v1-0-5dd4dec63a54%40kernel.org?part=3 [1] Link: https://lore.kernel.org/b5244dd4-205d-4abd-8ea5-fd879a97038f@redhat.com Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/mib.h | 2 +- net/mptcp/options.c | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/mib.h +++ b/net/mptcp/mib.h @@ -XXX,XX +XXX,XX @@ enum linux_mptcp_mib_field { MPTCP_MIB_FALLBACKFAILED, /* Can't fallback due to msk status */ MPTCP_MIB_WINPROBE, /* MPTCP-level zero window probe */ MPTCP_MIB_BACKLOGDROP, /* Backlog over memory limit */ - MPTCP_MIB_RCVPRUNED, /* Dropped due to memory constrains */ + MPTCP_MIB_RCVPRUNED, /* Dropped due to memory constraints */ MPTCP_MIB_OFO_PRUNED, /* MPTCP-level OoO queue pruned */ __MPTCP_MIB_MAX }; diff --git a/net/mptcp/options.c b/net/mptcp/options.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -XXX,XX +XXX,XX @@ static bool mptcp_over_limit(struct sock *sk, struct sock *ssk, if (likely(mem <= READ_ONCE(sk->sk_rcvbuf))) return false; - /* Avoid silently dropping pure acks, fin or zero win probes. */ + /* Avoid silently dropping pure acks, fin or already-acked segments. */ if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq || TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN || !after(TCP_SKB_CB(skb)->end_seq, tcp_sk(ssk)->rcv_nxt)) @@ -XXX,XX +XXX,XX @@ static bool mptcp_over_limit(struct sock *sk, struct sock *ssk, /* Dropped due to memory constraints, schedule an ack. */ inet_csk(ssk)->icsk_ack.pending |= ICSK_ACK_NOMEM | ICSK_ACK_NOW; inet_csk_schedule_ack(ssk); + + /* In fallback mode: skb is dropped before the TCP recv queue. */ + NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPRCVQDROP); + return true; } -- 2.53.0
Uniform the new counter with the other OFO ones. Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260724-net-next-mptcp-oooq-pruning-v1-0-5dd4dec63a54%40kernel.org?part=5 Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/mib.c | 2 +- net/mptcp/mib.h | 2 +- net/mptcp/protocol.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/mib.c +++ b/net/mptcp/mib.c @@ -XXX,XX +XXX,XX @@ static const struct snmp_mib mptcp_snmp_list[] = { SNMP_MIB_ITEM("WinProbe", MPTCP_MIB_WINPROBE), 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("OFOPruned", MPTCP_MIB_OFOPRUNED), }; /* mptcp_mib_alloc - allocate percpu mib counters diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/mib.h +++ b/net/mptcp/mib.h @@ -XXX,XX +XXX,XX @@ enum linux_mptcp_mib_field { MPTCP_MIB_WINPROBE, /* MPTCP-level zero window probe */ MPTCP_MIB_BACKLOGDROP, /* Backlog over memory limit */ MPTCP_MIB_RCVPRUNED, /* Dropped due to memory constraints */ - MPTCP_MIB_OFO_PRUNED, /* MPTCP-level OoO queue pruned */ + MPTCP_MIB_OFOPRUNED, /* MPTCP-level OoO queue pruned */ __MPTCP_MIB_MAX }; diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -XXX,XX +XXX,XX @@ static bool mptcp_prune_ofo_queue(struct sock *sk, u64 seq) } while (node); if (pruned) - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFO_PRUNED); + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOPRUNED); out: mem = (unsigned int)sk_rmem_alloc_get(sk); -- 2.53.0