Re: [PATCH mptcp-next v2 1/2] mptcp: pm: userspace: drop delete_local_addr helper

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Tue, 2025-04-15 at 13:00 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 15/04/2025 11:18, Geliang Tang wrote:
> > On Tue, 2025-04-15 at 11:09 +0800, Geliang Tang wrote:
> > > On Fri, 2025-04-11 at 17:26 -0700, Mat Martineau wrote:
> > > > On Thu, 10 Apr 2025, Geliang Tang wrote:
> > > > 
> > > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > > 
> > > > > Address entries should not be removed from local_addr_list
> > > > > when a
> > > > > subflow
> > > > > is deleted by the userspace PM, should only be removed when
> > > > > sending
> > > > > a
> > > > > REMOVE_ADDR.
> > > > > 
> > > > > So mptcp_userspace_pm_delete_local_addr() helper shouldn't be
> > > > > called in
> > > > > mptcp_pm_nl_subflow_create_doit() and
> > > > > mptcp_pm_nl_subflow_destroy_doit().
> > > > > 
> > > > > Since this helper is open-coding in
> > > > > mptcp_pm_nl_remove_doit(), it
> > > > > can be
> > > > > dropped now.
> > > > > 
> > > > > Address entries are removed from local_addr_list when sending
> > > > > a
> > > > > REMOVE_ADDR
> > > > > by the userspace PM, the local_addr_used counter of PM should
> > > > > also
> > > > > be
> > > > > decremented accordingly.
> > > > > 
> > > > 
> > > > Hi Geliang -
> > > > 
> > > > Have you tried this patch with mptcpd? Does it affect any tests
> > > > there?
> > > 
> > > I haven't tested it. To be honest, I don't know how to run the
> > > test
> > > items of mptcpd. Can you give me some guidance?
> 
> 'make check' should execute all the tests. Do you mind trying this
> with
> and without your modifications?

This is the output with and without my modifications:

make  check-TESTS
make[3]: Entering directory '/home/tgl/mptcpd/tests'
make[4]: Entering directory '/home/tgl/mptcpd/tests'
PASS: test-plugin
PASS: test-network-monitor
PASS: test-path-manager
PASS: test-commands
PASS: test-configuration
PASS: test-id-manager
PASS: test-listener-manager
PASS: test-sockaddr
PASS: test-addr-info
PASS: test-murmur-hash
PASS: test-cxx-build
PASS: test-bad-log-empty
PASS: test-bad-log-long
PASS: test-bad-log-short
PASS: test-bad-option
PASS: test-bad-path-manager
PASS: test-bad-plugin-dir
SKIP: test-start-stop
PASS: test-mptcpwrap
=======================================================================
=====
Testsuite summary for mptcpd 0.13
=======================================================================
=====
# TOTAL: 19
# PASS:  18
# SKIP:  1
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
=======================================================================
=====
make[4]: Leaving directory '/home/tgl/mptcpd/tests'
make[3]: Leaving directory '/home/tgl/mptcpd/tests'
make[2]: Leaving directory '/home/tgl/mptcpd/tests'
make[1]: Leaving directory '/home/tgl/mptcpd/tests'
Making check in scripts
make[1]: Entering directory '/home/tgl/mptcpd/scripts'
make[1]: Nothing to be done for 'check'.
make[1]: Leaving directory '/home/tgl/mptcpd/scripts'
make[1]: Entering directory '/home/tgl/mptcpd'
make[1]: Nothing to be done for 'check-am'.
make[1]: Leaving directory '/home/tgl/mptcpd'

> 
> Please make sure relevant ones are not skipped because of missing
> dependences or tools.
> 
> > I just tested it this way, and here are the test results:
> 
> (...)
> 
> > These results appear to be unchanged from before. And in fact these
> > tests do not test userspace pm.
> 
> I don't remember what is being tested, but I guess some parts of the
> userspace PM are tested somewhere, no?
> 
> Coveralls [1] seems to indicate it is tested, but I don't see the
> details.
> 
> [1] https://coveralls.io/github/multipath-tcp/mptcpd
> 
> > So it seems that this patch will not affect the tests of mptcpd.
> 
> Regarding your patch 2/2 of this series: did you have to modify the
> selftests to make the tests passing again due to a changed behaviour,
> or
> did you modify them to extend the behaviour.

The first one, I tried to make the tests passing again. I updated these
userspace selftests in v3 to make sure does not change the behaviour of
these tests.

Thanks,
-Geliang

> 
> It looks like the behaviour has changed, but it is unclear to me if
> it
> is to fix something or add a new feature.
> 
> Cheers,
> Matt