[PATCH mptcp-next v2 0/6] mptcp: add some more diag info

[PATCH mptcp-next v2 0/6] mptcp: add some more diag info

[Paolo Abeni]

This is a follow up to the topic discussed in recent pubblic mtg.

Introduces unique id for accurate subflow stats tracking and
aggregate mptcp counters, plus some minimal self-tests.

The tests themself do not take in account support for running on
older kernel.

This is on top of "mptcp: a bunch of data race fixes".

There should be non trivial conflicts with:

"mptcp: use get_retrans wrapper".

v1 -> v2:
 - introduce MPTCP_FULL_INFO instead of overloading a tcp_info field
 - add related self-tests
 - fix a couple of subflow_id initialization bugs

Paolo Abeni (6):
  mptcp: add subflow unique id
  mptcp: introduce MPTCP_FULL_INFO getsockopt
  mptcp: move snd_una update earlier for fallback socket.
  mptcp: track some aggregate data counters.
  selftests: mptcp: explicitly tests aggregate counters
  selftests: mptcp: add MPTCP_FULL_INFO testcase

 include/uapi/linux/mptcp.h                    |  17 +++
 net/mptcp/options.c                           |  14 ++-
 net/mptcp/protocol.c                          |  22 ++--
 net/mptcp/protocol.h                          |   9 +-
 net/mptcp/sockopt.c                           |  97 ++++++++++++++++-
 net/mptcp/subflow.c                           |   2 +
 .../selftests/net/mptcp/mptcp_sockopt.c       | 103 +++++++++++++++++-
 7 files changed, 248 insertions(+), 16 deletions(-)

-- 
2.40.1

[PATCH mptcp-next v2 1/6] mptcp: add subflow unique id

[Paolo Abeni]

The user-space need to preperly account the data received/sent by
individual subflows. When additional subflows are created and/or
closed during the MPTCP socket lifetime, the information currently
exposed via MPTCP_TCPINFO are not enough: subflows are identifed only
by the sequential position inside the info dumps, and that will change
with the above mentioned events.

To solve the above problem, this patch introduces a new subflow identifier
that is unique inside the given mptcp socket scope.

The initial subflow get the id 1 and the other subflows get incremental
values at join time.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
 - properly set subflow_id for the first passive subflow and active subflows, too
 - drop the tcpi_fackets overload
---
 net/mptcp/protocol.c | 4 ++++
 net/mptcp/protocol.h | 5 ++++-
 net/mptcp/subflow.c  | 2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 28da6a9fe8fd..b2f0408a5d7c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -96,6 +96,7 @@ static int __mptcp_socket_create(struct mptcp_sock *msk)
 	list_add(&subflow->node, &msk->conn_list);
 	sock_hold(ssock->sk);
 	subflow->request_mptcp = 1;
+	subflow->subflow_id = msk->subflow_id++;
 
 	/* This is the first subflow, always with id 0 */
 	subflow->local_id_valid = 1;
@@ -845,6 +846,7 @@ static bool __mptcp_finish_join(struct mptcp_sock *msk, struct sock *ssk)
 	if (sk->sk_socket && !ssk->sk_socket)
 		mptcp_sock_graft(ssk, sk->sk_socket);
 
+	mptcp_subflow_ctx(ssk)->subflow_id = msk->subflow_id++;
 	mptcp_sockopt_sync_locked(msk, ssk);
 	mptcp_subflow_joined(msk, ssk);
 	return true;
@@ -2775,6 +2777,7 @@ static int __mptcp_init_sock(struct sock *sk)
 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
 	WRITE_ONCE(msk->allow_infinite_fallback, true);
 	msk->recovery = false;
+	msk->subflow_id = 1;
 
 	mptcp_pm_data_init(msk);
 
@@ -3203,6 +3206,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
 	msk->snd_nxt = msk->write_seq;
 	msk->snd_una = msk->write_seq;
 	msk->wnd_end = msk->snd_nxt + req->rsk_rcv_wnd;
+	msk->subflow_id++;
 	msk->setsockopt_seq = mptcp_sk(sk)->setsockopt_seq;
 	mptcp_init_sched(msk, mptcp_sk(sk)->sched);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index de94c01746dc..f9180ecce5e4 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -319,7 +319,8 @@ struct mptcp_sock {
 		u64	rtt_us; /* last maximum rtt of subflows */
 	} rcvq_space;
 
-	u32 setsockopt_seq;
+	u32		subflow_id;
+	u32		setsockopt_seq;
 	char		ca_name[TCP_CA_NAME_MAX];
 	struct mptcp_sock	*dl_next;
 };
