mptcp.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH mptcp-next v2 0/9] The infinite mapping support
@ 2021-09-09 11:51 Geliang Tang
  2021-09-09 11:51 ` [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag Geliang Tang
                   ` (8 more replies)
  0 siblings, 9 replies; 24+ messages in thread
From: Geliang Tang @ 2021-09-09 11:51 UTC (permalink / raw)
  To: mptcp, geliangtang; +Cc: Geliang Tang

From: Geliang Tang <geliangtang@xiaomi.com>

v2:
 - add MPTCP_INFINITE_DONE flag
 - add MAPPING_INFINITE mapping status
 - add start_seq in the msk

v1:
 - add noncontiguous flag
 - add the mibs check
 - tag: export/20210904T080009

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/216

Geliang Tang (9):
  mptcp: add noncontiguous flag
  mptcp: add MPTCP_INFINITE_DONE flag
  mptcp: add MAPPING_INFINITE mapping status
  mptcp: add start_seq in the msk
  mptcp: infinite mapping sending
  mptcp: infinite mapping receiving
  mptcp: add a mib for the infinite mapping sending
  selftests: mptcp: add infinite map mibs check
  DO-NOT-MERGE: mptcp: mp_fail test

 net/mptcp/mib.c                               |  1 +
 net/mptcp/mib.h                               |  1 +
 net/mptcp/pm.c                                |  6 ++
 net/mptcp/protocol.c                          | 41 ++++++++++++++
 net/mptcp/protocol.h                          | 37 ++++++++++++
 net/mptcp/subflow.c                           | 24 +++++---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 56 +++++++++++++++++++
 7 files changed, 159 insertions(+), 7 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag
  2021-09-09 11:51 [PATCH mptcp-next v2 0/9] The infinite mapping support Geliang Tang
@ 2021-09-09 11:51 ` Geliang Tang
  2021-09-09 13:25   ` Paolo Abeni
  2021-09-09 11:51 ` [PATCH mptcp-next v2 2/9] mptcp: add MPTCP_INFINITE_DONE flag Geliang Tang
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Geliang Tang @ 2021-09-09 11:51 UTC (permalink / raw)
  To: mptcp, geliangtang; +Cc: Geliang Tang

From: Geliang Tang <geliangtang@xiaomi.com>

This patch added a "noncontiguous" flag in the msk to track whether the
data is contiguous on a subflow. When retransmission happens, we could
set this flag, and once all retransmissions are DATA_ACK'd that flag
could be cleared.

When a bad checksum is detected and a single contiguous subflow is in
use, don't send RST + MP_FAIL, send data_ack + MP_FAIL instead.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 net/mptcp/protocol.c |  7 +++++++
 net/mptcp/protocol.h |  2 ++
 net/mptcp/subflow.c  | 12 ++++++------
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index bb8a2a231479..81ea03b9fff6 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1095,6 +1095,9 @@ static void __mptcp_clean_una(struct sock *sk)
 
 		dfrag_uncharge(sk, delta);
 		cleaned = true;
+
+		if (dfrag->resend_count == 0)
+			WRITE_ONCE(msk->noncontiguous, false);
 	}
 
 	/* all retransmitted data acked, recovery completed */
@@ -1171,6 +1174,7 @@ mptcp_carve_data_frag(const struct mptcp_sock *msk, struct page_frag *pfrag,
 	dfrag->overhead = offset - orig_offset + sizeof(struct mptcp_data_frag);
 	dfrag->offset = offset + sizeof(struct mptcp_data_frag);
 	dfrag->already_sent = 0;
+	dfrag->resend_count = 0;
 	dfrag->page = pfrag->page;
 
 	return dfrag;
@@ -2454,6 +2458,8 @@ static void __mptcp_retrans(struct sock *sk)
 		dfrag->already_sent = max(dfrag->already_sent, info.sent);
 		tcp_push(ssk, 0, info.mss_now, tcp_sk(ssk)->nonagle,
 			 info.size_goal);
+		dfrag->resend_count++;
+		WRITE_ONCE(msk->noncontiguous, true);
 	}
 
 	release_sock(ssk);
@@ -2872,6 +2878,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
 	WRITE_ONCE(msk->fully_established, false);
 	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
 		WRITE_ONCE(msk->csum_enabled, true);
+	WRITE_ONCE(msk->noncontiguous, false);
 
 	msk->write_seq = subflow_req->idsn + 1;
 	msk->snd_nxt = msk->write_seq;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index d3e6fd1615f1..011f84ae1593 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -213,6 +213,7 @@ struct mptcp_data_frag {
 	u16 offset;
 	u16 overhead;
 	u16 already_sent;
+	u16 resend_count;
 	struct page *page;
 };
 
@@ -249,6 +250,7 @@ struct mptcp_sock {
 	bool		rcv_fastclose;
 	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
 	bool		csum_enabled;
+	bool		noncontiguous;
 	spinlock_t	join_list_lock;
 	struct work_struct work;
 	struct sk_buff  *ooo_last_skb;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 1de7ce883c37..951aafb6021e 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1166,15 +1166,15 @@ static bool subflow_check_data_avail(struct sock *ssk)
 fallback:
 	/* RFC 8684 section 3.7. */
 	if (subflow->send_mp_fail) {
-		if (mptcp_has_another_subflow(ssk)) {
+		if (mptcp_has_another_subflow(ssk) || READ_ONCE(msk->noncontiguous)) {
+			ssk->sk_err = EBADMSG;
+			tcp_set_state(ssk, TCP_CLOSE);
+			subflow->reset_transient = 0;
+			subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
+			tcp_send_active_reset(ssk, GFP_ATOMIC);
 			while ((skb = skb_peek(&ssk->sk_receive_queue)))
 				sk_eat_skb(ssk, skb);
 		}
-		ssk->sk_err = EBADMSG;
-		tcp_set_state(ssk, TCP_CLOSE);
-		subflow->reset_transient = 0;
-		subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
-		tcp_send_active_reset(ssk, GFP_ATOMIC);
 		WRITE_ONCE(subflow->data_avail, 0);
 		return true;
 	}
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH mptcp-next v2 2/9] mptcp: add MPTCP_INFINITE_DONE flag
  2021-09-09 11:51 [PATCH mptcp-next v2 0/9] The infinite mapping support Geliang Tang
  2021-09-09 11:51 ` [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag Geliang Tang
@ 2021-09-09 11:51 ` Geliang Tang
  2021-09-09 13:31   ` Paolo Abeni
  2021-09-09 11:51 ` [PATCH mptcp-next v2 3/9] mptcp: add MAPPING_INFINITE mapping status Geliang Tang
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Geliang Tang @ 2021-09-09 11:51 UTC (permalink / raw)
  To: mptcp, geliangtang; +Cc: Geliang Tang

From: Geliang Tang <geliangtang@xiaomi.com>

This patch added a new flag named MPTCP_INFINITE_DONE. Define
mptcp_do_infinite and __mptcp_do_infinite to set this flag, and
mptcp_check_infinite and __mptcp_check_infinite to check this flag.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 net/mptcp/protocol.h | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 011f84ae1593..5644a361b9c7 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -123,6 +123,7 @@
 #define MPTCP_RETRANSMIT	9
 #define MPTCP_WORK_SYNC_SETSOCKOPT 10
 #define MPTCP_CONNECTED		11
+#define MPTCP_INFINITE_DONE	12
 
 static inline bool before64(__u64 seq1, __u64 seq2)
 {
@@ -870,6 +871,38 @@ static inline void mptcp_do_fallback(struct sock *sk)
 
 #define pr_fallback(a) pr_debug("%s:fallback to TCP (msk=%p)", __func__, a)
 
+static inline bool __mptcp_check_infinite(const struct mptcp_sock *msk)
+{
+	return test_bit(MPTCP_INFINITE_DONE, &msk->flags);
+}
+
+static inline bool mptcp_check_infinite(const struct sock *sk)
+{
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
+	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+
+	return __mptcp_check_infinite(msk);
+}
+
+static inline void __mptcp_do_infinite(struct mptcp_sock *msk)
+{
+	if (test_bit(MPTCP_INFINITE_DONE, &msk->flags)) {
+		pr_debug("Infinite mapping already done (msk=%p)", msk);
+		return;
+	}
+	set_bit(MPTCP_INFINITE_DONE, &msk->flags);
+}
+
+static inline void mptcp_do_infinite(struct sock *sk)
+{
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
+	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+
+	__mptcp_do_infinite(msk);
+}
+
+#define pr_infinite(a) pr_debug("%s:infinite mapping (msk=%p)", __func__, a)
+
 static inline bool subflow_simultaneous_connect(struct sock *sk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH mptcp-next v2 3/9] mptcp: add MAPPING_INFINITE mapping status
  2021-09-09 11:51 [PATCH mptcp-next v2 0/9] The infinite mapping support Geliang Tang
  2021-09-09 11:51 ` [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag Geliang Tang
  2021-09-09 11:51 ` [PATCH mptcp-next v2 2/9] mptcp: add MPTCP_INFINITE_DONE flag Geliang Tang
@ 2021-09-09 11:51 ` Geliang Tang
  2021-09-09 13:39   ` Paolo Abeni
  2021-09-09 11:51 ` [PATCH mptcp-next v2 4/9] mptcp: add start_seq in the msk Geliang Tang
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Geliang Tang @ 2021-09-09 11:51 UTC (permalink / raw)
  To: mptcp, geliangtang; +Cc: Geliang Tang

From: Geliang Tang <geliangtang@xiaomi.com>

This patch added a new mapping status named MAPPING_INFINITE. If the
MPTCP_INFINITE_DONE flag is set in get_mapping_status, return this new
status. And in subflow_check_data_avail, if this status is set, goto the
'infinite' lable to fallback.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 net/mptcp/subflow.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 951aafb6021e..ad8efe56eab6 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -798,6 +798,7 @@ enum mapping_status {
 	MAPPING_INVALID,
 	MAPPING_EMPTY,
 	MAPPING_DATA_FIN,
+	MAPPING_INFINITE,
 	MAPPING_DUMMY
 };
 
@@ -938,6 +939,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
 	if (!skb)
 		return MAPPING_EMPTY;
 
+	if (mptcp_check_infinite(ssk))
+		return MAPPING_INFINITE;
+
 	if (mptcp_check_fallback(ssk))
 		return MAPPING_DUMMY;
 
@@ -1121,6 +1125,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
 
 		status = get_mapping_status(ssk, msk);
 		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
+		if (unlikely(status == MAPPING_INFINITE))
+			goto infinite;
+
 		if (unlikely(status == MAPPING_INVALID))
 			goto fallback;
 
@@ -1192,6 +1199,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 		return false;
 	}
 
+infinite:
 	__mptcp_do_fallback(msk);
 	skb = skb_peek(&ssk->sk_receive_queue);
 	subflow->map_valid = 1;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH mptcp-next v2 4/9] mptcp: add start_seq in the msk
  2021-09-09 11:51 [PATCH mptcp-next v2 0/9] The infinite mapping support Geliang Tang
                   ` (2 preceding siblings ...)
  2021-09-09 11:51 ` [PATCH mptcp-next v2 3/9] mptcp: add MAPPING_INFINITE mapping status Geliang Tang
