| Message ID | 2a89f983d02141661c2f7cf0bfb4109ee74db60e.1708588977.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | some cleanups | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 65 lines checked |
| matttbe/build | success | Build and static analysis OK |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
Hi Geliang,
Thank you for your modifications, that's great!
Our CI (GitHub Action) did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8001362171
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ddc6b3c484c4
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 Geliang, On 22/02/2024 09:03, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > This patch adds a new helper userspace_pm_flush() to flush all addresses > for the userspace PM. Invoke it in userspace pm dump address and subflow > tests. And use dump commands to check if the userspace pm local address > list is empty after addresses flushing. I'm sorry to insist, but please *always* *always* *always* add an answer to this question in your commit message: why should we take this patch? Answers can be very short -- e.g. avoid duplicated code, fix a bug when doing X, this new feature is useful for this use-case, etc. -- but these answers *have to* be present. Here, the commit message only explains what is being done: that's interesting to verify if what you wanted to do is what you did. But most of the time, we can see what is being done by quickly looking at the diff. What is *not easy* to get by looking at the diff is the *reason* why this patch is needed. Of course, ↑ is valid for any patches for the Linux kernel, as mentioned in different places in the doc: - https://docs.kernel.org/process/submitting-patches.html?highlight=reason#the-canonical-patch-format > The explanation body will be committed to the permanent source changelog, so should make sense to a competent reader who has long since forgotten the immediate details of the discussion that might have led to this patch. Including symptoms of the failure which the patch addresses (kernel log messages, oops messages, etc.) are especially useful for people who might be searching the commit logs looking for the applicable patch. The text should be written in such detail so that when read weeks, months or even years later, it can give the reader the needed details to grasp the reasoning for why the patch was created. - https://docs.kernel.org/process/maintainer-netdev.html?highlight=reason#preparing-changes > If your change is a bug fix, make sure your commit log indicates the end-user visible symptom, the underlying reason as to why it happens, and then if necessary, explain why the fix proposed is the best way to get things done. Now back to this patch, I can very quickly see that you added a new helper, called it, and checked there were no more addresses. But without the reason in the commit message, I don't understand why we should do that: - the output of userspace_pm_dump() is already verified - the actions done by userspace_pm_rm_addr() and userspace_pm_rm_sf() helpers are also already verified. - it looks like we are not covering more .c code by doing that, no? → What are you trying to verify that was not verified before? Maybe we don't need the patch? Cheers, Matt
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh index 292fccb1f7f4..be1d2005c4fe 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh @@ -3356,6 +3356,19 @@ userspace_pm_get_addr() ip netns exec $1 ./pm_nl_ctl get $2 token $tk } +# $1: ns +userspace_pm_flush() +{ + local ns="${1}" + local id + local addr + + while read -r _ id _ _ addr; do + userspace_pm_rm_addr "${ns}" "${id}" + userspace_pm_rm_sf "${ns}" "${addr}" $SUB_ESTABLISHED + done <<< $(userspace_pm_dump "${ns}") +} + userspace_pm_chk_dump_addr() { local ns="${1}" @@ -3500,27 +3513,37 @@ userspace_tests() if reset_with_events "userspace pm create destroy subflow" && continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then set_userspace_pm $ns2 - pm_nl_set_limits $ns1 0 1 + pm_nl_set_limits $ns1 0 2 speed=5 \ run_tests $ns1 $ns2 10.0.1.1 & local tests_pid=$! wait_mpj $ns2 + userspace_pm_add_sf $ns2 10.0.2.2 10 userspace_pm_add_sf $ns2 10.0.3.2 20 - chk_join_nr 1 1 1 - chk_mptcp_info subflows 1 subflows 1 - chk_subflows_total 2 2 + chk_join_nr 2 2 2 + chk_mptcp_info subflows 2 subflows 2 + chk_subflows_total 3 3 userspace_pm_chk_dump_addr "${ns2}" \ - "id 20 flags subflow 10.0.3.2" \ + $'id 10 flags subflow 10.0.2.2\nid 20 flags subflow 10.0.3.2' \ "subflow" + userspace_pm_chk_get_addr "${ns2}" "10" "id 10 flags subflow 10.0.2.2" userspace_pm_chk_get_addr "${ns2}" "20" "id 20 flags subflow 10.0.3.2" userspace_pm_rm_addr $ns2 20 userspace_pm_rm_sf $ns2 10.0.3.2 $SUB_ESTABLISHED userspace_pm_chk_dump_addr "${ns2}" \ - "" \ + "id 10 flags subflow 10.0.2.2" \ "after rm_addr 20" - chk_rm_nr 1 1 - chk_mptcp_info subflows 0 subflows 0 - chk_subflows_total 1 1 + if mptcp_lib_kallsyms_has "mptcp_userspace_pm_dump_addr$"; then + userspace_pm_flush $ns2 + userspace_pm_chk_dump_addr "${ns2}" "" "after flush" + chk_rm_nr 2 2 + chk_mptcp_info subflows 0 subflows 0 + chk_subflows_total 1 1 + else + chk_rm_nr 1 1 + chk_mptcp_info subflows 1 subflows 1 + chk_subflows_total 2 2 + fi kill_events_pids mptcp_lib_kill_wait $tests_pid fi