@@ -501,6 +502,8 @@ struct mptcp_subflow_context {
 	u8	reset_reason:4;
 	u8	stale_count;
 
+	u32	subflow_id;
+
 	long	delegated_status;
 	unsigned long	fail_tout;
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 63ac4dc621d4..c7001a23550a 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -819,6 +819,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			if (!ctx->conn)
 				goto fallback;
 
+			ctx->subflow_id = 1;
 			owner = mptcp_sk(ctx->conn);
 			mptcp_pm_new_connection(owner, child, 1);
 
@@ -1574,6 +1575,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	subflow->remote_id = remote_id;
 	subflow->request_join = 1;
 	subflow->request_bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
+	subflow->subflow_id = msk->subflow_id++;
 	mptcp_info2sockaddr(remote, &addr, ssk->sk_family);
 
 	sock_hold(ssk);
-- 
2.40.1

[PATCH mptcp-next v2 2/6] mptcp: introduce MPTCP_FULL_INFO getsockopt

[Paolo Abeni]

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/uapi/linux/mptcp.h | 12 ++++++
 net/mptcp/sockopt.c        | 75 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index 32af2d278cb4..483d4d2a3646 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -12,6 +12,7 @@
 #include <linux/in.h>		/* for sockaddr_in			*/
 #include <linux/in6.h>		/* for sockaddr_in6			*/
 #include <linux/socket.h>	/* for sockaddr_storage and sa_family	*/
+#include <linux/tcp.h>		/* for tcp_info				*/
 
 #define MPTCP_SUBFLOW_FLAG_MCAP_REM		_BITUL(0)
 #define MPTCP_SUBFLOW_FLAG_MCAP_LOC		_BITUL(1)
@@ -244,9 +245,20 @@ struct mptcp_subflow_addrs {
 	};
 };
 
+struct mptcp_subflow_info {
+	__u32				id;
+	struct mptcp_subflow_addrs	addrs;
+};
+
+struct mptcp_subflow_full_info {
+	struct mptcp_subflow_info	subflow_info;
+	struct tcp_info			tcp_info;
+};
+
 /* MPTCP socket options */
 #define MPTCP_INFO		1
 #define MPTCP_TCPINFO		2
 #define MPTCP_SUBFLOW_ADDRS	3
+#define MPTCP_FULL_INFO		4
 
 #endif /* _UAPI_MPTCP_H */
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index d4258869ac48..b4d04d5dc1f7 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -1146,6 +1146,79 @@ static int mptcp_getsockopt_subflow_addrs(struct mptcp_sock *msk, char __user *o
 	return 0;
 }
 
