* [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info
@ 2023-05-25 7:17 Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 1/6] mptcp: move snd_una update earlier for fallback socket Paolo Abeni
` (6 more replies)
0 siblings, 7 replies; 19+ messages in thread
From: Paolo Abeni @ 2023-05-25 7:17 UTC (permalink / raw)
To: mptcp; +Cc: Florian Westphal
Sorry for the high-freq spamming. I really want to close this topic
and move forward.
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".
v5 -> v6:
- fix another compiler warning
v4 -> v5:
- changed again binary layout for MPTCP_FULL_INFO structs (Matttbe)
- fixed 32bit build issue
- reordered the patches, less controversial first
v3 -> v4:
- change binary layout for MPTCP_FULL_INFO structs (Florian)
v2 -> v3:
- address Matttbe comments on patch 1, 2 and 5, see the indivdual
patches changelog for the details
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: move snd_una update earlier for fallback socket.
mptcp: track some aggregate data counters.
selftests: mptcp: explicitly tests aggregate counters
mptcp: add subflow unique id
mptcp: introduce MPTCP_FULL_INFO getsockopt
selftests: mptcp: add MPTCP_FULL_INFO testcase
include/uapi/linux/mptcp.h | 29 ++++
net/mptcp/options.c | 14 +-
net/mptcp/protocol.c | 24 ++-
net/mptcp/protocol.h | 9 +-
net/mptcp/sockopt.c | 149 +++++++++++++++++-
net/mptcp/subflow.c | 2 +
.../selftests/net/mptcp/mptcp_sockopt.c | 118 +++++++++++++-
7 files changed, 326 insertions(+), 19 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 mptcp-next 1/6] mptcp: move snd_una update earlier for fallback socket.
2023-05-25 7:17 [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Paolo Abeni
@ 2023-05-25 7:17 ` Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 2/6] mptcp: track some aggregate data counters Paolo Abeni
` (5 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Paolo Abeni @ 2023-05-25 7:17 UTC (permalink / raw)
To: mptcp; +Cc: Florian Westphal
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 763f709fd5f5..7ecd2f250909 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1004,12 +1004,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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v6 mptcp-next 2/6] mptcp: track some aggregate data counters.
2023-05-25 7:17 [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 1/6] mptcp: move snd_una update earlier for fallback socket Paolo Abeni
@ 2023-05-25 7:17 ` Paolo Abeni
2023-05-25 15:19 ` Matthieu Baerts
2023-05-25 7:17 ` [PATCH v6 mptcp-next 3/6] selftests: mptcp: explicitly tests aggregate counters Paolo Abeni
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Paolo Abeni @ 2023-05-25 7:17 UTC (permalink / raw)
To: mptcp; +Cc: Florian Westphal
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 32af2d278cb4..a124be6ebbba 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -123,6 +123,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 7ecd2f250909..79b3059ca3ef 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -377,6 +377,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))
@@ -760,6 +761,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;
}
@@ -1511,8 +1513,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)
@@ -2637,6 +2641,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:
@@ -3150,6 +3156,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 bd3771c7d79d..ec733955f18e 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 d4258869ac48..770279e0a598 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v6 mptcp-next 3/6] selftests: mptcp: explicitly tests aggregate counters
2023-05-25 7:17 [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 1/6] mptcp: move snd_una update earlier for fallback socket Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 2/6] mptcp: track some aggregate data counters Paolo Abeni
@ 2023-05-25 7:17 ` Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 4/6] mptcp: add subflow unique id Paolo Abeni
` (3 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Paolo Abeni @ 2023-05-25 7:17 UTC (permalink / raw)
To: mptcp; +Cc: Florian Westphal
Update the existing sockopt test-case to do some basic checks
on the newly added counters.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v2 -> v3:
- be more kind with older kernel (Matttbe)
---
.../selftests/net/mptcp/mptcp_sockopt.c | 27 ++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index ae61f39556ca..ff8fcdfccf76 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)
@@ -318,8 +325,9 @@ static void do_getsockopt_mptcp_info(struct so_state *s, int fd, size_t w)
if (ret < 0)
die_perror("getsockopt MPTCP_INFO");
- 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 +564,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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v6 mptcp-next 4/6] mptcp: add subflow unique id
2023-05-25 7:17 [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Paolo Abeni
` (2 preceding siblings ...)
2023-05-25 7:17 ` [PATCH v6 mptcp-next 3/6] selftests: mptcp: explicitly tests aggregate counters Paolo Abeni
@ 2023-05-25 7:17 ` Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 5/6] mptcp: introduce MPTCP_FULL_INFO getsockopt Paolo Abeni
` (2 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Paolo Abeni @ 2023-05-25 7:17 UTC (permalink / raw)
To: mptcp; +Cc: Florian Westphal
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>
---
v2 -> v3:
- fix msk subflow_id init (Matttbe)
v1 -> v2:
- properly set subflow_id for the first passive subflow and active subflows, too
- drop the tcpi_fackets overload
---
net/mptcp/protocol.c | 6 ++++++
net/mptcp/protocol.h | 5 ++++-
net/mptcp/subflow.c | 2 ++
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 79b3059ca3ef..895eda3d79e8 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;
@@ -847,6 +848,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);
@@ -3210,6 +3213,9 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
msk->setsockopt_seq = mptcp_sk(sk)->setsockopt_seq;
mptcp_init_sched(msk, mptcp_sk(sk)->sched);
+ /* passive msk is created after the first/MPC subflow */
+ msk->subflow_id = 2;
+
sock_reset_flag(nsk, SOCK_RCU_FREE);
security_inet_csk_clone(nsk, req);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ec733955f18e..b4b5c5004f34 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -323,7 +323,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;
};
@@ -505,6 +506,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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v6 mptcp-next 5/6] mptcp: introduce MPTCP_FULL_INFO getsockopt
2023-05-25 7:17 [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Paolo Abeni
` (3 preceding siblings ...)
2023-05-25 7:17 ` [PATCH v6 mptcp-next 4/6] mptcp: add subflow unique id Paolo Abeni
@ 2023-05-25 7:17 ` Paolo Abeni
2023-05-25 15:20 ` Matthieu Baerts
2023-05-25 7:17 ` [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase Paolo Abeni
2023-05-25 15:18 ` [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Matthieu Baerts
6 siblings, 1 reply; 19+ messages in thread
From: Paolo Abeni @ 2023-05-25 7:17 UTC (permalink / raw)
To: mptcp; +Cc: Florian Westphal
Some user-space applications want to monitor the subflows utilization.
Dumping the per subflow tcp_info is not enough, as the PM could close
and re-create the subflows under-the-hood, fooling the accounting.
Even checking the src/dst addresses used by each subflow could not
be enough, because new subflows could re-use the same address/port of
the just closed one.
This patch introduces a new socket option, allow dumping all the relevant
information all-at-once (everything, everywhere...), in a consistent
manner.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v5 -> v6:
- fix compiler warning - unused variable
v4 -> v5:
- full_info struct re-design (Florian)
- fix build issue on 32 bit hosts
v3 -> v4:
- full_info struct re-design (Florian)
v2 -> v3:
- added missing changelog (oops)
---
include/uapi/linux/mptcp.h | 24 +++++++
net/mptcp/sockopt.c | 127 ++++++++++++++++++++++++++++++++++++-
2 files changed, 149 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index a124be6ebbba..ee9c49f949a2 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -249,9 +249,33 @@ struct mptcp_subflow_addrs {
};
};
+struct mptcp_subflow_info {
+ __u32 id;
+ struct mptcp_subflow_addrs addrs;
+};
+
+struct mptcp_full_info {
+ __u32 size_tcpinfo_kernel; /* must be 0, set by kernel */
+ __u32 size_tcpinfo_user;
+ __u32 size_sfinfo_kernel; /* must be 0, set by kernel */
+ __u32 size_sfinfo_user;
+ __u32 num_subflows; /* must be 0, set by kernel (real subflow count) */
+ __u32 size_arrays_user; /* max subflows that userspace is interested in;
+ * the buffers at subflow_info/tcp_info
+ * are respectively at least:
+ * size_arrays * size_sfinfo_user
+ * size_arrays * size_tcpinfo_user
+ * bytes wide
+ */
+ __aligned_u64 subflow_info;
+ __aligned_u64 tcp_info;
+ struct mptcp_info mptcp_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 770279e0a598..bfbe0378e997 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -14,7 +14,8 @@
#include <net/mptcp.h>
#include "protocol.h"
-#define MIN_INFO_OPTLEN_SIZE 16
+#define MIN_INFO_OPTLEN_SIZE 16
+#define MIN_FULL_INFO_OPTLEN_SIZE 40
static struct sock *__mptcp_tcp_fallback(struct mptcp_sock *msk)
{
@@ -977,7 +978,8 @@ static int mptcp_put_subflow_data(struct mptcp_subflow_data *sfd,
}
static int mptcp_get_subflow_data(struct mptcp_subflow_data *sfd,
- char __user *optval, int __user *optlen)
+ char __user *optval,
+ int __user *optlen)
{
int len, copylen;
@@ -1158,6 +1160,125 @@ static int mptcp_getsockopt_subflow_addrs(struct mptcp_sock *msk, char __user *o
return 0;
}
+static int mptcp_get_full_info(struct mptcp_full_info *mfi,
+ char __user *optval,
+ int __user *optlen)
+{
+ int len;
+
+ BUILD_BUG_ON(offsetof(struct mptcp_full_info, mptcp_info) !=
+ MIN_FULL_INFO_OPTLEN_SIZE);
+
+ if (get_user(len, optlen))
+ return -EFAULT;
+
+ if (len < MIN_FULL_INFO_OPTLEN_SIZE)
+ return -EINVAL;
+
+ memset(mfi, 0, sizeof(*mfi));
+ if (copy_from_user(mfi, optval, MIN_FULL_INFO_OPTLEN_SIZE))
+ return -EFAULT;
+
+ if (mfi->size_tcpinfo_kernel ||
+ mfi->size_sfinfo_kernel ||
+ mfi->num_subflows)
+ return -EINVAL;
+
+ if (mfi->size_sfinfo_user > INT_MAX ||
+ mfi->size_tcpinfo_user > INT_MAX)
+ return -EINVAL;
+
+ return len - MIN_FULL_INFO_OPTLEN_SIZE;
+}
+
+static int mptcp_put_full_info(struct mptcp_full_info *mfi,
+ char __user *optval,
+ u32 copylen,
+ int __user *optlen)
+{
+ copylen += MIN_FULL_INFO_OPTLEN_SIZE;
+ if (put_user(copylen, optlen))
+ return -EFAULT;
+
+ if (copy_to_user(optval, mfi, copylen))
+ return -EFAULT;
+ return 0;
+}
+
+static int mptcp_getsockopt_full_info(struct mptcp_sock *msk, char __user *optval,
+ int __user *optlen)
+{
+ unsigned int sfcount = 0, copylen = 0;
+ struct mptcp_subflow_context *subflow;
+ struct sock *sk = (struct sock *)msk;
+ void __user *tcpinfoptr, *sfinfoptr;
+ struct mptcp_full_info mfi;
+ int len;
+
+ len = mptcp_get_full_info(&mfi, optval, optlen);
+ if (len < 0)
+ return len;
+
+ /* don't bother filling the mptcp info if there is not enough
+ * user-space-provided storage
+ */
+ if (len > 0) {
+ mptcp_diag_fill_info(msk, &mfi.mptcp_info);
+ copylen += min_t(unsigned int, len, sizeof(struct mptcp_info));
+ }
+
+ mfi.size_tcpinfo_kernel = sizeof(struct tcp_info);
+ mfi.size_tcpinfo_user = min_t(unsigned int, mfi.size_tcpinfo_user,
+ sizeof(struct tcp_info));
+ sfinfoptr = u64_to_user_ptr(mfi.subflow_info);
+ mfi.size_sfinfo_kernel = sizeof(struct mptcp_subflow_info);
+ mfi.size_sfinfo_user = min_t(unsigned int, mfi.size_sfinfo_user,
+ sizeof(struct mptcp_subflow_info));
+ tcpinfoptr = u64_to_user_ptr(mfi.tcp_info);
+
+ lock_sock(sk);
+ mptcp_for_each_subflow(msk, subflow) {
+ struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+ struct mptcp_subflow_info sfinfo;
+ struct tcp_info tcp_info;
+
+ if (sfcount++ >= mfi.size_arrays_user)
+ continue;
+
+ /* fetch addr/tcp_info only if the user space buffers
+ * are wide enough
+ */
+ memset(&sfinfo, 0, sizeof(sfinfo));
+ sfinfo.id = subflow->subflow_id;
+ if (mfi.size_sfinfo_user >
+ offsetof(struct mptcp_subflow_info, addrs))
+ mptcp_get_sub_addrs(ssk, &sfinfo.addrs);
+ if (copy_to_user(sfinfoptr, &sfinfo, mfi.size_sfinfo_user))
+ goto fail_release;
+
+ if (mfi.size_tcpinfo_user) {
+ tcp_get_info(ssk, &tcp_info);
+ if (copy_to_user(tcpinfoptr, &tcp_info,
+ mfi.size_tcpinfo_user))
+ goto fail_release;
+ }
+
+ tcpinfoptr += mfi.size_tcpinfo_user;
+ sfinfoptr += mfi.size_sfinfo_user;
+ }
+ release_sock(sk);
+
+ mfi.num_subflows = sfcount;
+ if (mptcp_put_full_info(&mfi, optval, copylen, optlen))
+ return -EFAULT;
+
+ return 0;
+
+fail_release:
+ release_sock(sk);
+ return -EFAULT;
+}
+
static int mptcp_put_int_option(struct mptcp_sock *msk, char __user *optval,
int __user *optlen, int val)
{
@@ -1231,6 +1352,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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase
2023-05-25 7:17 [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Paolo Abeni
` (4 preceding siblings ...)
2023-05-25 7:17 ` [PATCH v6 mptcp-next 5/6] mptcp: introduce MPTCP_FULL_INFO getsockopt Paolo Abeni
@ 2023-05-25 7:17 ` Paolo Abeni
2023-05-25 8:35 ` selftests: mptcp: add MPTCP_FULL_INFO testcase: Tests Results MPTCP CI
` (2 more replies)
2023-05-25 15:18 ` [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Matthieu Baerts
6 siblings, 3 replies; 19+ messages in thread
From: Paolo Abeni @ 2023-05-25 7:17 UTC (permalink / raw)
To: mptcp; +Cc: Florian Westphal
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 | 91 ++++++++++++++++++-
1 file changed, 89 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index ff8fcdfccf76..568e95599355 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -86,9 +86,38 @@ 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_full_info {
+ __u32 size_tcpinfo_kernel; /* must be 0, set by kernel */
+ __u32 size_tcpinfo_user;
+ __u32 size_sfinfo_kernel; /* must be 0, set by kernel */
+ __u32 size_sfinfo_user;
+ __u32 num_subflows; /* must be 0, set by kernel (real subflow count) */
+ __u32 size_arrays_user; /* max subflows that userspace is interested in;
+ * the buffers at subflow_info/tcp_info
+ * are respectively at least:
+ * size_arrays * size_sfinfo_user
+ * size_arrays * size_tcpinfo_user
+ * bytes wide
+ */
+ __aligned_u64 subflow_info;
+ __aligned_u64 tcp_info;
+ struct mptcp_info mptcp_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;
@@ -365,6 +394,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;
@@ -386,7 +417,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;
@@ -433,6 +464,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));
@@ -453,13 +485,68 @@ 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 = sizeof(struct mptcp_full_info);
+ struct mptcp_subflow_info sfinfo[2];
+ struct tcp_info tcp_info[2];
+ struct mptcp_full_info mfi;
+ socklen_t olen;
+ int ret;
+
+ memset(&mfi, 0, data_size);
+ memset(tcp_info, 0, sizeof(tcp_info));
+ memset(sfinfo, 0, sizeof(sfinfo));
+
+ mfi.size_tcpinfo_user = sizeof(struct tcp_info);
+ mfi.size_sfinfo_user = sizeof(struct mptcp_subflow_info);
+ mfi.size_arrays_user = 2;
+ mfi.subflow_info = (unsigned long) &sfinfo[0];
+ mfi.tcp_info = (unsigned long) &tcp_info[0];
+ olen = data_size;
+
+ ret = getsockopt(fd, SOL_MPTCP, MPTCP_FULL_INFO, &mfi, &olen);
+ if (ret < 0) {
+ if (errno == EOPNOTSUPP) {
+ perror("MPTCP_FULL_INFO test skipped");
+ return;
+ }
+ xerror("getsockopt MPTCP_FULL_INFO");
+ }
+
+ assert(olen <= data_size);
+ assert(mfi.size_tcpinfo_user == mfi.size_tcpinfo_kernel);
+ assert(mfi.size_tcpinfo_user == sizeof(struct tcp_info));
+ assert(mfi.size_sfinfo_user == mfi.size_sfinfo_kernel);
+ assert(mfi.size_sfinfo_user == sizeof(struct mptcp_subflow_info));
+ assert(mfi.num_subflows == 1);
+
+ /* Tolerate future extension to mptcp_info struct and running newer
+ * test on top of older kernel.
+ * Anyway any kernel supporting MPTCP_FULL_INFO must at least include
+ * the following in mptcp_info.
+ */
+ assert(olen > (socklen_t)__builtin_offsetof(struct mptcp_full_info, tcp_info));
+ assert(mfi.mptcp_info.mptcpi_subflows == 0);
+ assert(mfi.mptcp_info.mptcpi_bytes_sent == s->last_sample.mptcpi_bytes_sent);
+ assert(mfi.mptcp_info.mptcpi_bytes_received == s->last_sample.mptcpi_bytes_received);
+
+ assert(sfinfo[0].id == 1);
+ assert(tcp_info[0].tcpi_bytes_sent == s->tcp_info.tcpi_bytes_sent);
+ assert(tcp_info[0].tcpi_bytes_received == s->tcp_info.tcpi_bytes_received);
+ assert(!memcmp(&sfinfo->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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: selftests: mptcp: add MPTCP_FULL_INFO testcase: Tests Results
2023-05-25 7:17 ` [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase Paolo Abeni
@ 2023-05-25 8:35 ` MPTCP CI
2023-05-25 15:22 ` [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase Matthieu Baerts
2023-05-25 16:51 ` selftests: mptcp: add MPTCP_FULL_INFO testcase: Tests Results MPTCP CI
2 siblings, 0 replies; 19+ messages in thread
From: MPTCP CI @ 2023-05-25 8:35 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5296261184618496
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5296261184618496/summary/summary.txt
- KVM Validation: debug (except selftest_mptcp_join):
- Unstable: 4 failed test(s): packetdrill_add_addr packetdrill_dss packetdrill_fastopen selftest_diag 🔴:
- Task: https://cirrus-ci.com/task/5014786207907840
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5014786207907840/summary/summary.txt
- KVM Validation: normal (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/6422161091461120
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6422161091461120/summary/summary.txt
- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/6140686114750464
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6140686114750464/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/0ae31b1c5534
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-debug
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info
2023-05-25 7:17 [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Paolo Abeni
` (5 preceding siblings ...)
2023-05-25 7:17 ` [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase Paolo Abeni
@ 2023-05-25 15:18 ` Matthieu Baerts
2023-05-28 16:25 ` Matthieu Baerts
6 siblings, 1 reply; 19+ messages in thread
From: Matthieu Baerts @ 2023-05-25 15:18 UTC (permalink / raw)
To: Paolo Abeni, mptcp; +Cc: Florian Westphal
Hi Paolo,
On 25/05/2023 09:17, Paolo Abeni wrote:
> Sorry for the high-freq spamming. I really want to close this topic
> and move forward.
>
> 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".
>
> v5 -> v6:
> - fix another compiler warning
>
> v4 -> v5:
> - changed again binary layout for MPTCP_FULL_INFO structs (Matttbe)
> - fixed 32bit build issue
> - reordered the patches, less controversial first
Thank you for the new version! It looks good to me!
Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
I'm sorry I was not confident about the previous version and I was not
feeling good asking you to change it!
I do like the last version, I hope you too :)
I have a few questions in the individual patches if you don't mind but
that should not be blocking the series. I can even do the modifications
when applying the patches if that's OK for you!
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 mptcp-next 2/6] mptcp: track some aggregate data counters.
2023-05-25 7:17 ` [PATCH v6 mptcp-next 2/6] mptcp: track some aggregate data counters Paolo Abeni
@ 2023-05-25 15:19 ` Matthieu Baerts
2023-05-25 16:42 ` Paolo Abeni
0 siblings, 1 reply; 19+ messages in thread
From: Matthieu Baerts @ 2023-05-25 15:19 UTC (permalink / raw)
To: Paolo Abeni, mptcp; +Cc: Florian Westphal
Hi Paolo,
On 25/05/2023 09:17, Paolo Abeni wrote:
> 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 32af2d278cb4..a124be6ebbba 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -123,6 +123,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;
I'm sure sure I followed correctly the discussions about that with
Florian: did we not want to switch to __aligned_u64 for the new fields?
(and why not the old ones as well if it doesn't change anything but at
least it would be uniformed)
If I'm not mistaken, it would not change anything but maybe better to be
on the safe side?
It is not blocking for the moment, a squash-to patch can still be used
later (or I do the modification directly when applying the patches).
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 mptcp-next 5/6] mptcp: introduce MPTCP_FULL_INFO getsockopt
2023-05-25 7:17 ` [PATCH v6 mptcp-next 5/6] mptcp: introduce MPTCP_FULL_INFO getsockopt Paolo Abeni
@ 2023-05-25 15:20 ` Matthieu Baerts
2023-05-25 16:29 ` Paolo Abeni
0 siblings, 1 reply; 19+ messages in thread
From: Matthieu Baerts @ 2023-05-25 15:20 UTC (permalink / raw)
To: Paolo Abeni, mptcp; +Cc: Florian Westphal
Hi Paolo,
On 25/05/2023 09:17, Paolo Abeni wrote:
> Some user-space applications want to monitor the subflows utilization.
>
> Dumping the per subflow tcp_info is not enough, as the PM could close
> and re-create the subflows under-the-hood, fooling the accounting.
> Even checking the src/dst addresses used by each subflow could not
> be enough, because new subflows could re-use the same address/port of
> the just closed one.
>
> This patch introduces a new socket option, allow dumping all the relevant
> information all-at-once (everything, everywhere...), in a consistent
> manner.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v5 -> v6:
> - fix compiler warning - unused variable
>
> v4 -> v5:
> - full_info struct re-design (Florian)
> - fix build issue on 32 bit hosts
Thank you for the update! I find the new version clearer, hopefully
easier to maintain and to use from the userspace side.
(...)
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 770279e0a598..bfbe0378e997 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -14,7 +14,8 @@
> #include <net/mptcp.h>
> #include "protocol.h"
>
> -#define MIN_INFO_OPTLEN_SIZE 16
> +#define MIN_INFO_OPTLEN_SIZE 16
> +#define MIN_FULL_INFO_OPTLEN_SIZE 40
>
> static struct sock *__mptcp_tcp_fallback(struct mptcp_sock *msk)
> {
> @@ -977,7 +978,8 @@ static int mptcp_put_subflow_data(struct mptcp_subflow_data *sfd,
> }
>
> static int mptcp_get_subflow_data(struct mptcp_subflow_data *sfd,
> - char __user *optval, int __user *optlen)
> + char __user *optval,
> + int __user *optlen)
Do you mind if I undo this modification when applying the patch?
Or did you do that on purpose?
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase
2023-05-25 7:17 ` [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase Paolo Abeni
2023-05-25 8:35 ` selftests: mptcp: add MPTCP_FULL_INFO testcase: Tests Results MPTCP CI
@ 2023-05-25 15:22 ` Matthieu Baerts
2023-05-25 16:39 ` Paolo Abeni
2023-05-25 16:51 ` selftests: mptcp: add MPTCP_FULL_INFO testcase: Tests Results MPTCP CI
2 siblings, 1 reply; 19+ messages in thread
From: Matthieu Baerts @ 2023-05-25 15:22 UTC (permalink / raw)
To: Paolo Abeni, mptcp; +Cc: Florian Westphal
Hi Paolo,
On 25/05/2023 09:17, Paolo Abeni wrote:
> Add a testcase explicitly triggering the newly introduce
> MPTCP_FULL_INFO getsockopt.
(...)
> +static void do_getsockopt_mptcp_full_info(struct so_state *s, int fd)
> +{
> + size_t data_size = sizeof(struct mptcp_full_info);
> + struct mptcp_subflow_info sfinfo[2];
> + struct tcp_info tcp_info[2];
> + struct mptcp_full_info mfi;
> + socklen_t olen;
> + int ret;
> +
> + memset(&mfi, 0, data_size);
> + memset(tcp_info, 0, sizeof(tcp_info));
> + memset(sfinfo, 0, sizeof(sfinfo));
> +
> + mfi.size_tcpinfo_user = sizeof(struct tcp_info);
> + mfi.size_sfinfo_user = sizeof(struct mptcp_subflow_info);
> + mfi.size_arrays_user = 2;
> + mfi.subflow_info = (unsigned long) &sfinfo[0];
> + mfi.tcp_info = (unsigned long) &tcp_info[0];
> + olen = data_size;
> +
> + ret = getsockopt(fd, SOL_MPTCP, MPTCP_FULL_INFO, &mfi, &olen);
> + if (ret < 0) {
> + if (errno == EOPNOTSUPP) {
> + perror("MPTCP_FULL_INFO test skipped");
> + return;
> + }
> + xerror("getsockopt MPTCP_FULL_INFO");
> + }
> +
> + assert(olen <= data_size);
> + assert(mfi.size_tcpinfo_user == mfi.size_tcpinfo_kernel);
> + assert(mfi.size_tcpinfo_user == sizeof(struct tcp_info));
> + assert(mfi.size_sfinfo_user == mfi.size_sfinfo_kernel);
> + assert(mfi.size_sfinfo_user == sizeof(struct mptcp_subflow_info));
Similar to the patch I sent to support old kernels, we cannot do these 4
last assert(), see:
https://patchwork.kernel.org/project/mptcp/patch/20230406-mptcp-issue-368-selftests-old-kernels-v2-17-50313e4f83ab@tessares.net/
Because we cannot guess the size of these two structures in the kernel,
I suggest to do this:
assert(mfi.size_tcpinfo_user > 0);
assert(mfi.size_tcpinfo_kernel > 0);
assert(mfi.size_sfinfo_user > 0);
assert(mfi.size_sfinfo_kernel > 0);
Better than nothing... In case of error, one would be set to 0 or we
would not get info from these structures below.
> + assert(mfi.num_subflows == 1);
> +
> + /* Tolerate future extension to mptcp_info struct and running newer
> + * test on top of older kernel.
> + * Anyway any kernel supporting MPTCP_FULL_INFO must at least include
> + * the following in mptcp_info.
> + */
> + assert(olen > (socklen_t)__builtin_offsetof(struct mptcp_full_info, tcp_info));
Likely, this should always be correct because 'mptcp_info' is after
tcp_info, no? On the other hand, hard to do better if we need to
support old kernels... So fine to keep it I suppose.
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 mptcp-next 5/6] mptcp: introduce MPTCP_FULL_INFO getsockopt
2023-05-25 15:20 ` Matthieu Baerts
@ 2023-05-25 16:29 ` Paolo Abeni
0 siblings, 0 replies; 19+ messages in thread
From: Paolo Abeni @ 2023-05-25 16:29 UTC (permalink / raw)
To: Matthieu Baerts, mptcp; +Cc: Florian Westphal
On Thu, 2023-05-25 at 17:20 +0200, Matthieu Baerts wrote:
> Hi Paolo,
>
> On 25/05/2023 09:17, Paolo Abeni wrote:
> > Some user-space applications want to monitor the subflows utilization.
> >
> > Dumping the per subflow tcp_info is not enough, as the PM could close
> > and re-create the subflows under-the-hood, fooling the accounting.
> > Even checking the src/dst addresses used by each subflow could not
> > be enough, because new subflows could re-use the same address/port of
> > the just closed one.
> >
> > This patch introduces a new socket option, allow dumping all the relevant
> > information all-at-once (everything, everywhere...), in a consistent
> > manner.
> >
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > v5 -> v6:
> > - fix compiler warning - unused variable
> >
> > v4 -> v5:
> > - full_info struct re-design (Florian)
> > - fix build issue on 32 bit hosts
>
> Thank you for the update! I find the new version clearer, hopefully
> easier to maintain and to use from the userspace side.
>
> (...)
>
> > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> > index 770279e0a598..bfbe0378e997 100644
> > --- a/net/mptcp/sockopt.c
> > +++ b/net/mptcp/sockopt.c
> > @@ -14,7 +14,8 @@
> > #include <net/mptcp.h>
> > #include "protocol.h"
> >
> > -#define MIN_INFO_OPTLEN_SIZE 16
> > +#define MIN_INFO_OPTLEN_SIZE 16
> > +#define MIN_FULL_INFO_OPTLEN_SIZE 40
> >
> > static struct sock *__mptcp_tcp_fallback(struct mptcp_sock *msk)
> > {
> > @@ -977,7 +978,8 @@ static int mptcp_put_subflow_data(struct mptcp_subflow_data *sfd,
> > }
> >
> > static int mptcp_get_subflow_data(struct mptcp_subflow_data *sfd,
> > - char __user *optval, int __user *optlen)
> > + char __user *optval,
> > + int __user *optlen)
>
> Do you mind if I undo this modification when applying the patch?
> Or did you do that on purpose?
Oops, landed there unintentionally. Thanks for spotting. Please restore
the original code, thanks!
/P
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase
2023-05-25 15:22 ` [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase Matthieu Baerts
@ 2023-05-25 16:39 ` Paolo Abeni
2023-05-25 16:49 ` Matthieu Baerts
0 siblings, 1 reply; 19+ messages in thread
From: Paolo Abeni @ 2023-05-25 16:39 UTC (permalink / raw)
To: Matthieu Baerts, mptcp; +Cc: Florian Westphal
On Thu, 2023-05-25 at 17:22 +0200, Matthieu Baerts wrote:
> Hi Paolo,
>
> On 25/05/2023 09:17, Paolo Abeni wrote:
> > Add a testcase explicitly triggering the newly introduce
> > MPTCP_FULL_INFO getsockopt.
>
> (...)
>
> > +static void do_getsockopt_mptcp_full_info(struct so_state *s, int fd)
> > +{
> > + size_t data_size = sizeof(struct mptcp_full_info);
> > + struct mptcp_subflow_info sfinfo[2];
> > + struct tcp_info tcp_info[2];
> > + struct mptcp_full_info mfi;
> > + socklen_t olen;
> > + int ret;
> > +
> > + memset(&mfi, 0, data_size);
> > + memset(tcp_info, 0, sizeof(tcp_info));
> > + memset(sfinfo, 0, sizeof(sfinfo));
> > +
> > + mfi.size_tcpinfo_user = sizeof(struct tcp_info);
> > + mfi.size_sfinfo_user = sizeof(struct mptcp_subflow_info);
> > + mfi.size_arrays_user = 2;
> > + mfi.subflow_info = (unsigned long) &sfinfo[0];
> > + mfi.tcp_info = (unsigned long) &tcp_info[0];
> > + olen = data_size;
> > +
> > + ret = getsockopt(fd, SOL_MPTCP, MPTCP_FULL_INFO, &mfi, &olen);
> > + if (ret < 0) {
> > + if (errno == EOPNOTSUPP) {
> > + perror("MPTCP_FULL_INFO test skipped");
> > + return;
> > + }
> > + xerror("getsockopt MPTCP_FULL_INFO");
> > + }
> > +
> > + assert(olen <= data_size);
> > + assert(mfi.size_tcpinfo_user == mfi.size_tcpinfo_kernel);
> > + assert(mfi.size_tcpinfo_user == sizeof(struct tcp_info));
> > + assert(mfi.size_sfinfo_user == mfi.size_sfinfo_kernel);
> > + assert(mfi.size_sfinfo_user == sizeof(struct mptcp_subflow_info));
>
> Similar to the patch I sent to support old kernels, we cannot do these 4
> last assert(), see:
>
> https://patchwork.kernel.org/project/mptcp/patch/20230406-mptcp-issue-368-selftests-old-kernels-v2-17-50313e4f83ab@tessares.net/
>
> Because we cannot guess the size of these two structures in the kernel,
Regarding mptcp_subflow_info we can infer it must be >=
sizeof(mptcp_subflow_addrs) + 4...
> I suggest to do this:
>
> assert(mfi.size_tcpinfo_user > 0);
> assert(mfi.size_tcpinfo_kernel > 0);
> assert(mfi.size_sfinfo_user > 0);
> assert(mfi.size_sfinfo_kernel > 0);
>
> Better than nothing... In case of error, one would be set to 0 or we
> would not get info from these structures below.
... so we could check:
int min_sfinfo_size = 4 + sizeof(struct mptcp_subflow_addrs);
// ...
assert(mfi.size_tcpinfo_kernel > 0);
assert(mfi.size_tcpinfo_user ==
min(mfi.size_tcpinfo_kernel, sizeof(struct tcp_info));
assert(mfi.size_sfinfo_kernel >= min_sfinfo_size);
assert(mfi.size_sfinfo_user ==
min(mfi.size_sfinfo_kernel, sizeof(struct mptcp_subflow_info));
/P
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 mptcp-next 2/6] mptcp: track some aggregate data counters.
2023-05-25 15:19 ` Matthieu Baerts
@ 2023-05-25 16:42 ` Paolo Abeni
2023-05-25 17:00 ` Matthieu Baerts
0 siblings, 1 reply; 19+ messages in thread
From: Paolo Abeni @ 2023-05-25 16:42 UTC (permalink / raw)
To: Matthieu Baerts, mptcp; +Cc: Florian Westphal
On Thu, 2023-05-25 at 17:19 +0200, Matthieu Baerts wrote:
> Hi Paolo,
>
> On 25/05/2023 09:17, Paolo Abeni wrote:
> > 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 32af2d278cb4..a124be6ebbba 100644
> > --- a/include/uapi/linux/mptcp.h
> > +++ b/include/uapi/linux/mptcp.h
> > @@ -123,6 +123,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;
>
> I'm sure sure I followed correctly the discussions about that with
> Florian: did we not want to switch to __aligned_u64 for the new fields?
> (and why not the old ones as well if it doesn't change anything but at
> least it would be uniformed)
>
> If I'm not mistaken, it would not change anything but maybe better to be
> on the safe side?
IMHO this is the main point. I also think adding __aligned_u64 should
not change nothing. _but_ if will change something e.g. on some
esotheric arch, adding __aligned_u64 would break uAPI and will be very
bad.
IMHO the safe side is not adding such annotation here.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase
2023-05-25 16:39 ` Paolo Abeni
@ 2023-05-25 16:49 ` Matthieu Baerts
0 siblings, 0 replies; 19+ messages in thread
From: Matthieu Baerts @ 2023-05-25 16:49 UTC (permalink / raw)
To: Paolo Abeni, mptcp; +Cc: Florian Westphal
Hi Paolo,
On 25/05/2023 18:39, Paolo Abeni wrote:
> On Thu, 2023-05-25 at 17:22 +0200, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 25/05/2023 09:17, Paolo Abeni wrote:
>>> Add a testcase explicitly triggering the newly introduce
>>> MPTCP_FULL_INFO getsockopt.
>>
>> (...)
>>
>>> +static void do_getsockopt_mptcp_full_info(struct so_state *s, int fd)
>>> +{
>>> + size_t data_size = sizeof(struct mptcp_full_info);
>>> + struct mptcp_subflow_info sfinfo[2];
>>> + struct tcp_info tcp_info[2];
>>> + struct mptcp_full_info mfi;
>>> + socklen_t olen;
>>> + int ret;
>>> +
>>> + memset(&mfi, 0, data_size);
>>> + memset(tcp_info, 0, sizeof(tcp_info));
>>> + memset(sfinfo, 0, sizeof(sfinfo));
>>> +
>>> + mfi.size_tcpinfo_user = sizeof(struct tcp_info);
>>> + mfi.size_sfinfo_user = sizeof(struct mptcp_subflow_info);
>>> + mfi.size_arrays_user = 2;
>>> + mfi.subflow_info = (unsigned long) &sfinfo[0];
>>> + mfi.tcp_info = (unsigned long) &tcp_info[0];
>>> + olen = data_size;
>>> +
>>> + ret = getsockopt(fd, SOL_MPTCP, MPTCP_FULL_INFO, &mfi, &olen);
>>> + if (ret < 0) {
>>> + if (errno == EOPNOTSUPP) {
>>> + perror("MPTCP_FULL_INFO test skipped");
>>> + return;
>>> + }
>>> + xerror("getsockopt MPTCP_FULL_INFO");
>>> + }
>>> +
>>> + assert(olen <= data_size);
>>> + assert(mfi.size_tcpinfo_user == mfi.size_tcpinfo_kernel);
>>> + assert(mfi.size_tcpinfo_user == sizeof(struct tcp_info));
>>> + assert(mfi.size_sfinfo_user == mfi.size_sfinfo_kernel);
>>> + assert(mfi.size_sfinfo_user == sizeof(struct mptcp_subflow_info));
>>
>> Similar to the patch I sent to support old kernels, we cannot do these 4
>> last assert(), see:
>>
>> https://patchwork.kernel.org/project/mptcp/patch/20230406-mptcp-issue-368-selftests-old-kernels-v2-17-50313e4f83ab@tessares.net/
>>
>> Because we cannot guess the size of these two structures in the kernel,
>
> Regarding mptcp_subflow_info we can infer it must be >=
> sizeof(mptcp_subflow_addrs) + 4...
Does that mean sizeof(mptcp_subflow_addrs) can never be modified?
>> I suggest to do this:
>>
>> assert(mfi.size_tcpinfo_user > 0);
>> assert(mfi.size_tcpinfo_kernel > 0);
>> assert(mfi.size_sfinfo_user > 0);
>> assert(mfi.size_sfinfo_kernel > 0);
>>
>> Better than nothing... In case of error, one would be set to 0 or we
>> would not get info from these structures below.
>
> ... so we could check:
>
> int min_sfinfo_size = 4 + sizeof(struct mptcp_subflow_addrs);
>
> // ...
>
> assert(mfi.size_tcpinfo_kernel > 0);
> assert(mfi.size_tcpinfo_user ==
> min(mfi.size_tcpinfo_kernel, sizeof(struct tcp_info));
Good idea to use 'min' here!
> assert(mfi.size_sfinfo_kernel >= min_sfinfo_size);
> assert(mfi.size_sfinfo_user ==
> min(mfi.size_sfinfo_kernel, sizeof(struct mptcp_subflow_info));
I guess I should do the same in:
https://patchwork.kernel.org/project/mptcp/patch/20230406-mptcp-issue-368-selftests-old-kernels-v2-17-50313e4f83ab@tessares.net/
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: selftests: mptcp: add MPTCP_FULL_INFO testcase: Tests Results
2023-05-25 7:17 ` [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase Paolo Abeni
2023-05-25 8:35 ` selftests: mptcp: add MPTCP_FULL_INFO testcase: Tests Results MPTCP CI
2023-05-25 15:22 ` [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase Matthieu Baerts
@ 2023-05-25 16:51 ` MPTCP CI
2 siblings, 0 replies; 19+ messages in thread
From: MPTCP CI @ 2023-05-25 16:51 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/4764400553295872
- Summary: https://api.cirrus-ci.com/v1/artifact/task/4764400553295872/summary/summary.txt
- KVM Validation: normal (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5890300460138496
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5890300460138496/summary/summary.txt
- KVM Validation: debug (except selftest_mptcp_join):
- Unstable: 2 failed test(s): packetdrill_fastopen selftest_diag 🔴:
- Task: https://cirrus-ci.com/task/5327350506717184
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5327350506717184/summary/summary.txt
- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/6453250413559808
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6453250413559808/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/de1f158647f1
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-debug
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 mptcp-next 2/6] mptcp: track some aggregate data counters.
2023-05-25 16:42 ` Paolo Abeni
@ 2023-05-25 17:00 ` Matthieu Baerts
0 siblings, 0 replies; 19+ messages in thread
From: Matthieu Baerts @ 2023-05-25 17:00 UTC (permalink / raw)
To: Paolo Abeni, mptcp; +Cc: Florian Westphal
On 25/05/2023 18:42, Paolo Abeni wrote:
> On Thu, 2023-05-25 at 17:19 +0200, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 25/05/2023 09:17, Paolo Abeni wrote:
>>> 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 32af2d278cb4..a124be6ebbba 100644
>>> --- a/include/uapi/linux/mptcp.h
>>> +++ b/include/uapi/linux/mptcp.h
>>> @@ -123,6 +123,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;
>>
>> I'm sure sure I followed correctly the discussions about that with
>> Florian: did we not want to switch to __aligned_u64 for the new fields?
>> (and why not the old ones as well if it doesn't change anything but at
>> least it would be uniformed)
>>
>> If I'm not mistaken, it would not change anything but maybe better to be
>> on the safe side?
>
> IMHO this is the main point. I also think adding __aligned_u64 should
> not change nothing. _but_ if will change something e.g. on some
> esotheric arch, adding __aligned_u64 would break uAPI and will be very
> bad.
>
> IMHO the safe side is not adding such annotation here.
Thank you for your reply! Sounds good to me!
And despite having 64-bit fields, "struct tcp_info" doesn't use
__aligned_u64 either. So we can always say we are "aligned" to what TCP
did :)
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info
2023-05-25 15:18 ` [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Matthieu Baerts
@ 2023-05-28 16:25 ` Matthieu Baerts
0 siblings, 0 replies; 19+ messages in thread
From: Matthieu Baerts @ 2023-05-28 16:25 UTC (permalink / raw)
To: Paolo Abeni, mptcp; +Cc: Florian Westphal
Hi Paolo,
On 25/05/2023 17:18, Matthieu Baerts wrote:
> Hi Paolo,
>
> On 25/05/2023 09:17, Paolo Abeni wrote:
>> Sorry for the high-freq spamming. I really want to close this topic
>> and move forward.
>>
>> 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".
>>
>> v5 -> v6:
>> - fix another compiler warning
>>
>> v4 -> v5:
>> - changed again binary layout for MPTCP_FULL_INFO structs (Matttbe)
>> - fixed 32bit build issue
>> - reordered the patches, less controversial first
>
> Thank you for the new version! It looks good to me!
>
> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>
> I'm sorry I was not confident about the previous version and I was not
> feeling good asking you to change it!
> I do like the last version, I hope you too :)
>
> I have a few questions in the individual patches if you don't mind but
> that should not be blocking the series. I can even do the modifications
> when applying the patches if that's OK for you!
Thank you for the different answers, for the instructions on how to fix
the conflicts and again sorry for the previous reviews!
I added links to these tickets in the different patches:
https://github.com/multipath-tcp/mptcp_net-next/issues/385
https://github.com/multipath-tcp/mptcp_net-next/issues/388
Now in our tree (feat. for net-next) with my RvB tag:
New patches for t/upstream:
- c7cb50a32f60: mptcp: move snd_una update earlier for fallback socket
- 34d08f8b2ac9: mptcp: track some aggregate data counters
- f8d85c73f57a: selftests: mptcp: explicitly tests aggregate counters
- 7b8be8bf5ef4: mptcp: add subflow unique id
- 3f1aaf669e62: mptcp: introduce MPTCP_FULL_INFO getsockopt
- 28cb4413f233: selftests: mptcp: add MPTCP_FULL_INFO testcase
- 91ea9989b43d: conflict in t/mptcp-use-get_retrans-wrapper
- Results: 723979747226..e5a080352635 (export)
Tests are now in progress:
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230528T161955
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-05-28 16:25 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-25 7:17 [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 1/6] mptcp: move snd_una update earlier for fallback socket Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 2/6] mptcp: track some aggregate data counters Paolo Abeni
2023-05-25 15:19 ` Matthieu Baerts
2023-05-25 16:42 ` Paolo Abeni
2023-05-25 17:00 ` Matthieu Baerts
2023-05-25 7:17 ` [PATCH v6 mptcp-next 3/6] selftests: mptcp: explicitly tests aggregate counters Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 4/6] mptcp: add subflow unique id Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 5/6] mptcp: introduce MPTCP_FULL_INFO getsockopt Paolo Abeni
2023-05-25 15:20 ` Matthieu Baerts
2023-05-25 16:29 ` Paolo Abeni
2023-05-25 7:17 ` [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase Paolo Abeni
2023-05-25 8:35 ` selftests: mptcp: add MPTCP_FULL_INFO testcase: Tests Results MPTCP CI
2023-05-25 15:22 ` [PATCH v6 mptcp-next 6/6] selftests: mptcp: add MPTCP_FULL_INFO testcase Matthieu Baerts
2023-05-25 16:39 ` Paolo Abeni
2023-05-25 16:49 ` Matthieu Baerts
2023-05-25 16:51 ` selftests: mptcp: add MPTCP_FULL_INFO testcase: Tests Results MPTCP CI
2023-05-25 15:18 ` [PATCH v6 mptcp-next 0/6] mptcp: add some more diag info Matthieu Baerts
2023-05-28 16:25 ` Matthieu Baerts
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.