These patches from Geliang add support for the "last time" field in MPTCP Info, and verify that the counters look valid.
Patch 1 adds these counters: last_data_sent, last_data_recv and last_ack_recv. They are available in the MPTCP Info, so exposed via getsockopt(MPTCP_INFO) and the Netlink Diag interface.
Patch 2 adds a test in diag.sh MPTCP selftest, to check that the counters have moved by at least 250ms, after having waited twice that time.
Signed-off-by: Matthieu Baerts (NGI0) matttbe@kernel.org --- Changes in v2: - Only patch 1/2 has been modified following Eric's suggestion, see the individual changelog for more details. - Link to v1: https://lore.kernel.org/r/20240405-upstream-net-next-20240405-mptcp-last-tim...
--- Geliang Tang (2): mptcp: add last time fields in mptcp_info selftests: mptcp: test last time mptcp_info
include/uapi/linux/mptcp.h | 4 +++ net/mptcp/options.c | 1 + net/mptcp/protocol.c | 7 ++++ net/mptcp/protocol.h | 3 ++ net/mptcp/sockopt.c | 16 +++++++--- tools/testing/selftests/net/mptcp/diag.sh | 53 +++++++++++++++++++++++++++++++ 6 files changed, 79 insertions(+), 5 deletions(-) --- base-commit: 2ecd487b670fcbb1ad4893fff1af4aafdecb6023 change-id: 20240405-upstream-net-next-20240405-mptcp-last-time-info-9b03618e08f1
Best regards,
From: Geliang Tang tanggeliang@kylinos.cn
This patch adds "last time" fields last_data_sent, last_data_recv and last_ack_recv in struct mptcp_sock to record the last time data_sent, data_recv and ack_recv happened. They all are initialized as tcp_jiffies32 in __mptcp_init_sock(), and updated as tcp_jiffies32 too when data is sent in __subflow_push_pending(), data is received in __mptcp_move_skbs_from_subflow(), and ack is received in ack_update_msk().
Similar to tcpi_last_data_sent, tcpi_last_data_recv and tcpi_last_ack_recv exposed with TCP, this patch exposes the last time "an action happened" for MPTCP in mptcp_info, named mptcpi_last_data_sent, mptcpi_last_data_recv and mptcpi_last_ack_recv, calculated in mptcp_diag_fill_info() as the time deltas between now and the newly added last time fields in mptcp_sock.
Since msk->last_ack_recv needs to be protected by mptcp_data_lock/unlock, and lock_sock_fast can sleep and be quite slow, move the entire mptcp_data_lock/unlock block after the lock/unlock_sock_fast block. Then mptcpi_last_data_sent and mptcpi_last_data_recv are set in lock/unlock_sock_fast block, while mptcpi_last_ack_recv is set in mptcp_data_lock/unlock block, which is protected by a spinlock and should not block for too long.
Also add three reserved bytes in struct mptcp_info not to have holes in this structure exposed to userspace.
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/446 Signed-off-by: Geliang Tang tanggeliang@kylinos.cn Reviewed-by: Mat Martineau martineau@kernel.org Reviewed-by: Matthieu Baerts (NGI0) matttbe@kernel.org Signed-off-by: Matthieu Baerts (NGI0) matttbe@kernel.org --- v2: - Set 'now = jiffies32' inside '(un)lock_sock_fast' block (Eric). - Move 'mptcp_data_(un)lock' block after '(un)lock_sock_fast', not to reload 'now', and have all counters synced (Matthieu / Mat). --- include/uapi/linux/mptcp.h | 4 ++++ net/mptcp/options.c | 1 + net/mptcp/protocol.c | 7 +++++++ net/mptcp/protocol.h | 3 +++ net/mptcp/sockopt.c | 16 +++++++++++----- 5 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h index 74cfe496891e..67d015df8893 100644 --- a/include/uapi/linux/mptcp.h +++ b/include/uapi/linux/mptcp.h @@ -58,6 +58,10 @@ struct mptcp_info { __u64 mptcpi_bytes_received; __u64 mptcpi_bytes_acked; __u8 mptcpi_subflows_total; + __u8 reserved[3]; + __u32 mptcpi_last_data_sent; + __u32 mptcpi_last_data_recv; + __u32 mptcpi_last_ack_recv; };
/* MPTCP Reset reason codes, rfc8684 */ diff --git a/net/mptcp/options.c b/net/mptcp/options.c index 27ca42c77b02..8e8dcfbc2993 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -1068,6 +1068,7 @@ static void ack_update_msk(struct mptcp_sock *msk, __mptcp_snd_una_update(msk, new_snd_una); __mptcp_data_acked(sk); } + msk->last_ack_recv = tcp_jiffies32; mptcp_data_unlock(sk);
trace_ack_update_msk(mp_opt->data_ack, diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 995b53cd021c..f8bc34f0d973 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -706,6 +706,8 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk, } } while (more_data_avail);
+ if (moved > 0) + msk->last_data_recv = tcp_jiffies32; *bytes += moved; return done; } @@ -1556,6 +1558,8 @@ static int __subflow_push_pending(struct sock *sk, struct sock *ssk, err = copied;
out: + if (err > 0) + msk->last_data_sent = tcp_jiffies32; return err; }
@@ -2793,6 +2797,9 @@ static void __mptcp_init_sock(struct sock *sk) WRITE_ONCE(msk->allow_infinite_fallback, true); msk->recovery = false; msk->subflow_id = 1; + msk->last_data_sent = tcp_jiffies32; + msk->last_data_recv = tcp_jiffies32; + msk->last_ack_recv = tcp_jiffies32;
mptcp_pm_data_init(msk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 46f4655b7123..fdfa843e2d88 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -282,6 +282,9 @@ struct mptcp_sock { u64 bytes_acked; u64 snd_una; u64 wnd_end; + u32 last_data_sent; + u32 last_data_recv; + u32 last_ack_recv; unsigned long timer_ival; u32 token; int rmem_released; diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index 9d5d42a77bcc..1fea43f5b6f3 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -898,6 +898,7 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info) struct sock *sk = (struct sock *)msk; u32 flags = 0; bool slow; + u32 now;
memset(info, 0, sizeof(*info));
@@ -926,11 +927,6 @@ 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; - 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 = READ_ONCE(msk->csum_enabled); @@ -942,7 +938,17 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info) info->mptcpi_bytes_retrans = msk->bytes_retrans; info->mptcpi_subflows_total = info->mptcpi_subflows + __mptcp_has_initial_subflow(msk); + now = tcp_jiffies32; + info->mptcpi_last_data_sent = jiffies_to_msecs(now - msk->last_data_sent); + info->mptcpi_last_data_recv = jiffies_to_msecs(now - msk->last_data_recv); unlock_sock_fast(sk, slow); + + mptcp_data_lock(sk); + info->mptcpi_last_ack_recv = jiffies_to_msecs(now - msk->last_ack_recv); + 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); } EXPORT_SYMBOL_GPL(mptcp_diag_fill_info);
From: Geliang Tang tanggeliang@kylinos.cn
This patch adds a new helper chk_msk_info() to show the counters in mptcp_info of the given info, and check that the timestamps move forward. Use it to show newly added last_data_sent, last_data_recv and last_ack_recv in mptcp_info in chk_last_time_info().
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn Reviewed-by: Matthieu Baerts (NGI0) matttbe@kernel.org Reviewed-by: Mat Martineau martineau@kernel.org Signed-off-by: Matthieu Baerts (NGI0) matttbe@kernel.org --- tools/testing/selftests/net/mptcp/diag.sh | 53 +++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh index bc97ab33a00e..776d43a6922d 100755 --- a/tools/testing/selftests/net/mptcp/diag.sh +++ b/tools/testing/selftests/net/mptcp/diag.sh @@ -200,6 +200,58 @@ chk_msk_cestab() "${expected}" "${msg}" "" }
+msk_info_get_value() +{ + local port="${1}" + local info="${2}" + + ss -N "${ns}" -inHM dport "${port}" | \ + mptcp_lib_get_info_value "${info}" "${info}" +} + +chk_msk_info() +{ + local port="${1}" + local info="${2}" + local cnt="${3}" + local msg="....chk ${info}" + local delta_ms=250 # half what we waited before, just to be sure + local now + + now=$(msk_info_get_value "${port}" "${info}") + + mptcp_lib_print_title "${msg}" + if { [ -z "${cnt}" ] || [ -z "${now}" ]; } && + ! mptcp_lib_expect_all_features; then + mptcp_lib_pr_skip "Feature probably not supported" + mptcp_lib_result_skip "${msg}" + elif [ "$((cnt + delta_ms))" -lt "${now}" ]; then + mptcp_lib_pr_ok + mptcp_lib_result_pass "${msg}" + else + mptcp_lib_pr_fail "value of ${info} changed by $((now - cnt))ms," \ + "expected at least ${delta_ms}ms" + mptcp_lib_result_fail "${msg}" + ret=${KSFT_FAIL} + fi +} + +chk_last_time_info() +{ + local port="${1}" + local data_sent data_recv ack_recv + + data_sent=$(msk_info_get_value "${port}" "last_data_sent") + data_recv=$(msk_info_get_value "${port}" "last_data_recv") + ack_recv=$(msk_info_get_value "${port}" "last_ack_recv") + + sleep 0.5 # wait to check after if the timestamps difference + + chk_msk_info "${port}" "last_data_sent" "${data_sent}" + chk_msk_info "${port}" "last_data_recv" "${data_recv}" + chk_msk_info "${port}" "last_ack_recv" "${ack_recv}" +} + wait_connected() { local listener_ns="${1}" @@ -233,6 +285,7 @@ echo "b" | \ 127.0.0.1 >/dev/null & wait_connected $ns 10000 chk_msk_nr 2 "after MPC handshake " +chk_last_time_info 10000 chk_msk_remote_key_nr 2 "....chk remote_key" chk_msk_fallback_nr 0 "....chk no fallback" chk_msk_inuse 2
On Wed, 10 Apr 2024 11:48:23 +0200 Matthieu Baerts (NGI0) wrote:
These patches from Geliang add support for the "last time" field in MPTCP Info, and verify that the counters look valid.
Patch 1 adds these counters: last_data_sent, last_data_recv and last_ack_recv. They are available in the MPTCP Info, so exposed via getsockopt(MPTCP_INFO) and the Netlink Diag interface.
Patch 2 adds a test in diag.sh MPTCP selftest, to check that the counters have moved by at least 250ms, after having waited twice that time.
Hi Mat, is this causing skips in selftests by any chance?
# 07 ....chk last_data_sent [SKIP] Feature probably not supported # 08 ....chk last_data_recv [SKIP] Feature probably not supported # 09 ....chk last_ack_recv [SKIP] Feature probably not supported
I'll "hide it" from patchwork for now..
Hi Jakub,
Thank you for your email!
10 Apr 2024 20:34:54 Jakub Kicinski kuba@kernel.org:
On Wed, 10 Apr 2024 11:48:23 +0200 Matthieu Baerts (NGI0) wrote:
These patches from Geliang add support for the "last time" field in MPTCP Info, and verify that the counters look valid.
Patch 1 adds these counters: last_data_sent, last_data_recv and last_ack_recv. They are available in the MPTCP Info, so exposed via getsockopt(MPTCP_INFO) and the Netlink Diag interface.
Patch 2 adds a test in diag.sh MPTCP selftest, to check that the counters have moved by at least 250ms, after having waited twice that time.
Hi Mat, is this causing skips in selftests by any chance?
# 07 ....chk last_data_sent [SKIP] Feature probably not supported # 08 ....chk last_data_recv [SKIP] Feature probably not supported # 09 ....chk last_ack_recv [SKIP] Feature probably not supported
Yes it does, I should have added a note about that, sorry: that's because SS needs to be patched as well to display the new counters.
https://patchwork.kernel.org/project/mptcp/patch/fd9e850f1e00691204f1dfebc63...
This patch will be sent when the kernel one will be accepted.
Is it an issue? The modification of the selftests can be applied later if you prefer.
Earlier today, I was looking at changing NIPA not to mark the whole selftest as "SKIP" but I saw it was not a bug: a check is there to mark everything as skipped if one subtest is marked as skipped from what I understood. So I guess we don't want to change that :)
I'll "hide it" from patchwork for now..
pw-bot: defer
Thank you! Do you prefer if I resend only patch 1/2 for now?
Cheers, Matt
On Wed, 10 Apr 2024 21:31:13 +0200 (GMT+02:00) Matthieu Baerts wrote:
Hi Mat, is this causing skips in selftests by any chance?
# 07 ....chk last_data_sent [SKIP] Feature probably not supported # 08 ....chk last_data_recv [SKIP] Feature probably not supported # 09 ....chk last_ack_recv [SKIP] Feature probably not supported
Yes it does, I should have added a note about that, sorry: that's because SS needs to be patched as well to display the new counters.
https://patchwork.kernel.org/project/mptcp/patch/fd9e850f1e00691204f1dfebc63...
This patch will be sent when the kernel one will be accepted.
I see, applied locally now, thanks!
Is it an issue? The modification of the selftests can be applied later if you prefer.
Not sure. If it doesn't happen super often maybe co-post the iproute2 patch as described: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#co-posti... and I'll apply it on the worker machines manually.
Earlier today, I was looking at changing NIPA not to mark the whole selftest as "SKIP" but I saw it was not a bug: a check is there to mark everything as skipped if one subtest is marked as skipped from what I understood. So I guess we don't want to change that :)
Correct :) It's working as intended :)
I'll "hide it" from patchwork for now..
pw-bot: defer
Thank you! Do you prefer if I resend only patch 1/2 for now?
No need, restored the patches back, let's see if next run is clean.
Hi Jakub,
On 10/04/2024 23:01, Jakub Kicinski wrote:
On Wed, 10 Apr 2024 21:31:13 +0200 (GMT+02:00) Matthieu Baerts wrote:
Hi Mat, is this causing skips in selftests by any chance?
# 07 ....chk last_data_sent [SKIP] Feature probably not supported # 08 ....chk last_data_recv [SKIP] Feature probably not supported # 09 ....chk last_ack_recv [SKIP] Feature probably not supported
Yes it does, I should have added a note about that, sorry: that's because SS needs to be patched as well to display the new counters.
https://patchwork.kernel.org/project/mptcp/patch/fd9e850f1e00691204f1dfebc63...
This patch will be sent when the kernel one will be accepted.
I see, applied locally now, thanks!
Thank you!
Is it an issue? The modification of the selftests can be applied later if you prefer.
Not sure. If it doesn't happen super often maybe co-post the iproute2 patch as described: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#co-posti... and I'll apply it on the worker machines manually.
I missed that. Indeed, that should be rare. We will do that next time!
Earlier today, I was looking at changing NIPA not to mark the whole selftest as "SKIP" but I saw it was not a bug: a check is there to mark everything as skipped if one subtest is marked as skipped from what I understood. So I guess we don't want to change that :)
Correct :) It's working as intended :)
It can maybe be modified when we can re-use the option to parse embedded TAP results :)
I'll "hide it" from patchwork for now..
pw-bot: defer
Thank you! Do you prefer if I resend only patch 1/2 for now?
No need, restored the patches back, let's see if next run is clean.
Thank you! It looks like they are OK now!
Cheers, Matt
Hello:
This series was applied to netdev/net-next.git (main) by Jakub Kicinski kuba@kernel.org:
On Wed, 10 Apr 2024 11:48:23 +0200 you wrote:
These patches from Geliang add support for the "last time" field in MPTCP Info, and verify that the counters look valid.
Patch 1 adds these counters: last_data_sent, last_data_recv and last_ack_recv. They are available in the MPTCP Info, so exposed via getsockopt(MPTCP_INFO) and the Netlink Diag interface.
[...]
Here is the summary with links: - [net-next,v2,1/2] mptcp: add last time fields in mptcp_info https://git.kernel.org/netdev/net-next/c/18d82cde7432 - [net-next,v2,2/2] selftests: mptcp: test last time mptcp_info https://git.kernel.org/netdev/net-next/c/22724c89892f
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org