| Message ID | 1716450994-5145-1-git-send-email-liyonglong@chinatelecom.cn (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: make RMADDR MIB counter clearly | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/build | success | Build and static analysis OK |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
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)
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
Hi Matt, Thanks for your replay. On 5/24/2024 22:50, Matthieu Baerts wrote: > 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? Thanks. > >> >> 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? > I reporduce it just add a bcast ip addr which will cause join connect failed. Like this: if reset "remove addresses with join failed"; then pm_nl_set_limits $ns1 3 3 pm_nl_add_endpoint $ns1 224.0.0.1 flags signal pm_nl_add_endpoint $ns1 10.0.2.1 flags signal pm_nl_add_endpoint $ns1 10.0.3.1 flags signal pm_nl_set_limits $ns2 4 4 run_tests $ns1 $ns2 10.0.1.1 0 -3 0 speed_10 chk_join_nr 2 2 2 chk_add_nr 3 3 chk_rm_nr 3 2 invert chk_rst_nr 0 0 fi > 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
Hi Yonglong Li, On 27/05/2024 08:44, YonglongLi wrote: > On 5/24/2024 22:50, Matthieu Baerts wrote: >> On 23/05/2024 09:56, Yonglong Li wrote: (...) >> 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? >> > > I reporduce it just add a bcast ip addr which will cause join connect failed. > Like this: Good idea to use this broadcast IP address. I just sent a v2 of your patch modifying an existing test, instead of adding a dedicated one: https://lore.kernel.org/mptcp/20240527-mptcp-rmaddr-counter-once-v2-1-3e61360a90ac@kernel.org/T/ Please tell me if it is OK for you. Cheers, Matt
On 5/28/2024 00:05, 【外部账号】 Matthieu Baerts wrote: > Hi Yonglong Li, > > On 27/05/2024 08:44, YonglongLi wrote: >> On 5/24/2024 22:50, Matthieu Baerts wrote: >>> On 23/05/2024 09:56, Yonglong Li wrote: > > (...) > >>> 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? >>> >> >> I reporduce it just add a bcast ip addr which will cause join connect failed. >> Like this: > > Good idea to use this broadcast IP address. I just sent a v2 of your > patch modifying an existing test, instead of adding a dedicated one: > > > https://lore.kernel.org/mptcp/20240527-mptcp-rmaddr-counter-once-v2-1-3e61360a90ac@kernel.org/T/ > > Please tell me if it is OK for you. It's ok. Thank you. :) > > Cheers, > Matt
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;