@ 2021-09-09 11:51 ` Geliang Tang
  2021-09-10  0:39   ` Mat Martineau
  2021-09-09 11:51 ` [PATCH mptcp-next v2 5/9] mptcp: infinite mapping sending Geliang Tang
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Geliang Tang @ 2021-09-09 11:51 UTC (permalink / raw)
  To: mptcp, geliangtang; +Cc: Geliang Tang

From: Geliang Tang <geliangtang@xiaomi.com>

This patch added a new member named start_seq to the msk to keep track of
the beginning of the last fully-acked data segment. This would be updated
in __mptcp_move_skb and mptcp_pending_data_fin.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 net/mptcp/protocol.c | 4 ++++
 net/mptcp/protocol.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 81ea03b9fff6..c7ecd3e3b537 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -308,6 +308,7 @@ static bool __mptcp_move_skb(struct mptcp_sock *msk, struct sock *ssk,
 
 	if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
 		/* in sequence */
+		WRITE_ONCE(msk->start_seq, msk->ack_seq);
 		WRITE_ONCE(msk->ack_seq, msk->ack_seq + copy_len);
 		tail = skb_peek_tail(&sk->sk_receive_queue);
 		if (tail && mptcp_try_coalesce(sk, tail, skb))
@@ -523,6 +524,7 @@ static bool mptcp_check_data_fin(struct sock *sk)
 	 */
 
 	if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
+		WRITE_ONCE(msk->start_seq, msk->ack_seq);
 		WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
 		WRITE_ONCE(msk->rcv_data_fin, 0);
 
@@ -2894,6 +2896,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
 		WRITE_ONCE(msk->ack_seq, ack_seq);
 		WRITE_ONCE(msk->rcv_wnd_sent, ack_seq);
 	}
+	WRITE_ONCE(msk->start_seq, 0);
 
 #if !IS_ENABLED(CONFIG_KASAN)
 	sock_reset_flag(nsk, SOCK_RCU_FREE);
@@ -3141,6 +3144,7 @@ void mptcp_finish_connect(struct sock *ssk)
 	WRITE_ONCE(msk->rcv_wnd_sent, ack_seq);
 	WRITE_ONCE(msk->can_ack, 1);
 	WRITE_ONCE(msk->snd_una, msk->write_seq);
+	WRITE_ONCE(msk->start_seq, 0);
 
 	mptcp_pm_new_connection(msk, ssk, 0);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 5644a361b9c7..77af55171ded 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -226,6 +226,7 @@ struct mptcp_sock {
 	u64		remote_key;
 	u64		write_seq;
 	u64		snd_nxt;
+	u64		start_seq;
 	u64		ack_seq;
 	u64		rcv_wnd_sent;
 	u64		rcv_data_fin_seq;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH mptcp-next v2 5/9] mptcp: infinite mapping sending
  2021-09-09 11:51 [PATCH mptcp-next v2 0/9] The infinite mapping support Geliang Tang
                   ` (3 preceding siblings ...)
  2021-09-09 11:51 ` [PATCH mptcp-next v2 4/9] mptcp: add start_seq in the msk Geliang Tang
@ 2021-09-09 11:51 ` Geliang Tang
  2021-09-09 13:54   ` Paolo Abeni
  2021-09-09 11:51 ` [PATCH mptcp-next v2 6/9] mptcp: infinite mapping receiving Geliang Tang
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Geliang Tang @ 2021-09-09 11:51 UTC (permalink / raw)
  To: mptcp, geliangtang; +Cc: Geliang Tang

From: Geliang Tang <geliangtang@xiaomi.com>

This patch added the infinite mapping sending logic.

Added a new flag snd_infinite_mapping_enable in mptcp_sock. Set it true
when a single contiguous subflow is in use in mptcp_pm_mp_fail_received.
In mptcp_sendmsg_frag, if this flag is true, call the new function
mptcp_update_infinite_mapping to set the infinite mapping.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 net/mptcp/pm.c       |  6 ++++++
 net/mptcp/protocol.c | 18 ++++++++++++++++++
 net/mptcp/protocol.h |  1 +
 3 files changed, 25 insertions(+)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 6ab386ff3294..2830adf64f79 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -251,7 +251,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
 
 void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 {
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
+	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+
 	pr_debug("fail_seq=%llu", fail_seq);
+
+	if (!mptcp_has_another_subflow(sk) && !READ_ONCE(msk->noncontiguous))
+		WRITE_ONCE(msk->snd_infinite_mapping_enable, true);
 }
 
 /* path manager helpers */
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index c7ecd3e3b537..4ebbbc6f1d01 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1278,6 +1278,21 @@ static void mptcp_update_data_checksum(struct sk_buff *skb, int added)
 	mpext->csum = csum_fold(csum_block_add(csum, skb_checksum(skb, offset, added, 0), offset));
 }
 
+static void mptcp_update_infinite_mapping(struct mptcp_sock *msk, struct mptcp_ext *mpext)
+{
+	if (!mpext)
+		return;
+
+	mpext->data_seq = READ_ONCE(msk->start_seq);
+	mpext->subflow_seq = 0;
+	mpext->data_len = 0;
+	mpext->csum = 0;
+
+	WRITE_ONCE(msk->snd_infinite_mapping_enable, false);
+	pr_infinite(msk);
+	__mptcp_do_infinite(msk);
+}
+
 static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 			      struct mptcp_data_frag *dfrag,
 			      struct mptcp_sendmsg_info *info)
