This patchset replaced all the pm_nl_ctl commands in selftests with the
'ip mptcp' commands.

Geliang Tang (4):
  selftests: mptcp: use 'ip mptcp' in mptcp_join.sh
  selftests: mptcp: use 'ip mptcp' in mptcp_sockopt.sh
  selftests: mptcp: use 'ip mptcp' in pm_netlink.sh
  selftests: mptcp: use 'ip mptcp' in simult_flows.sh

 .../testing/selftests/net/mptcp/mptcp_join.sh | 686 +++++++++---------
 .../selftests/net/mptcp/mptcp_sockopt.sh      |  12 +-
 .../testing/selftests/net/mptcp/pm_netlink.sh | 164 ++---
 .../selftests/net/mptcp/simult_flows.sh       |   6 +-
 4 files changed, 432 insertions(+), 436 deletions(-)

-- 
2.31.1

Hi Geliang,

On 04/01/2022 11:11, Geliang Tang wrote:
> This patchset replaced all the pm_nl_ctl commands in selftests with the
> 'ip mptcp' commands.

Thank you for looking at that, that's a good idea!

I see that you are using features that are not in iproute2 yet. This
depends on your "more patches from pm_nl_ctl" series, right?

We can easily modify the environment of our CI but not the other ones.
To continue having these other CIs validating MPTCP selftests, I think
we should do two things:

- check IPRoute2 version and skip the tests with a clear message if a
too old version is used. I guess these CI uses at least the last stable
version of IPRoute2.

- wait for patches to be at least in "iproute2-next" before applying
patches in our "export" branch: it means we can already apply the
modifications that doesn't needs your patches for iproute2.

(if it helps, we can also have an mptcp-iproute2-next repo, e.g. if we
see we have too many IPRoute2 patches waiting to be applied upstream)

WDYT?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Hi Matt,

On Tue, Jan 04, 2022 at 05:47:35PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 04/01/2022 11:11, Geliang Tang wrote:
> > This patchset replaced all the pm_nl_ctl commands in selftests with the
> > 'ip mptcp' commands.
> 
> Thank you for looking at that, that's a good idea!
> 
> I see that you are using features that are not in iproute2 yet. This
> depends on your "more patches from pm_nl_ctl" series, right?

Only ./mptcp_join.sh -b (backup) and ./mptcp_join.sh -m (fullmesh) depend
on the "more patches from pm_nl_ctl" series.

> 
> We can easily modify the environment of our CI but not the other ones.
> To continue having these other CIs validating MPTCP selftests, I think
> we should do two things:
> 
> - check IPRoute2 version and skip the tests with a clear message if a
> too old version is used. I guess these CI uses at least the last stable
> version of IPRoute2.
> 
> - wait for patches to be at least in "iproute2-next" before applying
> patches in our "export" branch: it means we can already apply the
> modifications that doesn't needs your patches for iproute2.
> 
> (if it helps, we can also have an mptcp-iproute2-next repo, e.g. if we
> see we have too many IPRoute2 patches waiting to be applied upstream)
> 
> WDYT?

Maybe it's better to use both pm_nl_ctl and 'ip mptcp'. Add some code to
check IPRoute2 version in mptcp_join.sh to decide which one to use.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

On Tue, 4 Jan 2022, Geliang Tang wrote:

> This patchset replaced all the pm_nl_ctl commands in selftests with the
> 'ip mptcp' commands.
>
> Geliang Tang (4):
>  selftests: mptcp: use 'ip mptcp' in mptcp_join.sh
>  selftests: mptcp: use 'ip mptcp' in mptcp_sockopt.sh
>  selftests: mptcp: use 'ip mptcp' in pm_netlink.sh
>  selftests: mptcp: use 'ip mptcp' in simult_flows.sh
>
> .../testing/selftests/net/mptcp/mptcp_join.sh | 686 +++++++++---------
> .../selftests/net/mptcp/mptcp_sockopt.sh      |  12 +-
> .../testing/selftests/net/mptcp/pm_netlink.sh | 164 ++---
> .../selftests/net/mptcp/simult_flows.sh       |   6 +-
> 4 files changed, 432 insertions(+), 436 deletions(-)
>
> -- 
> 2.31.1

Hi Geliang -

I think we had been using pm_nl_ctl to avoid possible problems with 
varying 'ip' command versions on host systems, and to make sure we are 
checking for kernel regressions rather than 'ip' regressions, and to be 
able to check the netlink API in ways that might not make sense for 
the 'ip mptcp' command.

Could you comment on the reasons to move away from pm_nl_ctl?


Thanks,

--
Mat Martineau
Intel

