From: YonglongLi <liyonglong@chinatelecom.cn>

it should inc RMADDR MIB counter when received RMADDR but didn't
find any subflow related,

Fixes: 7a7e52e38a40 ("mptcp: add RM_ADDR related mibs")

Signed-off-by: YonglongLi <liyonglong@chinatelecom.cn>
---
 net/mptcp/pm_netlink.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 7f53e02..766a840 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -814,10 +814,13 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
 			spin_lock_bh(&msk->pm.lock);
 
 			removed = true;
-			__MPTCP_INC_STATS(sock_net(sk), rm_type);
+			if (rm_type == MPTCP_MIB_RMSUBFLOW)
+				__MPTCP_INC_STATS(sock_net(sk), rm_type);
 		}
 		if (rm_type == MPTCP_MIB_RMSUBFLOW)
 			__set_bit(rm_id ? rm_id : msk->mpc_endpoint_id, msk->pm.id_avail_bitmap);
+		else if (rm_type == MPTCP_MIB_RMADDR)
+			__MPTCP_INC_STATS(sock_net(sk), rm_type);
 		if (!removed)
 			continue;
 
-- 
1.8.3.1

Hi Yonglong,

Thank you for sharing this patch!

On 23/05/2024 09:56, Yonglong Li wrote:
> From: YonglongLi <liyonglong@chinatelecom.cn>
> 
> it should inc RMADDR MIB counter when received RMADDR but didn't
> find any subflow related,

I suggest using this text instead:

 The RmAddr MIB counter is supposed to be incremented once when a valid
 RM_ADDR has been received. Before this patch, it could have been
 incremented as many times as the number of subflows connected to the
 the linked address ID, so it could have been 0, 1 or more than 1.

 The "RmSubflow" is incremented after a local operation. In this case,
 it is normal to tied it with the number of subflows that have been
 actually removed.

WDYT?

> 
> Fixes: 7a7e52e38a40 ("mptcp: add RM_ADDR related mibs")
> 

There should be no empty lines here between the Fixes and the SoB tags.

(I can modify the commit message when applying the patch, no need to
send a v2)

> Signed-off-by: YonglongLi <liyonglong@chinatelecom.cn>
> ---
>  net/mptcp/pm_netlink.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 7f53e02..766a840 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -814,10 +814,13 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
>  			spin_lock_bh(&msk->pm.lock);
>  
>  			removed = true;
> -			__MPTCP_INC_STATS(sock_net(sk), rm_type);
> +			if (rm_type == MPTCP_MIB_RMSUBFLOW)
> +				__MPTCP_INC_STATS(sock_net(sk), rm_type);
>  		}
>  		if (rm_type == MPTCP_MIB_RMSUBFLOW)
>  			__set_bit(rm_id ? rm_id : msk->mpc_endpoint_id, msk->pm.id_avail_bitmap);
> +		else if (rm_type == MPTCP_MIB_RMADDR)
> +			__MPTCP_INC_STATS(sock_net(sk), rm_type);

The modification looks good to me.

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

It would be good to modify the selftests to avoid any regressions later,
and to cover this case. But I'm not sure how to reproduce your case
where a RM_ADDR is received but for an ID that is not being used by the
PM. Do you know more about your case? Maybe because the subflows have
just been removed?

The issue to reproduce that in the selftests is that we cannot (easily)
send a RM_ADDR for an ID that is not currently handled by the PM (if I'm
not mistaken). Probably better to do that with Packetdrill [1].

What we can do instead, is to have subflows reusing the same ID multiple
times, e.g. using the fullmesh PM. Before the end of the connection, we
can send a RM_ADDR for an address that is used for multiple subflows by
the other end. Then we can check that the counter has been decreased
only once. That's what I tried to do, but I got another issue, see [2].

[1] https://github.com/multipath-tcp/packetdrill
[2] https://github.com/multipath-tcp/mptcp_net-next/issues/492

>  		if (!removed)
>  			continue;
>  

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

Hi YonglongLi,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/9204665497

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/d0561f49bc09
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=855243


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-normal

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 (NGI0 Core)