@@ -1410,6 +1425,8 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 out:
 	if (READ_ONCE(msk->csum_enabled))
 		mptcp_update_data_checksum(skb, copy);
+	if (READ_ONCE(msk->snd_infinite_mapping_enable))
+		mptcp_update_infinite_mapping(msk, mpext);
 	mptcp_subflow_ctx(ssk)->rel_write_seq += copy;
 	return copy;
 }
@@ -2881,6 +2898,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
 	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
 		WRITE_ONCE(msk->csum_enabled, true);
 	WRITE_ONCE(msk->noncontiguous, false);
+	WRITE_ONCE(msk->snd_infinite_mapping_enable, false);
 
 	msk->write_seq = subflow_req->idsn + 1;
 	msk->snd_nxt = msk->write_seq;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 77af55171ded..b4a7c54f0d78 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -253,6 +253,7 @@ struct mptcp_sock {
 	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
 	bool		csum_enabled;
 	bool		noncontiguous;
+	bool		snd_infinite_mapping_enable;
 	spinlock_t	join_list_lock;
 	struct work_struct work;
 	struct sk_buff  *ooo_last_skb;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH mptcp-next v2 6/9] mptcp: infinite mapping receiving
  2021-09-09 11:51 [PATCH mptcp-next v2 0/9] The infinite mapping support Geliang Tang
                   ` (4 preceding siblings ...)
  2021-09-09 11:51 ` [PATCH mptcp-next v2 5/9] mptcp: infinite mapping sending Geliang Tang
@ 2021-09-09 11:51 ` Geliang Tang
  2021-09-09 13:55   ` Paolo Abeni
  2021-09-09 11:51 ` [PATCH mptcp-next v2 7/9] mptcp: add a mib for the infinite mapping sending Geliang Tang
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Geliang Tang @ 2021-09-09 11:51 UTC (permalink / raw)
  To: mptcp, geliangtang; +Cc: Geliang Tang

From: Geliang Tang <geliangtang@xiaomi.com>

This patch added the infinite mapping receiving logic. When the infinite
mapping is received. invoke __mptcp_do_infinite to set the
MPTCP_INFINITE_DONE flag and return MAPPING_INFINITE.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 net/mptcp/subflow.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index ad8efe56eab6..bf535cc46c5c 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -972,7 +972,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
 	data_len = mpext->data_len;
 	if (data_len == 0) {
 		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX);
-		return MAPPING_INVALID;
+		pr_infinite(msk);
+		__mptcp_do_infinite(msk);
+		return MAPPING_INFINITE;
 	}
 
 	if (mpext->data_fin == 1) {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH mptcp-next v2 7/9] mptcp: add a mib for the infinite mapping sending
  2021-09-09 11:51 [PATCH mptcp-next v2 0/9] The infinite mapping support Geliang Tang
                   ` (5 preceding siblings ...)
  2021-09-09 11:51 ` [PATCH mptcp-next v2 6/9] mptcp: infinite mapping receiving Geliang Tang
@ 2021-09-09 11:51 ` Geliang Tang
  2021-09-09 11:51 ` [PATCH mptcp-next v2 8/9] selftests: mptcp: add infinite map mibs check Geliang Tang
  2021-09-09 11:51 ` [PATCH mptcp-next v2 9/9] DO-NOT-MERGE: mptcp: mp_fail test Geliang Tang
  8 siblings, 0 replies; 24+ messages in thread
From: Geliang Tang @ 2021-09-09 11:51 UTC (permalink / raw)
  To: mptcp, geliangtang; +Cc: Geliang Tang

From: Geliang Tang <geliangtang@xiaomi.com>

This patch added a new mib named MPTCP_MIB_INFINITEMAPTX, increase it
when a infinite mapping has been sent out.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 net/mptcp/mib.c      | 1 +
 net/mptcp/mib.h      | 1 +
 net/mptcp/protocol.c | 3 +++
 3 files changed, 5 insertions(+)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index b21ff9be04c6..ab55afdcae22 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -24,6 +24,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
 	SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
 	SNMP_MIB_ITEM("DSSNotMatching", MPTCP_MIB_DSSNOMATCH),
+	SNMP_MIB_ITEM("InfiniteMapTx", MPTCP_MIB_INFINITEMAPTX),
 	SNMP_MIB_ITEM("InfiniteMapRx", MPTCP_MIB_INFINITEMAPRX),
 	SNMP_MIB_ITEM("DSSNoMatchTCP", MPTCP_MIB_DSSTCPMISMATCH),
 	SNMP_MIB_ITEM("DataCsumErr", MPTCP_MIB_DATACSUMERR),
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index ecd3d8b117e0..7901f1338d15 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -17,6 +17,7 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_JOINACKRX,		/* Received an ACK + MP_JOIN */
 	MPTCP_MIB_JOINACKMAC,		/* HMAC was wrong on ACK + MP_JOIN */
 	MPTCP_MIB_DSSNOMATCH,		/* Received a new mapping that did not match the previous one */
+	MPTCP_MIB_INFINITEMAPTX,	/* Sent an infinite mapping */
 	MPTCP_MIB_INFINITEMAPRX,	/* Received an infinite mapping */
 	MPTCP_MIB_DSSTCPMISMATCH,	/* DSS-mapping did not map with TCP's sequence numbers */
 	MPTCP_MIB_DATACSUMERR,		/* The data checksum fail */
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 4ebbbc6f1d01..7036c78ccef2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1280,6 +1280,8 @@ static void mptcp_update_data_checksum(struct sk_buff *skb, int added)
 
 static void mptcp_update_infinite_mapping(struct mptcp_sock *msk, struct mptcp_ext *mpext)
 {
+	struct sock *sk = (struct sock *)msk;
+
 	if (!mpext)
 		return;
 
@@ -1288,6 +1290,7 @@ static void mptcp_update_infinite_mapping(struct mptcp_sock *msk, struct mptcp_e
 	mpext->data_len = 0;
 	mpext->csum = 0;
 
+	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_INFINITEMAPTX);
 	WRITE_ONCE(msk->snd_infinite_mapping_enable, false);
 	pr_infinite(msk);
 	__mptcp_do_infinite(msk);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH mptcp-next v2 8/9] selftests: mptcp: add infinite map mibs check
  2021-09-09 11:51 [PATCH mptcp-next v2 0/9] The infinite mapping support Geliang Tang
                   ` (6 preceding siblings ...)
  2021-09-09 11:51 ` [PATCH mptcp-next v2 7/9] mptcp: add a mib for the infinite mapping sending Geliang Tang
@ 2021-09-09 11:51 ` Geliang Tang
  2021-09-09 11:51 ` [PATCH mptcp-next v2 9/9] DO-NOT-MERGE: mptcp: mp_fail test Geliang Tang
  8 siblings, 0 replies; 24+ messages in thread
From: Geliang Tang @ 2021-09-09 11:51 UTC (permalink / raw)
  To: mptcp, geliangtang; +Cc: Geliang Tang

From: Geliang Tang <geliangtang@xiaomi.com>

This patch added a function chk_infi_nr to check the mibs for the
infinite mapping.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 255793c5ac4f..fe0c8f3164a7 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -615,6 +615,43 @@ chk_fail_nr()
 	fi
 }
 