On Tue, Jan 04, 2022 at 05:41:21PM -0800, Mat Martineau wrote:
> On Tue, 4 Jan 2022, Geliang Tang wrote:
> 
> > This patchset replaced all the pm_nl_ctl commands in selftests with the
> > 'ip mptcp' commands.
> > 
> > Geliang Tang (4):
> >  selftests: mptcp: use 'ip mptcp' in mptcp_join.sh
> >  selftests: mptcp: use 'ip mptcp' in mptcp_sockopt.sh
> >  selftests: mptcp: use 'ip mptcp' in pm_netlink.sh
> >  selftests: mptcp: use 'ip mptcp' in simult_flows.sh
> > 
> > .../testing/selftests/net/mptcp/mptcp_join.sh | 686 +++++++++---------
> > .../selftests/net/mptcp/mptcp_sockopt.sh      |  12 +-
> > .../testing/selftests/net/mptcp/pm_netlink.sh | 164 ++---
> > .../selftests/net/mptcp/simult_flows.sh       |   6 +-
> > 4 files changed, 432 insertions(+), 436 deletions(-)
> > 
> > -- 
> > 2.31.1
> 
> Hi Geliang -
> 
> I think we had been using pm_nl_ctl to avoid possible problems with varying
> 'ip' command versions on host systems, and to make sure we are checking for
> kernel regressions rather than 'ip' regressions, and to be able to check the
> netlink API in ways that might not make sense for the 'ip mptcp' command.
> 
> Could you comment on the reasons to move away from pm_nl_ctl?
> 

Hi Mat,

How about using both pm_nl_ctl and 'ip mptcp' in the selftests? Add an
argument to choose which one to use. Or add the 'ip' command version check
to decide which one to use.

My intention to add these patches was to check whether all the changes in
pm_nl_ctl have been merged into 'ip mptcp'. I hesitated to use both
pm_nl_ctl and 'ip mptcp' or replace pm_nl_ctl with 'ip mptcp'. The latter
is easier to implement, so I chose the latter. Now I think maybe using both
of them is much better.

Thanks,
-Geliang

> 
> Thanks,
> 
> --
> Mat Martineau
> Intel
> 

On Wed, 5 Jan 2022, Geliang Tang wrote:

> On Tue, Jan 04, 2022 at 05:41:21PM -0800, Mat Martineau wrote:
>> On Tue, 4 Jan 2022, Geliang Tang wrote:
>>
>>> This patchset replaced all the pm_nl_ctl commands in selftests with the
>>> 'ip mptcp' commands.
>>>
>>> Geliang Tang (4):
>>>  selftests: mptcp: use 'ip mptcp' in mptcp_join.sh
>>>  selftests: mptcp: use 'ip mptcp' in mptcp_sockopt.sh
>>>  selftests: mptcp: use 'ip mptcp' in pm_netlink.sh
>>>  selftests: mptcp: use 'ip mptcp' in simult_flows.sh
>>>
>>> .../testing/selftests/net/mptcp/mptcp_join.sh | 686 +++++++++---------
>>> .../selftests/net/mptcp/mptcp_sockopt.sh      |  12 +-
>>> .../testing/selftests/net/mptcp/pm_netlink.sh | 164 ++---
>>> .../selftests/net/mptcp/simult_flows.sh       |   6 +-
>>> 4 files changed, 432 insertions(+), 436 deletions(-)
>>>
>>> --
>>> 2.31.1
>>
>> Hi Geliang -
>>
>> I think we had been using pm_nl_ctl to avoid possible problems with varying
>> 'ip' command versions on host systems, and to make sure we are checking for
>> kernel regressions rather than 'ip' regressions, and to be able to check the
>> netlink API in ways that might not make sense for the 'ip mptcp' command.
>>
>> Could you comment on the reasons to move away from pm_nl_ctl?
>>
>
> Hi Mat,
>
> How about using both pm_nl_ctl and 'ip mptcp' in the selftests? Add an
> argument to choose which one to use. Or add the 'ip' command version check
> to decide which one to use.
>

Hi Geliang -

I like the idea to add a command line option to use 'ip' commands, but 
still defaulting to pm_nl_ctl. It's important to have consistent behavior 
across systems with the default self tests (especially with various 
automated build systems like kbuild where we don't know or control the 
iproute2 version).

> My intention to add these patches was to check whether all the changes in
> pm_nl_ctl have been merged into 'ip mptcp'.

Yes, it's very useful to be able to run all these test cases with 'ip 
mptcp' in addition to pm_nl_ctl.

> I hesitated to use both pm_nl_ctl and 'ip mptcp' or replace pm_nl_ctl 
> with 'ip mptcp'. The latter is easier to implement, so I chose the 
> latter. Now I think maybe using both of them is much better.

I agree!

--
Mat Martineau
Intel