1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v3 0/8] mptcp: MIB counters for sent MP_JOIN
  4. View patch

[PATCH mptcp-next v3 2/8] mptcp: MIB counters for sent MP_JOIN

Matthieu Baerts (NGI0) posted 8 patches 4 months ago
There is a newer version of this series
[PATCH mptcp-next v3 2/8] mptcp: MIB counters for sent MP_JOIN
Posted by Matthieu Baerts (NGI0) 4 months ago 
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>
---
Notes:
  - v2:
    - Add "ERR" suffix in variable names. (Geliang)
  - v3:
    - removed Fully Established Error counter: should only happen with
      the userspace PM, which will propagate the error in this case
      (ENOTCONN). (Geliang)
---
 net/mptcp/mib.c     |  4 ++++
 net/mptcp/mib.h     |  4 ++++
 net/mptcp/subflow.c | 21 ++++++++++++++++++---
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index 7884217f33eb..ec0d461cb921 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -25,6 +25,10 @@ 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("MPJoinSynTxCreatSkErr", MPTCP_MIB_JOINSYNTXCREATSKERR),
+	SNMP_MIB_ITEM("MPJoinSynTxBindErr", MPTCP_MIB_JOINSYNTXBINDERR),
+	SNMP_MIB_ITEM("MPJoinSynTxConnectErr", MPTCP_MIB_JOINSYNTXCONNECTERR),
 	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..d68136f93dac 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -20,6 +20,10 @@ 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_JOINSYNTXCREATSKERR,	/* Not able to create a socket when sending a SYN + MP_JOIN */
+	MPTCP_MIB_JOINSYNTXBINDERR,	/* Not able to bind() the address when sending a SYN + MP_JOIN */
+	MPTCP_MIB_JOINSYNTXCONNECTERR,	/* 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 a7fb4d46e024..fdeb7df8b095 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1575,12 +1575,17 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_local *local,
 	u32 remote_token;
 	int addrlen;
 
+	/* The userspace PM sent the request too early? */
 	if (!mptcp_is_fully_established(sk))
 		goto err_out;
 
 	err = mptcp_subflow_create_socket(sk, local->addr.family, &sf);
-	if (err)
+	if (err) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXCREATSKERR);
+		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);
@@ -1615,8 +1620,12 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_local *local,
 #endif
 	ssk->sk_bound_dev_if = local->ifindex;
 	err = kernel_bind(sf, (struct sockaddr *)&addr, addrlen);
-	if (err)
+	if (err) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXBINDERR);
+		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,
@@ -1631,8 +1640,14 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_local *local,
 	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_JOINSYNTXCONNECTERR);
+		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.2
Re: [PATCH mptcp-next v3 2/8] mptcp: MIB counters for sent MP_JOIN
Posted by Geliang Tang 3 months, 4 weeks ago 
On Tue, 2024-08-06 at 13:18 +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>
> ---
> Notes:
>   - v2:
>     - Add "ERR" suffix in variable names. (Geliang)
>   - v3:
>     - removed Fully Established Error counter: should only happen
> with
>       the userspace PM, which will propagate the error in this case
>       (ENOTCONN). (Geliang)
> ---
>  net/mptcp/mib.c     |  4 ++++
>  net/mptcp/mib.h     |  4 ++++
>  net/mptcp/subflow.c | 21 ++++++++++++++++++---
>  3 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
> index 7884217f33eb..ec0d461cb921 100644
> --- a/net/mptcp/mib.c
> +++ b/net/mptcp/mib.c
> @@ -25,6 +25,10 @@ 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("MPJoinSynTxCreatSkErr",
> MPTCP_MIB_JOINSYNTXCREATSKERR),
> +	SNMP_MIB_ITEM("MPJoinSynTxBindErr",
> MPTCP_MIB_JOINSYNTXBINDERR),
> +	SNMP_MIB_ITEM("MPJoinSynTxConnectErr",
> MPTCP_MIB_JOINSYNTXCONNECTERR),
>  	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..d68136f93dac 100644
> --- a/net/mptcp/mib.h
> +++ b/net/mptcp/mib.h
> @@ -20,6 +20,10 @@ 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_JOINSYNTXCREATSKERR,	/* Not able to create a
> socket when sending a SYN + MP_JOIN */
> +	MPTCP_MIB_JOINSYNTXBINDERR,	/* Not able to bind() the
> address when sending a SYN + MP_JOIN */
> +	MPTCP_MIB_JOINSYNTXCONNECTERR,	/* 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 a7fb4d46e024..fdeb7df8b095 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1575,12 +1575,17 @@ int __mptcp_subflow_connect(struct sock *sk,
> const struct mptcp_pm_local *local,
>  	u32 remote_token;
>  	int addrlen;
>  
> +	/* The userspace PM sent the request too early? */
>  	if (!mptcp_is_fully_established(sk))
>  		goto err_out;
>  
>  	err = mptcp_subflow_create_socket(sk, local->addr.family,
> &sf);
> -	if (err)
> +	if (err) {
> +		MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNTXCREATSKERR);
> +		pr_debug("msk=%p local=%d remote:%d create sock
> error: %d\n",

It's better to use "remote=%d" instead of "remote:%d" I guess. Same
below.

> +			 msk, local_id, remote_id, err);
>  		goto err_out;
> +	}
>  
>  	ssk = sf->sk;
>  	subflow = mptcp_subflow_ctx(ssk);
> @@ -1615,8 +1620,12 @@ int __mptcp_subflow_connect(struct sock *sk,
> const struct mptcp_pm_local *local,
>  #endif
>  	ssk->sk_bound_dev_if = local->ifindex;
>  	err = kernel_bind(sf, (struct sockaddr *)&addr, addrlen);
> -	if (err)
> +	if (err) {
> +		MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNTXBINDERR);
> +		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,
> @@ -1631,8 +1640,14 @@ int __mptcp_subflow_connect(struct sock *sk,
> const struct mptcp_pm_local *local,
>  	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_JOINSYNTXCONNECTERR);
> +		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);
>
Re: [PATCH mptcp-next v3 2/8] mptcp: MIB counters for sent MP_JOIN
Posted by Matthieu Baerts 3 months, 4 weeks ago 
Hi Geliang,

On 08/08/2024 04:46, Geliang Tang wrote:
> On Tue, 2024-08-06 at 13:18 +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.

(...)

>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index a7fb4d46e024..fdeb7df8b095 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -1575,12 +1575,17 @@ int __mptcp_subflow_connect(struct sock *sk,
>> const struct mptcp_pm_local *local,
>>  	u32 remote_token;
>>  	int addrlen;
>>  
>> +	/* The userspace PM sent the request too early? */
>>  	if (!mptcp_is_fully_established(sk))
>>  		goto err_out;
>>  
>>  	err = mptcp_subflow_create_socket(sk, local->addr.family,
>> &sf);
>> -	if (err)
>> +	if (err) {
>> +		MPTCP_INC_STATS(sock_net(sk),
>> MPTCP_MIB_JOINSYNTXCREATSKERR);
>> +		pr_debug("msk=%p local=%d remote:%d create sock
>> error: %d\n",
> 
> It's better to use "remote=%d" instead of "remote:%d" I guess. Same
> below.

Indeed, I see we usually use '=', I can do the modification when
applying the patches if there is nothing bigger to fix.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.