+chk_infi_nr()
+{
+	local mp_infi_nr_tx=$1
+	local mp_infi_nr_rx=$2
+	local count
+	local dump_stats
+
+	printf "%-39s %s" " " "itx"
+	count=`ip netns exec $ns1 nstat -as | grep InfiniteMapTx | awk '{print $2}'`
+	[ -z "$count" ] && count=0
+	if [ "$count" != "$mp_infi_nr_tx" ]; then
+		echo "[fail] got $count infinite map[s] TX expected $mp_infi_nr_tx"
+		ret=1
+		dump_stats=1
+	else
+		echo -n "[ ok ]"
+	fi
+
+	echo -n " - irx   "
+	count=`ip netns exec $ns2 nstat -as | grep InfiniteMapRx | awk '{print $2}'`
+	[ -z "$count" ] && count=0
+	if [ "$count" != "$mp_infi_nr_rx" ]; then
+		echo "[fail] got $count infinite map[s] RX expected $mp_infi_nr_rx"
+		ret=1
+		dump_stats=1
+	else
+		echo "[ ok ]"
+	fi
+
+	if [ "${dump_stats}" = 1 ]; then
+		echo Server ns stats
+		ip netns exec $ns1 nstat -as | grep MPTcp
+		echo Client ns stats
+		ip netns exec $ns2 nstat -as | grep MPTcp
+	fi
+}
+
 chk_join_nr()
 {
 	local msg="$1"
@@ -665,6 +702,7 @@ chk_join_nr()
 	if [ $checksum -eq 1 ]; then
 		chk_csum_nr
 		chk_fail_nr 0 0
+		chk_infi_nr 0 0
 	fi
 }
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH mptcp-next v2 9/9] DO-NOT-MERGE: mptcp: mp_fail test
  2021-09-09 11:51 [PATCH mptcp-next v2 0/9] The infinite mapping support Geliang Tang
                   ` (7 preceding siblings ...)
  2021-09-09 11:51 ` [PATCH mptcp-next v2 8/9] selftests: mptcp: add infinite map mibs check Geliang Tang
@ 2021-09-09 11:51 ` Geliang Tang
  2021-09-09 14:23   ` Paolo Abeni
  8 siblings, 1 reply; 24+ messages in thread
From: Geliang Tang @ 2021-09-09 11:51 UTC (permalink / raw)
  To: mptcp, geliangtang; +Cc: Geliang Tang

From: Geliang Tang <geliangtang@xiaomi.com>

./mptcp_join.sh -Cf

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 net/mptcp/protocol.c                           |  9 +++++++++
 .../testing/selftests/net/mptcp/mptcp_join.sh  | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7036c78ccef2..10d15e93d70b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1296,6 +1296,8 @@ static void mptcp_update_infinite_mapping(struct mptcp_sock *msk, struct mptcp_e
 	__mptcp_do_infinite(msk);
 }
 
+static int j;
+
 static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 			      struct mptcp_data_frag *dfrag,
 			      struct mptcp_sendmsg_info *info)
@@ -1430,6 +1432,13 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 		mptcp_update_data_checksum(skb, copy);
 	if (READ_ONCE(msk->snd_infinite_mapping_enable))
 		mptcp_update_infinite_mapping(msk, mpext);
+
+	pr_debug("%s j=%d", __func__, j++);
+	if (j == 20)
+		skb->data_len = 1;
+	if (j > 40)
+		j = 0;
+
 	mptcp_subflow_ctx(ssk)->rel_write_seq += copy;
 	return copy;
 }
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fe0c8f3164a7..38663f6373b8 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -977,6 +977,24 @@ chk_link_usage()
 
 subflows_tests()
 {
+	# 1 subflow
+	reset
+	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
+	ip netns exec $ns2 ./pm_nl_ctl limits 0 2
+	run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow
+	chk_join_nr "1 subflow" 0 0 0
+
+	exit
+
+	# multiple subflows
+	reset
+	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
+	ip netns exec $ns2 ./pm_nl_ctl limits 0 2
+	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
+	ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 flags subflow
+	run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow
+	chk_join_nr "multiple subflows" 2 2 2
+
 	reset
 	run_tests $ns1 $ns2 10.0.1.1
 	chk_join_nr "no JOIN" "0" "0" "0"
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag
  2021-09-09 11:51 ` [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag Geliang Tang
@ 2021-09-09 13:25   ` Paolo Abeni
  2021-09-10  0:00     ` Mat Martineau
  0 siblings, 1 reply; 24+ messages in thread
From: Paolo Abeni @ 2021-09-09 13:25 UTC (permalink / raw)
  To: Geliang Tang, mptcp; +Cc: Geliang Tang

On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> This patch added a "noncontiguous" flag in the msk to track whether the
> data is contiguous on a subflow. When retransmission happens, we could
> set this flag, and once all retransmissions are DATA_ACK'd that flag
> could be cleared.
> 
> When a bad checksum is detected and a single contiguous subflow is in
> use, don't send RST + MP_FAIL, send data_ack + MP_FAIL instead.
> 
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  net/mptcp/protocol.c |  7 +++++++
>  net/mptcp/protocol.h |  2 ++
>  net/mptcp/subflow.c  | 12 ++++++------
>  3 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index bb8a2a231479..81ea03b9fff6 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1095,6 +1095,9 @@ static void __mptcp_clean_una(struct sock *sk)
>  
>  		dfrag_uncharge(sk, delta);
>  		cleaned = true;
> +
> +		if (dfrag->resend_count == 0)
> +			WRITE_ONCE(msk->noncontiguous, false);
>  	}
>  
>  	/* all retransmitted data acked, recovery completed */
> @@ -1171,6 +1174,7 @@ mptcp_carve_data_frag(const struct mptcp_sock *msk, struct page_frag *pfrag,
>  	dfrag->overhead = offset - orig_offset + sizeof(struct mptcp_data_frag);
>  	dfrag->offset = offset + sizeof(struct mptcp_data_frag);
>  	dfrag->already_sent = 0;
> +	dfrag->resend_count = 0;
>  	dfrag->page = pfrag->page;
>  
>  	return dfrag;
> @@ -2454,6 +2458,8 @@ static void __mptcp_retrans(struct sock *sk)
>  		dfrag->already_sent = max(dfrag->already_sent, info.sent);
>  		tcp_push(ssk, 0, info.mss_now, tcp_sk(ssk)->nonagle,
>  			 info.size_goal);
> +		dfrag->resend_count++;
> +		WRITE_ONCE(msk->noncontiguous, true);
>  	}
>  
>  	release_sock(ssk);
> @@ -2872,6 +2878,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
>  	WRITE_ONCE(msk->fully_established, false);
>  	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
>  		WRITE_ONCE(msk->csum_enabled, true);
> +	WRITE_ONCE(msk->noncontiguous, false);
>  
>  	msk->write_seq = subflow_req->idsn + 1;
>  	msk->snd_nxt = msk->write_seq;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index d3e6fd1615f1..011f84ae1593 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -213,6 +213,7 @@ struct mptcp_data_frag {
>  	u16 offset;
>  	u16 overhead;
>  	u16 already_sent;
> +	u16 resend_count;
>  	struct page *page;
>  };

Ouch, the above increases mptcp_data_frag size by 8 bytes, due to
holes.

Is this necessary? I think the packet scheduler never reinject with a
single subflow. It used to do that, but it should not do anymore.

Even if the scheduler re-inject with a single subflow, can we instead
keep track of the last retrans sequence number - in __mptcp_retrans().

msk stream is 'continuous' if and only if 'before64(last_retrnas_seq,
msk->snd_una)'

/P


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 2/9] mptcp: add MPTCP_INFINITE_DONE flag
  2021-09-09 11:51 ` [PATCH mptcp-next v2 2/9] mptcp: add MPTCP_INFINITE_DONE flag Geliang Tang
@ 2021-09-09 13:31   ` Paolo Abeni
  0 siblings, 0 replies; 24+ messages in thread
From: Paolo Abeni @ 2021-09-09 13:31 UTC (permalink / raw)
  To: Geliang Tang, mptcp; +Cc: Geliang Tang

On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> This patch added a new flag named MPTCP_INFINITE_DONE. Define
> mptcp_do_infinite and __mptcp_do_infinite to set this flag, and
> mptcp_check_infinite and __mptcp_check_infinite to check this flag.
> 
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  net/mptcp/protocol.h | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 011f84ae1593..5644a361b9c7 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -123,6 +123,7 @@
>  #define MPTCP_RETRANSMIT	9
>  #define MPTCP_WORK_SYNC_SETSOCKOPT 10
>  #define MPTCP_CONNECTED		11
> +#define MPTCP_INFINITE_DONE	12

I'm likely the last person that should raise this kind of topics, but
IMHO this flag name is a bit unclear. What aboyt 'INFINITE_MAP_SET'?

/P


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 3/9] mptcp: add MAPPING_INFINITE mapping status
  2021-09-09 11:51 ` [PATCH mptcp-next v2 3/9] mptcp: add MAPPING_INFINITE mapping status Geliang Tang
@ 2021-09-09 13:39   ` Paolo Abeni
  2021-09-10  0:21     ` Mat Martineau
  0 siblings, 1 reply; 24+ messages in thread
From: Paolo Abeni @ 2021-09-09 13:39 UTC (permalink / raw)
  To: Geliang Tang, mptcp; +Cc: Geliang Tang

On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> This patch added a new mapping status named MAPPING_INFINITE. If the
> MPTCP_INFINITE_DONE flag is set in get_mapping_status, return this new
> status. And in subflow_check_data_avail, if this status is set, goto the
> 'infinite' lable to fallback.
> 
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  net/mptcp/subflow.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 951aafb6021e..ad8efe56eab6 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -798,6 +798,7 @@ enum mapping_status {
>  	MAPPING_INVALID,
>  	MAPPING_EMPTY,
>  	MAPPING_DATA_FIN,
> +	MAPPING_INFINITE,
>  	MAPPING_DUMMY
>  };
>  
> @@ -938,6 +939,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
>  	if (!skb)
>  		return MAPPING_EMPTY;
>  
> +	if (mptcp_check_infinite(ssk))
> +		return MAPPING_INFINITE;
> +
>  	if (mptcp_check_fallback(ssk))
>  		return MAPPING_DUMMY;
>  
> @@ -1121,6 +1125,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  
>  		status = get_mapping_status(ssk, msk);
>  		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
> +		if (unlikely(status == MAPPING_INFINITE))
> +			goto infinite;
> +
>  		if (unlikely(status == MAPPING_INVALID))
>  			goto fallback;
>  
> @@ -1192,6 +1199,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  		return false;
>  	}
>  
> +infinite:
>  	__mptcp_do_fallback(msk);
>  	skb = skb_peek(&ssk->sk_receive_queue);
>  	subflow->map_valid = 1;