+static int mptcp_getsockopt_full_info(struct mptcp_sock *msk, char __user *optval,
+				      int __user *optlen)
+{
+	struct mptcp_subflow_context *subflow;
+	struct sock *sk = (struct sock *)msk;
+	unsigned int sfcount = 0, copied = 0;
+	int mptcp_info_len, len, size_user;
+	struct mptcp_subflow_data sfd;
+	char __user *infoptr;
+
+	len = mptcp_get_subflow_data(&sfd, optval, optlen);
+	if (len < 0)
+		return len;
+
+	/* don't bother filling the mptcp info if there is not enough
+	 * user-space-provided storage
+	 */
+	infoptr = optval + sfd.size_subflow_data;
+	if (len > sizeof(struct mptcp_subflow_data)) {
+		struct mptcp_info mptcp_info;
+
+		memset(&mptcp_info, 0, sizeof(mptcp_info));
+		mptcp_info_len = min_t(unsigned int, len, sizeof(struct mptcp_info));
+
+		mptcp_diag_fill_info(msk, &mptcp_info);
+
+		if (copy_to_user(infoptr, &mptcp_info, mptcp_info_len))
+			return -EFAULT;
+
+		len -= mptcp_info_len;
+		copied += mptcp_info_len;
+		infoptr += mptcp_info_len;
+	}
+
+	sfd.size_kernel = sizeof(struct mptcp_subflow_full_info);
+	sfd.size_user = min_t(unsigned int, sfd.size_user,
+			      sizeof(struct mptcp_subflow_full_info));
+	lock_sock(sk);
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+		++sfcount;
+		if (len >= size_user) {
+			struct mptcp_subflow_full_info sinfo;
+
+			memset(&sinfo, 0, sizeof(sinfo));
+			sinfo.subflow_info.id = subflow->subflow_id;
+			mptcp_get_sub_addrs(ssk, &sinfo.subflow_info.addrs);
+
+			/* don't bother filling the tcp info if the storage is too small */
+			if (sfd.size_user > sizeof(struct mptcp_subflow_info))
+				tcp_get_info(ssk, &sinfo.tcp_info);
+
+			if (copy_to_user(infoptr, &sinfo, sfd.size_user)) {
+				release_sock(sk);
+				return -EFAULT;
+			}
+
+			infoptr += sfd.size_user;
+			copied += sfd.size_user;
+			len -= sfd.size_user;
+		}
+	}
+	release_sock(sk);
+
+	sfd.num_subflows = sfcount;
+	if (mptcp_put_subflow_data(&sfd, optval, copied, optlen))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int mptcp_put_int_option(struct mptcp_sock *msk, char __user *optval,
 				int __user *optlen, int val)
 {
@@ -1219,6 +1292,8 @@ static int mptcp_getsockopt_sol_mptcp(struct mptcp_sock *msk, int optname,
 	switch (optname) {
 	case MPTCP_INFO:
 		return mptcp_getsockopt_info(msk, optval, optlen);
+	case MPTCP_FULL_INFO:
+		return mptcp_getsockopt_full_info(msk, optval, optlen);
 	case MPTCP_TCPINFO:
 		return mptcp_getsockopt_tcpinfo(msk, optval, optlen);
 	case MPTCP_SUBFLOW_ADDRS:
-- 
2.40.1

[PATCH mptcp-next v2 3/6] mptcp: move snd_una update earlier for fallback socket.

[Paolo Abeni]

That will avoid an unneeded conditional in both the fast-path
and in the fallback case and will simplify a bit the next
patch.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/options.c  | 6 ++++++
 net/mptcp/protocol.c | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 8a8083207be4..4bdcd2b326bd 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1119,6 +1119,12 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 		mptcp_data_lock(subflow->conn);
 		if (sk_stream_memory_free(sk))
 			__mptcp_check_push(subflow->conn, sk);
+
+		/* on fallback we just need to ignore the msk-level snd_una, as
+		 * this is really plain TCP
+		 */
+		msk->snd_una = READ_ONCE(msk->snd_nxt);
+
 		__mptcp_data_acked(subflow->conn);
 		mptcp_data_unlock(subflow->conn);
 		return true;
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b2f0408a5d7c..d93c8cdeeda7 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1006,12 +1006,6 @@ static void __mptcp_clean_una(struct sock *sk)
 	struct mptcp_data_frag *dtmp, *dfrag;
 	u64 snd_una;
 
-	/* on fallback we just need to ignore snd_una, as this is really
-	 * plain TCP
-	 */
-	if (__mptcp_check_fallback(msk))
-		msk->snd_una = READ_ONCE(msk->snd_nxt);
-
 	snd_una = msk->snd_una;
 	list_for_each_entry_safe(dfrag, dtmp, &msk->rtx_queue, list) {
 		if (after64(dfrag->data_seq + dfrag->data_len, snd_una))
-- 
2.40.1

[PATCH mptcp-next v2 4/6] mptcp: track some aggregate data counters.

[Paolo Abeni]

Currently there are no data transfer counters accounting for all
the subflows used by a given MPTCP socket. The user-space can compute
such figures aggregating the subflow info, but that is inaccurate
if any subflow is closed before the MPTCP socket itself.

Add the new counters in the MPTCP socket itself and expose them
via the existing diag and sockopt. While touching mptcp_diag_fill_info(),
acquire the relevant locks before fetching the msk data, to ensure
better data consistency

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/uapi/linux/mptcp.h |  5 +++++
 net/mptcp/options.c        | 10 ++++++++--
 net/mptcp/protocol.c       | 12 +++++++++++-
 net/mptcp/protocol.h       |  4 ++++
 net/mptcp/sockopt.c        | 22 +++++++++++++++++-----
 5 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index 483d4d2a3646..0e12ec7e05d0 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -124,6 +124,11 @@ struct mptcp_info {
 	__u8	mptcpi_local_addr_used;
 	__u8	mptcpi_local_addr_max;
 	__u8	mptcpi_csum_enabled;
+	__u32	mptcpi_retransmits;
+	__u64	mptcpi_bytes_retrans;
+	__u64	mptcpi_bytes_sent;
+	__u64	mptcpi_bytes_received;
+	__u64	mptcpi_bytes_acked;
 };
 
 /*
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 4bdcd2b326bd..c254accb14de 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1026,6 +1026,12 @@ u64 __mptcp_expand_seq(u64 old_seq, u64 cur_seq)
 	return cur_seq;
 }
 
+static void __mptcp_snd_una_update(struct mptcp_sock *msk, u64 new_snd_una)
+{
+	msk->bytes_acked += new_snd_una - msk->snd_una;
+	msk->snd_una = new_snd_una;
+}
+
 static void ack_update_msk(struct mptcp_sock *msk,
 			   struct sock *ssk,
 			   struct mptcp_options_received *mp_opt)
@@ -1057,7 +1063,7 @@ static void ack_update_msk(struct mptcp_sock *msk,
 		__mptcp_check_push(sk, ssk);
 
 	if (after64(new_snd_una, old_snd_una)) {
-		msk->snd_una = new_snd_una;
+		__mptcp_snd_una_update(msk, new_snd_una);
 		__mptcp_data_acked(sk);
 	}
 	mptcp_data_unlock(sk);
@@ -1123,7 +1129,7 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 		/* on fallback we just need to ignore the msk-level snd_una, as
 		 * this is really plain TCP
 		 */
-		msk->snd_una = READ_ONCE(msk->snd_nxt);
+		__mptcp_snd_una_update(msk, READ_ONCE(msk->snd_nxt));
 
 		__mptcp_data_acked(subflow->conn);
 		mptcp_data_unlock(subflow->conn);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d93c8cdeeda7..edf26782fe2e 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -378,6 +378,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 */
+		msk->bytes_received += copy_len;
 		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))
@@ -761,6 +762,7 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
 			MPTCP_SKB_CB(skb)->map_seq += delta;
 			__skb_queue_tail(&sk->sk_receive_queue, skb);
 		}
+		msk->bytes_received += end_seq - msk->ack_seq;
 		msk->ack_seq = end_seq;
 		moved = true;
 	}
