Recently, a few issues have been discovered around the creation of
additional subflows. Without these counters, it was difficult to point
out the reason why some subflows were not created as expected.
These counters should have been added earlier, because there is no other
simple ways to extract such information from the kernel, and understand
why subflows have not been created.
While at it, some pr_debug() have been added, just in case the errno
needs to be printed.
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/509
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
net/mptcp/mib.c | 5 +++++
net/mptcp/mib.h | 5 +++++
net/mptcp/subflow.c | 24 ++++++++++++++++++++----
3 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index 7884217f33eb..a0b1152c32f1 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -25,6 +25,11 @@ static const struct snmp_mib mptcp_snmp_list[] = {
SNMP_MIB_ITEM("MPJoinSynAckHMacFailure", MPTCP_MIB_JOINSYNACKMAC),
SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
+ SNMP_MIB_ITEM("MPJoinSynTx", MPTCP_MIB_JOINSYNTX),
+ SNMP_MIB_ITEM("MPJoinSynTxFEstabErr", MPTCP_MIB_JOINSYNTXFULLYESTAB),
+ SNMP_MIB_ITEM("MPJoinSynTxCreatSkErr", MPTCP_MIB_JOINSYNTXCREATESOCK),
+ SNMP_MIB_ITEM("MPJoinSynTxBindErr", MPTCP_MIB_JOINSYNTXBIND),
+ SNMP_MIB_ITEM("MPJoinSynTxConnectErr", MPTCP_MIB_JOINSYNTXCONNECT),
SNMP_MIB_ITEM("DSSNotMatching", MPTCP_MIB_DSSNOMATCH),
SNMP_MIB_ITEM("InfiniteMapTx", MPTCP_MIB_INFINITEMAPTX),
SNMP_MIB_ITEM("InfiniteMapRx", MPTCP_MIB_INFINITEMAPRX),
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 66aa67f49d03..ccb3a30e835a 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -20,6 +20,11 @@ enum linux_mptcp_mib_field {
MPTCP_MIB_JOINSYNACKMAC, /* HMAC was wrong on SYN/ACK + MP_JOIN */
MPTCP_MIB_JOINACKRX, /* Received an ACK + MP_JOIN */
MPTCP_MIB_JOINACKMAC, /* HMAC was wrong on ACK + MP_JOIN */
+ MPTCP_MIB_JOINSYNTX, /* Sending a SYN + MP_JOIN */
+ MPTCP_MIB_JOINSYNTXFULLYESTAB, /* Not in fully established when sending a SYN + MP_JOIN */
+ MPTCP_MIB_JOINSYNTXCREATESOCK, /* Not able to create a socket when sending a SYN + MP_JOIN */
+ MPTCP_MIB_JOINSYNTXBIND, /* Not able to bind() the address when sending a SYN + MP_JOIN */
+ MPTCP_MIB_JOINSYNTXCONNECT, /* Not able to connect() when sending a SYN + MP_JOIN */
MPTCP_MIB_DSSNOMATCH, /* Received a new mapping that did not match the previous one */
MPTCP_MIB_INFINITEMAPTX, /* Sent an infinite mapping */
MPTCP_MIB_INFINITEMAPRX, /* Received an infinite mapping */
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index a21c712350c3..d305c0f53a09 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1577,12 +1577,18 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
int ifindex;
u8 flags;
- if (!mptcp_is_fully_established(sk))
+ if (!mptcp_is_fully_established(sk)) {
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXFULLYESTAB);
goto err_out;
+ }
err = mptcp_subflow_create_socket(sk, loc->family, &sf);
- if (err)
+ if (err) {
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXCREATESOCK);
+ pr_debug("msk=%p local=%d remote:%d create sock error: %d\n",
+ msk, local_id, remote_id, err);
goto err_out;
+ }
ssk = sf->sk;
subflow = mptcp_subflow_ctx(ssk);
@@ -1608,8 +1614,12 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
#endif
ssk->sk_bound_dev_if = ifindex;
err = kernel_bind(sf, (struct sockaddr *)&addr, addrlen);
- if (err)
+ if (err) {
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXBIND);
+ pr_debug("msk=%p local=%d remote:%d bind error: %d\n",
+ msk, local_id, remote_id, err);
goto failed;
+ }
mptcp_crypto_key_sha(subflow->remote_key, &remote_token, NULL);
pr_debug("msk=%p remote_token=%u local_id=%d remote_id=%d", msk,
@@ -1624,8 +1634,14 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
sock_hold(ssk);
list_add_tail(&subflow->node, &msk->conn_list);
err = kernel_connect(sf, (struct sockaddr *)&addr, addrlen, O_NONBLOCK);
- if (err && err != -EINPROGRESS)
+ if (err && err != -EINPROGRESS) {
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXCONNECT);
+ pr_debug("msk=%p local=%d remote:%d connect error: %d\n",
+ msk, local_id, remote_id, err);
goto failed_unlink;
+ }
+
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTX);
/* discard the subflow socket */
mptcp_sock_graft(ssk, sk->sk_socket);
--
2.45.2Hi Matt,
On Fri, 2024-07-26 at 19:48 +0200, Matthieu Baerts (NGI0) wrote:
> Recently, a few issues have been discovered around the creation of
> additional subflows. Without these counters, it was difficult to
> point
> out the reason why some subflows were not created as expected.
>
> These counters should have been added earlier, because there is no
> other
> simple ways to extract such information from the kernel, and
> understand
> why subflows have not been created.
>
> While at it, some pr_debug() have been added, just in case the errno
> needs to be printed.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/509
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> net/mptcp/mib.c | 5 +++++
> net/mptcp/mib.h | 5 +++++
> net/mptcp/subflow.c | 24 ++++++++++++++++++++----
> 3 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
> index 7884217f33eb..a0b1152c32f1 100644
> --- a/net/mptcp/mib.c
> +++ b/net/mptcp/mib.c
> @@ -25,6 +25,11 @@ static const struct snmp_mib mptcp_snmp_list[] = {
> SNMP_MIB_ITEM("MPJoinSynAckHMacFailure",
> MPTCP_MIB_JOINSYNACKMAC),
> SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
> SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
> + SNMP_MIB_ITEM("MPJoinSynTx", MPTCP_MIB_JOINSYNTX),
> + SNMP_MIB_ITEM("MPJoinSynTxFEstabErr",
> MPTCP_MIB_JOINSYNTXFULLYESTAB),
It's better to keep the "Err" postfix to reduce ambiguity:
MPTCP_MIB_JOINSYNTXFULLYESTAB_ERR or MPTCP_MIB_JOINSYNTXFULLYESTABERR.
> + SNMP_MIB_ITEM("MPJoinSynTxCreatSkErr",
> MPTCP_MIB_JOINSYNTXCREATESOCK),
> + SNMP_MIB_ITEM("MPJoinSynTxBindErr",
> MPTCP_MIB_JOINSYNTXBIND),
> + SNMP_MIB_ITEM("MPJoinSynTxConnectErr",
> MPTCP_MIB_JOINSYNTXCONNECT),
Same these three.
Thanks,
-Geliang
> SNMP_MIB_ITEM("DSSNotMatching", MPTCP_MIB_DSSNOMATCH),
> SNMP_MIB_ITEM("InfiniteMapTx", MPTCP_MIB_INFINITEMAPTX),
> SNMP_MIB_ITEM("InfiniteMapRx", MPTCP_MIB_INFINITEMAPRX),
> diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
> index 66aa67f49d03..ccb3a30e835a 100644
> --- a/net/mptcp/mib.h
> +++ b/net/mptcp/mib.h
> @@ -20,6 +20,11 @@ enum linux_mptcp_mib_field {
> MPTCP_MIB_JOINSYNACKMAC, /* HMAC was wrong on SYN/ACK
> + MP_JOIN */
> MPTCP_MIB_JOINACKRX, /* Received an ACK + MP_JOIN
> */
> MPTCP_MIB_JOINACKMAC, /* HMAC was wrong on ACK +
> MP_JOIN */
> + MPTCP_MIB_JOINSYNTX, /* Sending a SYN + MP_JOIN
> */
> + MPTCP_MIB_JOINSYNTXFULLYESTAB, /* Not in fully established
> when sending a SYN + MP_JOIN */
> + MPTCP_MIB_JOINSYNTXCREATESOCK, /* Not able to create a
> socket when sending a SYN + MP_JOIN */
> + MPTCP_MIB_JOINSYNTXBIND, /* Not able to bind() the
> address when sending a SYN + MP_JOIN */
> + MPTCP_MIB_JOINSYNTXCONNECT, /* Not able to connect()
> when sending a SYN + MP_JOIN */
> MPTCP_MIB_DSSNOMATCH, /* Received a new mapping
> that did not match the previous one */
> MPTCP_MIB_INFINITEMAPTX, /* Sent an infinite mapping
> */
> MPTCP_MIB_INFINITEMAPRX, /* Received an infinite
> mapping */
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index a21c712350c3..d305c0f53a09 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1577,12 +1577,18 @@ int __mptcp_subflow_connect(struct sock *sk,
> const struct mptcp_addr_info *loc,
> int ifindex;
> u8 flags;
>
> - if (!mptcp_is_fully_established(sk))
> + if (!mptcp_is_fully_established(sk)) {
> + MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNTXFULLYESTAB);
> goto err_out;
> + }
>
> err = mptcp_subflow_create_socket(sk, loc->family, &sf);
> - if (err)
> + if (err) {
> + MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNTXCREATESOCK);
> + pr_debug("msk=%p local=%d remote:%d create sock
> error: %d\n",
> + msk, local_id, remote_id, err);
> goto err_out;
> + }
>
> ssk = sf->sk;
> subflow = mptcp_subflow_ctx(ssk);
> @@ -1608,8 +1614,12 @@ int __mptcp_subflow_connect(struct sock *sk,
> const struct mptcp_addr_info *loc,
> #endif
> ssk->sk_bound_dev_if = ifindex;
> err = kernel_bind(sf, (struct sockaddr *)&addr, addrlen);
> - if (err)
> + if (err) {
> + MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNTXBIND);
> + pr_debug("msk=%p local=%d remote:%d bind error:
> %d\n",
> + msk, local_id, remote_id, err);
> goto failed;
> + }
>
> mptcp_crypto_key_sha(subflow->remote_key, &remote_token,
> NULL);
> pr_debug("msk=%p remote_token=%u local_id=%d remote_id=%d",
> msk,
> @@ -1624,8 +1634,14 @@ int __mptcp_subflow_connect(struct sock *sk,
> const struct mptcp_addr_info *loc,
> sock_hold(ssk);
> list_add_tail(&subflow->node, &msk->conn_list);
> err = kernel_connect(sf, (struct sockaddr *)&addr, addrlen,
> O_NONBLOCK);
> - if (err && err != -EINPROGRESS)
> + if (err && err != -EINPROGRESS) {
> + MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNTXCONNECT);
> + pr_debug("msk=%p local=%d remote:%d connect error:
> %d\n",
> + msk, local_id, remote_id, err);
> goto failed_unlink;
> + }
> +
> + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTX);
>
> /* discard the subflow socket */
> mptcp_sock_graft(ssk, sk->sk_socket);
>
Hi Geliang,
On 29/07/2024 05:09, Geliang Tang wrote:
> Hi Matt,
>
> On Fri, 2024-07-26 at 19:48 +0200, Matthieu Baerts (NGI0) wrote:
>> Recently, a few issues have been discovered around the creation of
>> additional subflows. Without these counters, it was difficult to
>> point
>> out the reason why some subflows were not created as expected.
>>
>> These counters should have been added earlier, because there is no
>> other
>> simple ways to extract such information from the kernel, and
>> understand
>> why subflows have not been created.
>>
>> While at it, some pr_debug() have been added, just in case the errno
>> needs to be printed.
>>
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/509
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> net/mptcp/mib.c | 5 +++++
>> net/mptcp/mib.h | 5 +++++
>> net/mptcp/subflow.c | 24 ++++++++++++++++++++----
>> 3 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
>> index 7884217f33eb..a0b1152c32f1 100644
>> --- a/net/mptcp/mib.c
>> +++ b/net/mptcp/mib.c
>> @@ -25,6 +25,11 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>> SNMP_MIB_ITEM("MPJoinSynAckHMacFailure",
>> MPTCP_MIB_JOINSYNACKMAC),
>> SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
>> SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
>> + SNMP_MIB_ITEM("MPJoinSynTx", MPTCP_MIB_JOINSYNTX),
>> + SNMP_MIB_ITEM("MPJoinSynTxFEstabErr",
>> MPTCP_MIB_JOINSYNTXFULLYESTAB),
>
> It's better to keep the "Err" postfix to reduce ambiguity:
>
> MPTCP_MIB_JOINSYNTXFULLYESTAB_ERR or MPTCP_MIB_JOINSYNTXFULLYESTABERR.
I agree. I didn't do that because the names were already long enough,
they are only used in one place, and we didn't do that for the "HMAC
Failure" ones.
Or maybe I reuse a similar name to the ones exposed to the userspace?
MPTCP_MIB_JOINSYNTXFESTABERR
MPTCP_MIB_JOINSYNTXCREATSKERR
MPTCP_MIB_JOINSYNTXBINDERR
MPTCP_MIB_JOINSYNTXCONNECTERR
I can send a v2 with that.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
© 2016 - 2024 Red Hat, Inc.