It looks like MAPPING_INFINITE has almost the same behavior
of MAPPING_DUMMY.

I think we can avoid the new conditional in get_mapping_status().
Eventually we could do all the error checking after the 'fallback:'
label only if the msk has not fallen back yet:

fallback:
	if (!__mptcp_check_fallback(msk)) {
		/* RFC 8684 section 3.7. */
	        if (subflow->send_mp_fail) {
			...
		if (subflow->mp_join || subflow->fully_established) {
			...
		__mptcp_do_fallback(msk);
	}

	skb = skb_peek(&ssk->sk_receive_queue);
	...
WDYT?



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 5/9] mptcp: infinite mapping sending
  2021-09-09 11:51 ` [PATCH mptcp-next v2 5/9] mptcp: infinite mapping sending Geliang Tang
@ 2021-09-09 13:54   ` Paolo Abeni
  0 siblings, 0 replies; 24+ messages in thread
From: Paolo Abeni @ 2021-09-09 13:54 UTC (permalink / raw)
  To: Geliang Tang, mptcp; +Cc: Geliang Tang

On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> This patch added the infinite mapping sending logic.
> 
> Added a new flag snd_infinite_mapping_enable in mptcp_sock. Set it true
> when a single contiguous subflow is in use in mptcp_pm_mp_fail_received.
> In mptcp_sendmsg_frag, if this flag is true, call the new function
> mptcp_update_infinite_mapping to set the infinite mapping.
> 
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  net/mptcp/pm.c       |  6 ++++++
>  net/mptcp/protocol.c | 18 ++++++++++++++++++
>  net/mptcp/protocol.h |  1 +
>  3 files changed, 25 insertions(+)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 6ab386ff3294..2830adf64f79 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -251,7 +251,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>  
>  void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
>  {
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> +	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> +
>  	pr_debug("fail_seq=%llu", fail_seq);
> +
> +	if (!mptcp_has_another_subflow(sk) && !READ_ONCE(msk->noncontiguous))
> +		WRITE_ONCE(msk->snd_infinite_mapping_enable, true);
>  }
>  
>  /* path manager helpers */
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index c7ecd3e3b537..4ebbbc6f1d01 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1278,6 +1278,21 @@ static void mptcp_update_data_checksum(struct sk_buff *skb, int added)
>  	mpext->csum = csum_fold(csum_block_add(csum, skb_checksum(skb, offset, added, 0), offset));
>  }
>  
> +static void mptcp_update_infinite_mapping(struct mptcp_sock *msk, struct mptcp_ext *mpext)
> +{
> +	if (!mpext)
> +		return;
> +
> +	mpext->data_seq = READ_ONCE(msk->start_seq);
> +	mpext->subflow_seq = 0;
> +	mpext->data_len = 0;
> +	mpext->csum = 0;
> +
> +	WRITE_ONCE(msk->snd_infinite_mapping_enable, false);
> +	pr_infinite(msk);
> +	__mptcp_do_infinite(msk);
> +}

If you move the 'snd_infinite_mapping_enable' flag at the subflow level
(inside struct mptcp_subflow_context), all the _ONCE annotation will
not be needed. And the flag will be already initializated to 0 at ctx
creation time.

Additionally, I'm unsure the MPTCP_INFINITE_DONE bit is required ?!?
can we simply rely on FALLBACK ???

/P


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 6/9] mptcp: infinite mapping receiving
  2021-09-09 11:51 ` [PATCH mptcp-next v2 6/9] mptcp: infinite mapping receiving Geliang Tang
@ 2021-09-09 13:55   ` Paolo Abeni
  0 siblings, 0 replies; 24+ messages in thread
From: Paolo Abeni @ 2021-09-09 13:55 UTC (permalink / raw)
  To: Geliang Tang, mptcp; +Cc: Geliang Tang

On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> This patch added the infinite mapping receiving logic. When the infinite
> mapping is received. invoke __mptcp_do_infinite to set the
> MPTCP_INFINITE_DONE flag and return MAPPING_INFINITE.
> 
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  net/mptcp/subflow.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index ad8efe56eab6..bf535cc46c5c 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -972,7 +972,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
>  	data_len = mpext->data_len;
>  	if (data_len == 0) {
>  		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX);
> -		return MAPPING_INVALID;
> +		pr_infinite(msk);
> +		__mptcp_do_infinite(msk);
> +		return MAPPING_INFINITE;
>  	}
>  
>  	if (mpext->data_fin == 1) {

As for patch 3/9, I think we could simply return MAPPING_DUMMY.

/P


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 9/9] DO-NOT-MERGE: mptcp: mp_fail test
  2021-09-09 11:51 ` [PATCH mptcp-next v2 9/9] DO-NOT-MERGE: mptcp: mp_fail test Geliang Tang
@ 2021-09-09 14:23   ` Paolo Abeni
  2021-09-22  3:50     ` Geliang Tang
  0 siblings, 1 reply; 24+ messages in thread
From: Paolo Abeni @ 2021-09-09 14:23 UTC (permalink / raw)
  To: Geliang Tang, mptcp; +Cc: Geliang Tang

On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> ./mptcp_join.sh -Cf
> 
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>

I'm wondering if we could use the u32 target to replace all the TCP
options with u32 if find a DSS (tcp header len == 38, IIRC) and the DSS
len is not zero (checking 2 bytes at a fixed offset).

That will corrupt che tcp csum, but IIRC the csum is not really checked
over veth with default nic config.

/P


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag
  2021-09-09 13:25   ` Paolo Abeni
@ 2021-09-10  0:00     ` Mat Martineau
  2021-09-10 15:08       ` Paolo Abeni
  0 siblings, 1 reply; 24+ messages in thread
From: Mat Martineau @ 2021-09-10  0:00 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: Geliang Tang, mptcp, Geliang Tang

On Thu, 9 Sep 2021, Paolo Abeni wrote:

