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

[Geliang Tang <geliang@kernel.org>]

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);
>