@@ -1513,8 +1515,10 @@ static void mptcp_update_post_push(struct mptcp_sock *msk,
 	 * that has been handed to the subflow for transmission
 	 * and skip update in case it was old dfrag.
 	 */
-	if (likely(after64(snd_nxt_new, msk->snd_nxt)))
+	if (likely(after64(snd_nxt_new, msk->snd_nxt))) {
+		msk->bytes_sent += snd_nxt_new - msk->snd_nxt;
 		msk->snd_nxt = snd_nxt_new;
+	}
 }
 
 void mptcp_check_and_set_pending(struct sock *sk)
@@ -2639,6 +2643,8 @@ static void __mptcp_retrans(struct sock *sk)
 			msk->last_snd = ssk;
 		}
 	}
+
+	msk->bytes_retrans += len;
 	dfrag->already_sent = max(dfrag->already_sent, len);
 
 reset_timer:
@@ -3153,6 +3159,10 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
 	mptcp_pm_data_reset(msk);
 	mptcp_ca_reset(sk);
+	msk->bytes_acked = 0;
+	msk->bytes_received = 0;
+	msk->bytes_sent = 0;
+	msk->bytes_retrans = 0;
 
 	WRITE_ONCE(sk->sk_shutdown, 0);
 	sk_error_report(sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index f9180ecce5e4..0283383a09f4 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -261,10 +261,13 @@ struct mptcp_sock {
 	u64		local_key;
 	u64		remote_key;
 	u64		write_seq;
+	u64		bytes_sent;
 	u64		snd_nxt;
+	u64		bytes_received;
 	u64		ack_seq;
 	atomic64_t	rcv_wnd_sent;
 	u64		rcv_data_fin_seq;
+	u64		bytes_retrans;
 	int		rmem_fwd_alloc;
 	struct sock	*last_snd;
 	int		snd_burst;
@@ -273,6 +276,7 @@ struct mptcp_sock {
 						 * recovery related fields are under data_lock
 						 * protection
 						 */
+	u64		bytes_acked;
 	u64		snd_una;
 	u64		wnd_end;
 	unsigned long	timer_ival;
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index b4d04d5dc1f7..2464100ef820 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -888,7 +888,9 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
 
 void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 {
+	struct sock *sk = (struct sock *)msk;
 	u32 flags = 0;
+	bool slow;
 
 	memset(info, 0, sizeof(*info));
 
@@ -914,11 +916,21 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 	if (READ_ONCE(msk->can_ack))
 		flags |= MPTCP_INFO_FLAG_REMOTE_KEY_RECEIVED;
 	info->mptcpi_flags = flags;
-	info->mptcpi_token = READ_ONCE(msk->token);
-	info->mptcpi_write_seq = READ_ONCE(msk->write_seq);
-	info->mptcpi_snd_una = READ_ONCE(msk->snd_una);
-	info->mptcpi_rcv_nxt = READ_ONCE(msk->ack_seq);
-	info->mptcpi_csum_enabled = READ_ONCE(msk->csum_enabled);
+	mptcp_data_lock(sk);
+	info->mptcpi_snd_una = msk->snd_una;
+	info->mptcpi_rcv_nxt = msk->ack_seq;
+	info->mptcpi_bytes_acked = msk->bytes_acked;
+	mptcp_data_unlock(sk);
+
+	slow = lock_sock_fast(sk);
+	info->mptcpi_csum_enabled = msk->csum_enabled;
+	info->mptcpi_token = msk->token;
+	info->mptcpi_write_seq = msk->write_seq;
+	info->mptcpi_retransmits = inet_csk(sk)->icsk_retransmits;
+	info->mptcpi_bytes_sent = msk->bytes_sent;
+	info->mptcpi_bytes_received = msk->bytes_received;
+	info->mptcpi_bytes_retrans = msk->bytes_retrans;
+	unlock_sock_fast(sk, slow);
 }
 EXPORT_SYMBOL_GPL(mptcp_diag_fill_info);
 
-- 
2.40.1

[PATCH mptcp-next v2 5/6] selftests: mptcp: explicitly tests aggregate counters

[Paolo Abeni]

Update the existing sockopt test-case to do some some
basic checks on the newly added counters.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../selftests/net/mptcp/mptcp_sockopt.c       | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index ae61f39556ca..869c041c34f3 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -51,6 +51,11 @@ struct mptcp_info {
 	__u8	mptcpi_local_addr_used;
 	__u8	mptcpi_local_addr_max;
 	__u8	mptcpi_csum_enabled;
+	__u32	mptcpi_retransmits;
+	__u64	mptcpi_bytes_retrans;
+	__u64	mptcpi_bytes_sent;
+	__u64	mptcpi_bytes_received;
+	__u64	mptcpi_bytes_acked;
 };
 
 struct mptcp_subflow_data {
@@ -83,8 +88,10 @@ struct mptcp_subflow_addrs {
 
 struct so_state {
 	struct mptcp_info mi;
+	struct mptcp_info last_sample;
 	uint64_t mptcpi_rcv_delta;
 	uint64_t tcpi_rcv_delta;
+	bool pkt_stats_avail;
 };
 
 static void die_perror(const char *msg)
@@ -320,6 +327,9 @@ static void do_getsockopt_mptcp_info(struct so_state *s, int fd, size_t w)
 
 	assert(olen == sizeof(i));
 
+	s->pkt_stats_avail = olen == sizeof(i);
+
+	s->last_sample = i;
 	if (s->mi.mptcpi_write_seq == 0)
 		s->mi = i;
 
@@ -556,6 +566,23 @@ static void process_one_client(int fd, int pipefd)
 	do_getsockopts(&s, fd, ret, ret2);
 	if (s.mptcpi_rcv_delta != (uint64_t)ret + 1)
 		xerror("mptcpi_rcv_delta %" PRIu64 ", expect %" PRIu64, s.mptcpi_rcv_delta, ret + 1, s.mptcpi_rcv_delta - ret);
+
+	/* be nice when running on top of older kernel */
+	if (s.pkt_stats_avail) {
+		if (s.last_sample.mptcpi_bytes_sent != ret2)
+			xerror("mptcpi_bytes_sent %" PRIu64 ", expect %" PRIu64,
+			       s.last_sample.mptcpi_bytes_sent, ret2,
+			       s.last_sample.mptcpi_bytes_sent - ret2);
+		if (s.last_sample.mptcpi_bytes_received != ret)
+			xerror("mptcpi_bytes_received %" PRIu64 ", expect %" PRIu64,
+			       s.last_sample.mptcpi_bytes_received, ret,
+			       s.last_sample.mptcpi_bytes_received - ret);
+		if (s.last_sample.mptcpi_bytes_acked != ret)
+			xerror("mptcpi_bytes_acked %" PRIu64 ", expect %" PRIu64,
+			       s.last_sample.mptcpi_bytes_acked, ret2,
+			       s.last_sample.mptcpi_bytes_acked - ret2);
+	}
+
 	close(fd);
 }
 
-- 
2.40.1

[PATCH mptcp-next v2 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase

[Paolo Abeni]

Add a testcase explicitly triggering the newly introduce
MPTCP_FULL_INFO getsockopt.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../selftests/net/mptcp/mptcp_sockopt.c       | 76 ++++++++++++++++++-
 1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index 869c041c34f3..598e0f2c592c 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -86,9 +86,25 @@ struct mptcp_subflow_addrs {
 #define MPTCP_SUBFLOW_ADDRS	3
 #endif
 
+#ifndef MPTCP_FULL_INFO
+struct mptcp_subflow_info {
+	__u32				id;
+	struct mptcp_subflow_addrs	addrs;
+};
+
+struct mptcp_subflow_full_info {
+	struct mptcp_subflow_info	subflow_info;
+	struct tcp_info			tcp_info;
+};
+
+#define MPTCP_FULL_INFO		4
+#endif
+
 struct so_state {
 	struct mptcp_info mi;
 	struct mptcp_info last_sample;
+	struct tcp_info tcp_info;
+	struct mptcp_subflow_addrs addrs;
 	uint64_t mptcpi_rcv_delta;
 	uint64_t tcpi_rcv_delta;
 	bool pkt_stats_avail;
@@ -367,6 +383,8 @@ static void do_getsockopt_tcp_info(struct so_state *s, int fd, size_t r, size_t
 		olen -= sizeof(struct mptcp_subflow_data);
 		assert(olen == sizeof(struct tcp_info));
 
+		s->tcp_info = ti.ti[0];
+
 		if (ti.ti[0].tcpi_bytes_sent == w &&
 		    ti.ti[0].tcpi_bytes_received == r)
 			goto done;
@@ -388,7 +406,7 @@ static void do_getsockopt_tcp_info(struct so_state *s, int fd, size_t r, size_t
 	do_getsockopt_bogus_sf_data(fd, MPTCP_TCPINFO);
 }
 
-static void do_getsockopt_subflow_addrs(int fd)
+static void do_getsockopt_subflow_addrs(struct so_state *s, int fd)
 {
 	struct sockaddr_storage remote, local;
 	socklen_t olen, rlen, llen;
@@ -435,6 +453,7 @@ static void do_getsockopt_subflow_addrs(int fd)
 
 	assert(memcmp(&local, &addrs.addr[0].ss_local, sizeof(local)) == 0);
 	assert(memcmp(&remote, &addrs.addr[0].ss_remote, sizeof(remote)) == 0);
+	s->addrs = addrs.addr[0];
 
 	memset(&addrs, 0, sizeof(addrs));
 
@@ -455,13 +474,66 @@ static void do_getsockopt_subflow_addrs(int fd)
 	do_getsockopt_bogus_sf_data(fd, MPTCP_SUBFLOW_ADDRS);
 }
 
+static void do_getsockopt_mptcp_full_info(struct so_state *s, int fd)
+{
+	size_t data_size = 2 * sizeof(struct mptcp_subflow_full_info) +
+			    sizeof(struct mptcp_subflow_data) +
+			    sizeof(struct mptcp_info);
+	struct mptcp_subflow_full_info *sfi;
+	struct mptcp_subflow_data *sfd;
+	void *data = alloca(data_size);
+	struct mptcp_info *mi;
+	socklen_t olen;
+	int ret;
+
+	sfd = data;
+	memset(data, 0, data_size);
+
+	sfd->size_subflow_data = sizeof(struct mptcp_subflow_data);
+	sfd->size_user = sizeof(struct mptcp_subflow_full_info);
+	olen = data_size;
+
+	ret = getsockopt(fd, SOL_MPTCP, MPTCP_FULL_INFO, data, &olen);
+	if (ret < 0) {
+		if (errno == EOPNOTSUPP) {
+			fprintf(stderr, "\tMPTCP_FULL_INFO test skipped due to lack of kernel support\n");
+			return;
+		}
+		xerror("getsockopt MPTCP_FULL_INFO");
+	}
+
+	assert(olen <= data_size);
+	assert(sfd->size_user == sfd->size_kernel);
+	assert(sfd->size_user == sizeof(struct mptcp_subflow_full_info));
+	assert(sfd->num_subflows == 1);
+
+	assert(olen > (socklen_t)sizeof(struct mptcp_subflow_data));
+	olen -= sizeof(struct mptcp_subflow_data);
+	assert(olen > (socklen_t)sizeof(struct mptcp_info));
+	mi = data + sizeof(struct mptcp_subflow_data);
+	assert(mi->mptcpi_subflows == 0);
+	assert(mi->mptcpi_bytes_sent == s->last_sample.mptcpi_bytes_sent);
+	assert(mi->mptcpi_bytes_received == s->last_sample.mptcpi_bytes_received);
+
+	olen -= sizeof(struct mptcp_info);
+	assert(olen == sizeof(struct mptcp_subflow_full_info));
+	sfi = data + sizeof(struct mptcp_subflow_data) + sizeof(struct mptcp_info);
+	assert(sfi->subflow_info.id == 1);
+	assert(sfi->tcp_info.tcpi_bytes_sent == s->tcp_info.tcpi_bytes_sent);
+	assert(sfi->tcp_info.tcpi_bytes_received == s->tcp_info.tcpi_bytes_received);
+	assert(!memcmp(&sfi->subflow_info.addrs, &s->addrs, sizeof(struct mptcp_subflow_addrs)));
+}
+
 static void do_getsockopts(struct so_state *s, int fd, size_t r, size_t w)
 {
 	do_getsockopt_mptcp_info(s, fd, w);
 
 	do_getsockopt_tcp_info(s, fd, r, w);
 
-	do_getsockopt_subflow_addrs(fd);
+	do_getsockopt_subflow_addrs(s, fd);
+
+	if (r)
+		do_getsockopt_mptcp_full_info(s, fd);
 }
 
 static void connect_one_server(int fd, int pipefd)
-- 
2.40.1

Re: [PATCH mptcp-next v2 1/6] mptcp: add subflow unique id

[Matthieu Baerts]

Hi Paolo,

On 22/05/2023 15:56, Paolo Abeni wrote:
> The user-space need to preperly account the data received/sent by
> individual subflows. When additional subflows are created and/or
> closed during the MPTCP socket lifetime, the information currently
> exposed via MPTCP_TCPINFO are not enough: subflows are identifed only
> by the sequential position inside the info dumps, and that will change
> with the above mentioned events.
> 
> To solve the above problem, this patch introduces a new subflow identifier
> that is unique inside the given mptcp socket scope.
> 
> The initial subflow get the id 1 and the other subflows get incremental
> values at join time.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
>  - properly set subflow_id for the first passive subflow and active subflows, too
>  - drop the tcpi_fackets overload
> ---

(...)

> @@ -3203,6 +3206,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
>  	msk->snd_nxt = msk->write_seq;
>  	msk->snd_una = msk->write_seq;
>  	msk->wnd_end = msk->snd_nxt + req->rsk_rcv_wnd;
> +	msk->subflow_id++;

It is a bit confusing. Just to be sure, here it would be the same to do

  msk->subflow_id = 1;

because subflow_id from the listener will be 0, right?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v2 5/6] selftests: mptcp: explicitly tests aggregate counters

[Matthieu Baerts]

On 22/05/2023 15:56, Paolo Abeni wrote:
> Update the existing sockopt test-case to do some some
> basic checks on the newly added counters.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  .../selftests/net/mptcp/mptcp_sockopt.c       | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> index ae61f39556ca..869c041c34f3 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> @@ -51,6 +51,11 @@ struct mptcp_info {
>  	__u8	mptcpi_local_addr_used;
>  	__u8	mptcpi_local_addr_max;
>  	__u8	mptcpi_csum_enabled;
> +	__u32	mptcpi_retransmits;
> +	__u64	mptcpi_bytes_retrans;
> +	__u64	mptcpi_bytes_sent;
> +	__u64	mptcpi_bytes_received;
> +	__u64	mptcpi_bytes_acked;
>  };
>  
>  struct mptcp_subflow_data {
> @@ -83,8 +88,10 @@ struct mptcp_subflow_addrs {
>  
>  struct so_state {
>  	struct mptcp_info mi;
> +	struct mptcp_info last_sample;
>  	uint64_t mptcpi_rcv_delta;
>  	uint64_t tcpi_rcv_delta;
> +	bool pkt_stats_avail;
>  };
>  
>  static void die_perror(const char *msg)
> @@ -320,6 +327,9 @@ static void do_getsockopt_mptcp_info(struct so_state *s, int fd, size_t w)
>  
>  	assert(olen == sizeof(i));

(see below)

> +	s->pkt_stats_avail = olen == sizeof(i);

Good idea, it is simple! But then we should remove the assert just above.

And maybe we should have here: olen >= sizeof(i), no?
Maybe we don't need to support newer kernels but it is just in case we
add more info later even if I guess we would modify mptcp_info if we does.

Also, maybe "struct mptcp_info" will be defined somewhere else and we
will not use the one defined here above?

> +
> +	s->last_sample = i;
>  	if (s->mi.mptcpi_write_seq == 0)
>  		s->mi = i;
>  
> @@ -556,6 +566,23 @@ static void process_one_client(int fd, int pipefd)
>  	do_getsockopts(&s, fd, ret, ret2);
>  	if (s.mptcpi_rcv_delta != (uint64_t)ret + 1)
>  		xerror("mptcpi_rcv_delta %" PRIu64 ", expect %" PRIu64, s.mptcpi_rcv_delta, ret + 1, s.mptcpi_rcv_delta - ret);
> +
> +	/* be nice when running on top of older kernel */
> +	if (s.pkt_stats_avail) {
> +		if (s.last_sample.mptcpi_bytes_sent != ret2)
> +			xerror("mptcpi_bytes_sent %" PRIu64 ", expect %" PRIu64,
> +			       s.last_sample.mptcpi_bytes_sent, ret2,
> +			       s.last_sample.mptcpi_bytes_sent - ret2);
> +		if (s.last_sample.mptcpi_bytes_received != ret)
> +			xerror("mptcpi_bytes_received %" PRIu64 ", expect %" PRIu64,
> +			       s.last_sample.mptcpi_bytes_received, ret,
> +			       s.last_sample.mptcpi_bytes_received - ret);
> +		if (s.last_sample.mptcpi_bytes_acked != ret)
> +			xerror("mptcpi_bytes_acked %" PRIu64 ", expect %" PRIu64,
> +			       s.last_sample.mptcpi_bytes_acked, ret2,
> +			       s.last_sample.mptcpi_bytes_acked - ret2);
> +	}
> +
>  	close(fd);
>  }
>  

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v2 2/6] mptcp: introduce MPTCP_FULL_INFO getsockopt

[Matthieu Baerts]

Hi Paolo,

On 22/05/2023 15:56, Paolo Abeni wrote:
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Do you mind motivating the addition of this new socket option? :)

Also, it is not clear how we manage the different size for struct
tcp_info and struct mptcp_subflow_addrs but I will look at that in more
details later.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v2 1/6] mptcp: add subflow unique id

[Paolo Abeni]

On Tue, 2023-05-23 at 17:20 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 22/05/2023 15:56, Paolo Abeni wrote:
> > The user-space need to preperly account the data received/sent by
> > individual subflows. When additional subflows are created and/or
> > closed during the MPTCP socket lifetime, the information currently
> > exposed via MPTCP_TCPINFO are not enough: subflows are identifed only
> > by the sequential position inside the info dumps, and that will change
> > with the above mentioned events.
> > 
> > To solve the above problem, this patch introduces a new subflow identifier
> > that is unique inside the given mptcp socket scope.
> > 
> > The initial subflow get the id 1 and the other subflows get incremental
> > values at join time.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > v1 -> v2:
> >  - properly set subflow_id for the first passive subflow and active subflows, too
> >  - drop the tcpi_fackets overload
> > ---
> 
> (...)
> 
> > @@ -3203,6 +3206,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
> >  	msk->snd_nxt = msk->write_seq;
> >  	msk->snd_una = msk->write_seq;
> >  	msk->wnd_end = msk->snd_nxt + req->rsk_rcv_wnd;
> > +	msk->subflow_id++;
> 
> It is a bit confusing. Just to be sure, here it would be the same to do
> 
>   msk->subflow_id = 1;
> 
> because subflow_id from the listener will be 0, right?

You are right, my code above is confusing and even incorrect. I'll
replace it with the plain assignment.

Thanks!

/P

Re: [PATCH mptcp-next v2 2/6] mptcp: introduce MPTCP_FULL_INFO getsockopt

[Paolo Abeni]

On Tue, 2023-05-23 at 17:31 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 22/05/2023 15:56, Paolo Abeni wrote:
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> Do you mind motivating the addition of this new socket option? :)

Sure, I will do.

> Also, it is not clear how we manage the different size for struct
> tcp_info and struct mptcp_subflow_addrs but I will look at that in more
> details later.

This patch does not allow for different mptcp_subflow_addrs size. I
hope it's not a problem??!

/P

Re: [PATCH mptcp-next v2 2/6] mptcp: introduce MPTCP_FULL_INFO getsockopt

[Matthieu Baerts]

Hi Paolo,

On 23/05/2023 18:46, Paolo Abeni wrote:
> On Tue, 2023-05-23 at 17:31 +0200, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 22/05/2023 15:56, Paolo Abeni wrote:
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>
>> Do you mind motivating the addition of this new socket option? :)
> 
> Sure, I will do.
> 
>> Also, it is not clear how we manage the different size for struct
>> tcp_info and struct mptcp_subflow_addrs but I will look at that in more
>> details later.
> 
> This patch does not allow for different mptcp_subflow_addrs size. I
> hope it's not a problem??!

I guess it is fine, it should not change. But then it is probably better
to add a note in mptcp.h, no?

The alternative is write something like:

 - header with the features we want + number of subflows per feature
(and the kernel can write how many were available)
 - feature 1 (tcp_info):
   - mptcp_subflow_data structure
   - tcp_info for each subflow
 - feature 2 (addr):
   - mptcp_subflow_data structure
   - addr for each subflow

So we can accept multiple features and we can pick what we want.

Just an idea, maybe it is not useful or too complex to manage.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v2 5/6] selftests: mptcp: explicitly tests aggregate counters

[Paolo Abeni]

On Tue, 2023-05-23 at 17:29 +0200, Matthieu Baerts wrote:
> 
> On 22/05/2023 15:56, Paolo Abeni wrote:
> > Update the existing sockopt test-case to do some some
> > basic checks on the newly added counters.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  .../selftests/net/mptcp/mptcp_sockopt.c       | 27 +++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> > index ae61f39556ca..869c041c34f3 100644
> > --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> > @@ -51,6 +51,11 @@ struct mptcp_info {
> >  	__u8	mptcpi_local_addr_used;
> >  	__u8	mptcpi_local_addr_max;
> >  	__u8	mptcpi_csum_enabled;
> > +	__u32	mptcpi_retransmits;
> > +	__u64	mptcpi_bytes_retrans;
> > +	__u64	mptcpi_bytes_sent;
> > +	__u64	mptcpi_bytes_received;
> > +	__u64	mptcpi_bytes_acked;
> >  };
> >  
> >  struct mptcp_subflow_data {
> > @@ -83,8 +88,10 @@ struct mptcp_subflow_addrs {
> >  
> >  struct so_state {
> >  	struct mptcp_info mi;
> > +	struct mptcp_info last_sample;
> >  	uint64_t mptcpi_rcv_delta;
> >  	uint64_t tcpi_rcv_delta;
> > +	bool pkt_stats_avail;
> >  };
> >  
> >  static void die_perror(const char *msg)
> > @@ -320,6 +327,9 @@ static void do_getsockopt_mptcp_info(struct so_state *s, int fd, size_t w)
> >  
> >  	assert(olen == sizeof(i));
> 
> (see below)
> 
> > +	s->pkt_stats_avail = olen == sizeof(i);
> 
> Good idea, it is simple! But then we should remove the assert just above.
> 
> And maybe we should have here: olen >= sizeof(i), no?
> Maybe we don't need to support newer kernels but it is just in case we
> add more info later even if I guess we would modify mptcp_info if we does.
> 
> Also, maybe "struct mptcp_info" will be defined somewhere else and we
> will not use the one defined here above?

Yep, I'll use:

	olen >= sizeof(i)

thanks!

/P

Re: [PATCH mptcp-next v2 2/6] mptcp: introduce MPTCP_FULL_INFO getsockopt

[Paolo Abeni]

On Tue, 2023-05-23 at 18:55 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 23/05/2023 18:46, Paolo Abeni wrote:
> > On Tue, 2023-05-23 at 17:31 +0200, Matthieu Baerts wrote:
> > > Hi Paolo,
> > > 
> > > On 22/05/2023 15:56, Paolo Abeni wrote:
> > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > 
> > > Do you mind motivating the addition of this new socket option? :)
> > 
> > Sure, I will do.
> > 
> > > Also, it is not clear how we manage the different size for struct
> > > tcp_info and struct mptcp_subflow_addrs but I will look at that in more
> > > details later.
> > 
> > This patch does not allow for different mptcp_subflow_addrs size. I
> > hope it's not a problem??!
> 
> I guess it is fine, it should not change. But then it is probably better
> to add a note in mptcp.h, no?
> 
> The alternative is write something like:
> 
>  - header with the features we want + number of subflows per feature
> (and the kernel can write how many were available)
>  - feature 1 (tcp_info):
>    - mptcp_subflow_data structure
>    - tcp_info for each subflow
>  - feature 2 (addr):
>    - mptcp_subflow_data structure
>    - addr for each subflow
> 
> So we can accept multiple features and we can pick what we want.
> 
> Just an idea, maybe it is not useful or too complex to manage.

mptcp_subflow_addrs includes 2 __kernel_sockaddr_storage field, so it
should be as future proof as reasonably possible. Even IPv8 should fit
the 128 bytes there ;)

If not, we can always add an MPTCP_FULLER_INFO sock opt ;)

I think the current schema should work. I'll add a note in the uapi
file.

Thanks!

/P