> On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
>> From: Geliang Tang <geliangtang@xiaomi.com>
>>
>> This patch added a "noncontiguous" flag in the msk to track whether the
>> data is contiguous on a subflow. When retransmission happens, we could
>> set this flag, and once all retransmissions are DATA_ACK'd that flag
>> could be cleared.
>>
>> When a bad checksum is detected and a single contiguous subflow is in
>> use, don't send RST + MP_FAIL, send data_ack + MP_FAIL instead.
>>
>> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
>> ---
>>  net/mptcp/protocol.c |  7 +++++++
>>  net/mptcp/protocol.h |  2 ++
>>  net/mptcp/subflow.c  | 12 ++++++------
>>  3 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index bb8a2a231479..81ea03b9fff6 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -1095,6 +1095,9 @@ static void __mptcp_clean_una(struct sock *sk)
>>
>>  		dfrag_uncharge(sk, delta);
>>  		cleaned = true;
>> +
>> +		if (dfrag->resend_count == 0)
>> +			WRITE_ONCE(msk->noncontiguous, false);
>>  	}
>>
>>  	/* all retransmitted data acked, recovery completed */
>> @@ -1171,6 +1174,7 @@ mptcp_carve_data_frag(const struct mptcp_sock *msk, struct page_frag *pfrag,
>>  	dfrag->overhead = offset - orig_offset + sizeof(struct mptcp_data_frag);
>>  	dfrag->offset = offset + sizeof(struct mptcp_data_frag);
>>  	dfrag->already_sent = 0;
>> +	dfrag->resend_count = 0;
>>  	dfrag->page = pfrag->page;
>>
>>  	return dfrag;
>> @@ -2454,6 +2458,8 @@ static void __mptcp_retrans(struct sock *sk)
>>  		dfrag->already_sent = max(dfrag->already_sent, info.sent);
>>  		tcp_push(ssk, 0, info.mss_now, tcp_sk(ssk)->nonagle,
>>  			 info.size_goal);
>> +		dfrag->resend_count++;
>> +		WRITE_ONCE(msk->noncontiguous, true);
>>  	}
>>
>>  	release_sock(ssk);
>> @@ -2872,6 +2878,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
>>  	WRITE_ONCE(msk->fully_established, false);
>>  	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
>>  		WRITE_ONCE(msk->csum_enabled, true);
>> +	WRITE_ONCE(msk->noncontiguous, false);
>>
>>  	msk->write_seq = subflow_req->idsn + 1;
>>  	msk->snd_nxt = msk->write_seq;
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index d3e6fd1615f1..011f84ae1593 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -213,6 +213,7 @@ struct mptcp_data_frag {
>>  	u16 offset;
>>  	u16 overhead;
>>  	u16 already_sent;
>> +	u16 resend_count;
>>  	struct page *page;
>>  };
>
> Ouch, the above increases mptcp_data_frag size by 8 bytes, due to
> holes.
>

What about rearranging the struct to eliminate the holes? (Full 
disclosure: I asked Geliang to add this, but am open to other solutions)

I was also thinking it could be useful to have this information around for 
retransmission metrics, but that doesn't seem too important.

> Is this necessary? I think the packet scheduler never reinject with a
> single subflow. It used to do that, but it should not do anymore.
>

There may be a single subflow now, but multiple subflows (with 
unacked reinjections) a very short time ago.

> Even if the scheduler re-inject with a single subflow, can we instead
> keep track of the last retrans sequence number - in __mptcp_retrans().
>
> msk stream is 'continuous' if and only if 'before64(last_retrnas_seq,
> msk->snd_una)'
>

If last_retrans_seq is checked only when msk->noncontiguous is true, I 
think that works out. Otherwise the last_retrans_seq is stale/invalid if 
retransmission never happened, or any retransmissions have been fully 
acked.


--
Mat Martineau
Intel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 3/9] mptcp: add MAPPING_INFINITE mapping status
  2021-09-09 13:39   ` Paolo Abeni
@ 2021-09-10  0:21     ` Mat Martineau
  2021-09-10 15:23       ` Paolo Abeni
  0 siblings, 1 reply; 24+ messages in thread
From: Mat Martineau @ 2021-09-10  0:21 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: Geliang Tang, mptcp, Geliang Tang

On Thu, 9 Sep 2021, Paolo Abeni wrote:

> On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
>> From: Geliang Tang <geliangtang@xiaomi.com>
>>
>> This patch added a new mapping status named MAPPING_INFINITE. If the
>> MPTCP_INFINITE_DONE flag is set in get_mapping_status, return this new
>> status. And in subflow_check_data_avail, if this status is set, goto the
>> 'infinite' lable to fallback.
>>
>> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
>> ---
>>  net/mptcp/subflow.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 951aafb6021e..ad8efe56eab6 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -798,6 +798,7 @@ enum mapping_status {
>>  	MAPPING_INVALID,
>>  	MAPPING_EMPTY,
>>  	MAPPING_DATA_FIN,
>> +	MAPPING_INFINITE,
>>  	MAPPING_DUMMY
>>  };
>>
>> @@ -938,6 +939,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
>>  	if (!skb)
>>  		return MAPPING_EMPTY;
>>
>> +	if (mptcp_check_infinite(ssk))
>> +		return MAPPING_INFINITE;
>> +
>>  	if (mptcp_check_fallback(ssk))
>>  		return MAPPING_DUMMY;
>>
>> @@ -1121,6 +1125,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>
>>  		status = get_mapping_status(ssk, msk);
>>  		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
>> +		if (unlikely(status == MAPPING_INFINITE))
>> +			goto infinite;
>> +
>>  		if (unlikely(status == MAPPING_INVALID))
>>  			goto fallback;
>>
>> @@ -1192,6 +1199,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>  		return false;
>>  	}
>>
>> +infinite:
>>  	__mptcp_do_fallback(msk);
>>  	skb = skb_peek(&ssk->sk_receive_queue);
>>  	subflow->map_valid = 1;
>
>
> It looks like MAPPING_INFINITE has almost the same behavior
> of MAPPING_DUMMY.

This is something else I asked for in the v1 review, to avoid reusing 
MAPPING_INVALID in a confusing way :)

How about using a switch statement after get_mapping_status() instead of 4 
if's in a row?

>
> I think we can avoid the new conditional in get_mapping_status().
> Eventually we could do all the error checking after the 'fallback:'
> label only if the msk has not fallen back yet:
>
> fallback:
> 	if (!__mptcp_check_fallback(msk)) {
> 		/* RFC 8684 section 3.7. */
> 	        if (subflow->send_mp_fail) {
> 			...
> 		if (subflow->mp_join || subflow->fully_established) {

This condition also needs to check for the infinite mapping case, which is 
why it seemed useful to have a separate MAPPING_ enum. Fallback is being 
triggered here in response to the infinite mapping, so the subflow should 
not be forced to close.

> 			...
> 		__mptcp_do_fallback(msk);
> 	}
>
> 	skb = skb_peek(&ssk->sk_receive_queue);
> 	...
> WDYT?



--
Mat Martineau
Intel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 4/9] mptcp: add start_seq in the msk
  2021-09-09 11:51 ` [PATCH mptcp-next v2 4/9] mptcp: add start_seq in the msk Geliang Tang
@ 2021-09-10  0:39   ` Mat Martineau
  0 siblings, 0 replies; 24+ messages in thread
From: Mat Martineau @ 2021-09-10  0:39 UTC (permalink / raw)
  To: Geliang Tang; +Cc: mptcp, Geliang Tang

On Thu, 9 Sep 2021, Geliang Tang wrote:

> From: Geliang Tang <geliangtang@xiaomi.com>
>
> This patch added a new member named start_seq to the msk to keep track of
> the beginning of the last fully-acked data segment. This would be updated
> in __mptcp_move_skb and mptcp_pending_data_fin.
>
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
> net/mptcp/protocol.c | 4 ++++
> net/mptcp/protocol.h | 1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 81ea03b9fff6..c7ecd3e3b537 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -308,6 +308,7 @@ static bool __mptcp_move_skb(struct mptcp_sock *msk, struct sock *ssk,
>
> 	if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
> 		/* in sequence */
> +		WRITE_ONCE(msk->start_seq, msk->ack_seq);

This is tracking the previous value of ack_seq, which is not always 
aligned with the beginning of a DSS segment. It's also recording a 
sequence number for data received by this peer... but I think the RFC is 
asking for the inifinite mapping sequence number to be for data *sent* by 
this peer and acked by the other side.

This peer has already sent acknowledgements for the data received, so 
there's no need to send information (old rx sequence numbers) that the 
other side already has.

- Mat


> 		WRITE_ONCE(msk->ack_seq, msk->ack_seq + copy_len);
> 		tail = skb_peek_tail(&sk->sk_receive_queue);
> 		if (tail && mptcp_try_coalesce(sk, tail, skb))
> @@ -523,6 +524,7 @@ static bool mptcp_check_data_fin(struct sock *sk)
> 	 */
>
> 	if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
> +		WRITE_ONCE(msk->start_seq, msk->ack_seq);
> 		WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
> 		WRITE_ONCE(msk->rcv_data_fin, 0);
>
> @@ -2894,6 +2896,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
> 		WRITE_ONCE(msk->ack_seq, ack_seq);
> 		WRITE_ONCE(msk->rcv_wnd_sent, ack_seq);
> 	}
> +	WRITE_ONCE(msk->start_seq, 0);
>
> #if !IS_ENABLED(CONFIG_KASAN)
> 	sock_reset_flag(nsk, SOCK_RCU_FREE);
> @@ -3141,6 +3144,7 @@ void mptcp_finish_connect(struct sock *ssk)
> 	WRITE_ONCE(msk->rcv_wnd_sent, ack_seq);
> 	WRITE_ONCE(msk->can_ack, 1);
> 	WRITE_ONCE(msk->snd_una, msk->write_seq);
> +	WRITE_ONCE(msk->start_seq, 0);
>
> 	mptcp_pm_new_connection(msk, ssk, 0);
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 5644a361b9c7..77af55171ded 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -226,6 +226,7 @@ struct mptcp_sock {
> 	u64		remote_key;
> 	u64		write_seq;
> 	u64		snd_nxt;
> +	u64		start_seq;
> 	u64		ack_seq;
> 	u64		rcv_wnd_sent;
> 	u64		rcv_data_fin_seq;
> -- 
> 2.31.1
>
>
>

--
Mat Martineau
Intel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag
  2021-09-10  0:00     ` Mat Martineau
@ 2021-09-10 15:08       ` Paolo Abeni
  2021-09-10 16:49         ` Mat Martineau
  0 siblings, 1 reply; 24+ messages in thread
From: Paolo Abeni @ 2021-09-10 15:08 UTC (permalink / raw)
  To: Mat Martineau; +Cc: Geliang Tang, mptcp, Geliang Tang

On Thu, 2021-09-09 at 17:00 -0700, Mat Martineau wrote:
> > On Thu, 9 Sep 2021, Paolo Abeni wrote:
> > msk stream is 'continuous' if and only if 'before64(last_retrnas_seq,
> > msk->snd_una)'
> > 
> 
> If last_retrans_seq is checked only when msk->noncontiguous is true, I 
> think that works out. Otherwise the last_retrans_seq is stale/invalid if 
> retransmission never happened, or any retransmissions have been fully 
> acked.

I think can we just init 'last_retrans_seq' to intial_seq - 1 at msk
creation time and we could always use the above check:

	if (before64(last_retrnas_seq, msk->snd_una))

WDYT?

Cheers,

Paolo



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 3/9] mptcp: add MAPPING_INFINITE mapping status
  2021-09-10  0:21     ` Mat Martineau
@ 2021-09-10 15:23       ` Paolo Abeni
  2021-09-10 17:22         ` Mat Martineau
  0 siblings, 1 reply; 24+ messages in thread
From: Paolo Abeni @ 2021-09-10 15:23 UTC (permalink / raw)
  To: Mat Martineau; +Cc: Geliang Tang, mptcp, Geliang Tang

On Thu, 2021-09-09 at 17:21 -0700, Mat Martineau wrote:
> On Thu, 9 Sep 2021, Paolo Abeni wrote:
> 
> > On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> > > From: Geliang Tang <geliangtang@xiaomi.com>
> > > 
> > > This patch added a new mapping status named MAPPING_INFINITE. If the
> > > MPTCP_INFINITE_DONE flag is set in get_mapping_status, return this new
> > > status. And in subflow_check_data_avail, if this status is set, goto the
> > > 'infinite' lable to fallback.
> > > 
> > > Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> > > ---
> > >  net/mptcp/subflow.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > > index 951aafb6021e..ad8efe56eab6 100644
> > > --- a/net/mptcp/subflow.c
> > > +++ b/net/mptcp/subflow.c
> > > @@ -798,6 +798,7 @@ enum mapping_status {
> > >  	MAPPING_INVALID,
> > >  	MAPPING_EMPTY,
> > >  	MAPPING_DATA_FIN,
> > > +	MAPPING_INFINITE,
> > >  	MAPPING_DUMMY
> > >  };
> > > 
> > > @@ -938,6 +939,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
> > >  	if (!skb)
> > >  		return MAPPING_EMPTY;
> > > 
> > > +	if (mptcp_check_infinite(ssk))
> > > +		return MAPPING_INFINITE;
> > > +
> > >  	if (mptcp_check_fallback(ssk))
> > >  		return MAPPING_DUMMY;
> > > 
> > > @@ -1121,6 +1125,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
> > > 
> > >  		status = get_mapping_status(ssk, msk);
> > >  		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
> > > +		if (unlikely(status == MAPPING_INFINITE))
> > > +			goto infinite;
> > > +
> > >  		if (unlikely(status == MAPPING_INVALID))
> > >  			goto fallback;
> > > 
> > > @@ -1192,6 +1199,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
> > >  		return false;
> > >  	}
> > > 
> > > +infinite:
> > >  	__mptcp_do_fallback(msk);
> > >  	skb = skb_peek(&ssk->sk_receive_queue);
> > >  	subflow->map_valid = 1;
> > 
> > It looks like MAPPING_INFINITE has almost the same behavior
> > of MAPPING_DUMMY.
> 
> This is something else I asked for in the v1 review, to avoid reusing 
> MAPPING_INVALID in a confusing way :)

I read that ;) I thought 'MAPPING_DUMMY' would be less confusing.

> How about using a switch statement after get_mapping_status() instead of 4 
> if's in a row?

Will be mostly the same, from generate code perspective, I think.

What if we rename MAPPING_DUMMY into MAPPING_INFINITE, and we use only
a single value? MAPPING_DUMMY is currently used after a fallback to
implement the sort of infinite mapping we use there.

> > I think we can avoid the new conditional in get_mapping_status().
> > Eventually we could do all the error checking after the 'fallback:'
> > label only if the msk has not fallen back yet:
> > 
> > fallback:
> > 	if (!__mptcp_check_fallback(msk)) {
> > 		/* RFC 8684 section 3.7. */
> > 	        if (subflow->send_mp_fail) {
> > 			...
> > 		if (subflow->mp_join || subflow->fully_established) {
> 
> This condition also needs to check for the infinite mapping case, which is 
> why it seemed useful to have a separate MAPPING_ enum. Fallback is being 
> triggered here in response to the infinite mapping, so the subflow should 
> not be forced to close.

My point is all about avoiding additional conditionals in the 'fast-
path' / default receive path.

I think we still could do that, and being able to discriminate here
between infinite mapping received or not:

- get_mapping_status() returns the same value in the dummy and infinite
mapping case.
- in the infinite mapping case, get_mapping_status() additionally sets
  subflow->map_data_len to 0, 
- in the above code - under 'if (!__mptcp_check_fallback(msk)) {' -
 subflow->map_data_len == 0 implies infinite mapping received, 

WDYT?

Thanks!

Paolo



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag
  2021-09-10 15:08       ` Paolo Abeni
@ 2021-09-10 16:49         ` Mat Martineau
  0 siblings, 0 replies; 24+ messages in thread
From: Mat Martineau @ 2021-09-10 16:49 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: Geliang Tang, mptcp, Geliang Tang

On Fri, 10 Sep 2021, Paolo Abeni wrote:

> On Thu, 2021-09-09 at 17:00 -0700, Mat Martineau wrote:
>>> On Thu, 9 Sep 2021, Paolo Abeni wrote:
>>> msk stream is 'continuous' if and only if 'before64(last_retrnas_seq,
>>> msk->snd_una)'
>>>
>>
>> If last_retrans_seq is checked only when msk->noncontiguous is true, I
>> think that works out. Otherwise the last_retrans_seq is stale/invalid if
>> retransmission never happened, or any retransmissions have been fully
>> acked.
>
> I think can we just init 'last_retrans_seq' to intial_seq - 1 at msk
> creation time and we could always use the above check:
>
> 	if (before64(last_retrnas_seq, msk->snd_una))
>
> WDYT?
>

Ah, ok. Initialize it like that, and then keep moving last_retrans_seq 
ahead when updating snd_una and the MPTCP stream *is* contiguous, so we 
don't run in to problems when snd_una wraps around relative to 
last_retrans_seq.

And be sure to set last_retrans_seq to the sequence number for the end of 
the retransmitted data when entering recovery as well as in 
__mptcp_retrans().

--
Mat Martineau
Intel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 3/9] mptcp: add MAPPING_INFINITE mapping status
  2021-09-10 15:23       ` Paolo Abeni
@ 2021-09-10 17:22         ` Mat Martineau
  0 siblings, 0 replies; 24+ messages in thread
From: Mat Martineau @ 2021-09-10 17:22 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: Geliang Tang, mptcp, Geliang Tang

On Fri, 10 Sep 2021, Paolo Abeni wrote:

> On Thu, 2021-09-09 at 17:21 -0700, Mat Martineau wrote:
>> On Thu, 9 Sep 2021, Paolo Abeni wrote:
>>
>>> On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
>>>> From: Geliang Tang <geliangtang@xiaomi.com>
>>>>
>>>> This patch added a new mapping status named MAPPING_INFINITE. If the
>>>> MPTCP_INFINITE_DONE flag is set in get_mapping_status, return this new
>>>> status. And in subflow_check_data_avail, if this status is set, goto the
>>>> 'infinite' lable to fallback.
>>>>
>>>> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
>>>> ---
>>>>  net/mptcp/subflow.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>>>> index 951aafb6021e..ad8efe56eab6 100644
>>>> --- a/net/mptcp/subflow.c
>>>> +++ b/net/mptcp/subflow.c
>>>> @@ -798,6 +798,7 @@ enum mapping_status {
>>>>  	MAPPING_INVALID,
>>>>  	MAPPING_EMPTY,
>>>>  	MAPPING_DATA_FIN,
>>>> +	MAPPING_INFINITE,
>>>>  	MAPPING_DUMMY
>>>>  };
>>>>
>>>> @@ -938,6 +939,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
>>>>  	if (!skb)
>>>>  		return MAPPING_EMPTY;
>>>>
>>>> +	if (mptcp_check_infinite(ssk))
>>>> +		return MAPPING_INFINITE;
>>>> +
>>>>  	if (mptcp_check_fallback(ssk))
>>>>  		return MAPPING_DUMMY;
>>>>
>>>> @@ -1121,6 +1125,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>>>
>>>>  		status = get_mapping_status(ssk, msk);
>>>>  		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
>>>> +		if (unlikely(status == MAPPING_INFINITE))
>>>> +			goto infinite;
>>>> +
>>>>  		if (unlikely(status == MAPPING_INVALID))
>>>>  			goto fallback;
>>>>
>>>> @@ -1192,6 +1199,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>>>  		return false;
>>>>  	}
>>>>
>>>> +infinite:
>>>>  	__mptcp_do_fallback(msk);
>>>>  	skb = skb_peek(&ssk->sk_receive_queue);
>>>>  	subflow->map_valid = 1;
>>>
>>> It looks like MAPPING_INFINITE has almost the same behavior
>>> of MAPPING_DUMMY.
>>
>> This is something else I asked for in the v1 review, to avoid reusing
>> MAPPING_INVALID in a confusing way :)
>
> I read that ;) I thought 'MAPPING_DUMMY' would be less confusing.
>
>> How about using a switch statement after get_mapping_status() instead of 4
>> if's in a row?
>
> Will be mostly the same, from generate code perspective, I think.
>
> What if we rename MAPPING_DUMMY into MAPPING_INFINITE, and we use only
> a single value? MAPPING_DUMMY is currently used after a fallback to
> implement the sort of infinite mapping we use there.
>

That's fine.

>>> I think we can avoid the new conditional in get_mapping_status().
>>> Eventually we could do all the error checking after the 'fallback:'
>>> label only if the msk has not fallen back yet:
>>>
>>> fallback:
>>> 	if (!__mptcp_check_fallback(msk)) {
>>> 		/* RFC 8684 section 3.7. */
>>> 	        if (subflow->send_mp_fail) {
>>> 			...
>>> 		if (subflow->mp_join || subflow->fully_established) {
>>
>> This condition also needs to check for the infinite mapping case, which is
>> why it seemed useful to have a separate MAPPING_ enum. Fallback is being
>> triggered here in response to the infinite mapping, so the subflow should
>> not be forced to close.
>
> My point is all about avoiding additional conditionals in the 'fast-
> path' / default receive path.
>
> I think we still could do that, and being able to discriminate here
> between infinite mapping received or not:
>
> - get_mapping_status() returns the same value in the dummy and infinite
> mapping case.
> - in the infinite mapping case, get_mapping_status() additionally sets
>  subflow->map_data_len to 0,

  - and in other cases, set it to nonzero.

> - in the above code - under 'if (!__mptcp_check_fallback(msk)) {' -
> subflow->map_data_len == 0 implies infinite mapping received,
>
> WDYT?

I think it works. I'm slightly uneasy with overloading 
subflow->map_data_len from a code readability and maintenance perspective, 
but if the optimization pays off it's manageable with some good comments.

--
Mat Martineau
Intel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH mptcp-next v2 9/9] DO-NOT-MERGE: mptcp: mp_fail test
  2021-09-09 14:23   ` Paolo Abeni
@ 2021-09-22  3:50     ` Geliang Tang
  0 siblings, 0 replies; 24+ messages in thread
From: Geliang Tang @ 2021-09-22  3:50 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: MPTCP Upstream, Geliang Tang

Hi Paolo,

Paolo Abeni <pabeni@redhat.com> 于2021年9月9日周四 下午10:23写道:
>
> On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> > From: Geliang Tang <geliangtang@xiaomi.com>
> >
> > ./mptcp_join.sh -Cf
> >
> > Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
>
> I'm wondering if we could use the u32 target to replace all the TCP
> options with u32 if find a DSS (tcp header len == 38, IIRC) and the DSS
> len is not zero (checking 2 bytes at a fixed offset).

Please give me more help about how to use the u32 target to replace the TCP
options.

Thanks,
-Geliang

>
> That will corrupt che tcp csum, but IIRC the csum is not really checked
> over veth with default nic config.
>
> /P
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2021-09-22  3:50 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-09 11:51 [PATCH mptcp-next v2 0/9] The infinite mapping support Geliang Tang
2021-09-09 11:51 ` [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag Geliang Tang
2021-09-09 13:25   ` Paolo Abeni
2021-09-10  0:00     ` Mat Martineau
2021-09-10 15:08       ` Paolo Abeni
2021-09-10 16:49         ` Mat Martineau
2021-09-09 11:51 ` [PATCH mptcp-next v2 2/9] mptcp: add MPTCP_INFINITE_DONE flag Geliang Tang
2021-09-09 13:31   ` Paolo Abeni
2021-09-09 11:51 ` [PATCH mptcp-next v2 3/9] mptcp: add MAPPING_INFINITE mapping status Geliang Tang
2021-09-09 13:39   ` Paolo Abeni
2021-09-10  0:21     ` Mat Martineau
2021-09-10 15:23       ` Paolo Abeni
2021-09-10 17:22         ` Mat Martineau
2021-09-09 11:51 ` [PATCH mptcp-next v2 4/9] mptcp: add start_seq in the msk Geliang Tang
2021-09-10  0:39   ` Mat Martineau
2021-09-09 11:51 ` [PATCH mptcp-next v2 5/9] mptcp: infinite mapping sending Geliang Tang
2021-09-09 13:54   ` Paolo Abeni
2021-09-09 11:51 ` [PATCH mptcp-next v2 6/9] mptcp: infinite mapping receiving Geliang Tang
2021-09-09 13:55   ` Paolo Abeni
2021-09-09 11:51 ` [PATCH mptcp-next v2 7/9] mptcp: add a mib for the infinite mapping sending Geliang Tang
2021-09-09 11:51 ` [PATCH mptcp-next v2 8/9] selftests: mptcp: add infinite map mibs check Geliang Tang
2021-09-09 11:51 ` [PATCH mptcp-next v2 9/9] DO-NOT-MERGE: mptcp: mp_fail test Geliang Tang
2021-09-09 14:23   ` Paolo Abeni
2021-09-22  3:50     ` Geliang Tang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).