Re: [PATCH 5.10 51/84] selftests: mptcp: add ADD_ADDR timeout test case

Re: [PATCH 5.10 51/84] selftests: mptcp: add ADD_ADDR timeout test case

[Matthieu Baerts]

Hi Greg, Sasha,

(+ MPTCP upstream ML)

First, thank you again for maintaining the stable branches!

On 05/07/2022 13:58, Greg Kroah-Hartman wrote:
> From: Geliang Tang <geliangtang@gmail.com>
> 
> [ Upstream commit 8d014eaa9254a9b8e0841df40dd36782b451579a ]
> 
> This patch added the test case for retransmitting ADD_ADDR when timeout
> occurs. It set NS1's add_addr_timeout to 1 second, and drop NS2's ADD_ADDR
> echo packets.
TL;DR: Could it be possible to drop all selftests MPTCP patches from
v5.10 queue please?


I was initially reacting on this patch because it looks like it depends on:

  93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")

and indirectly to:

  9ce7deff92e8 ("docs: networking: mptcp: Add MPTCP sysctl entries")

to have "net.mptcp.add_addr_timeout" sysctl knob needed for this new
selftest.

But then I tried to understand why this current patch ("selftests:
mptcp: add ADD_ADDR timeout test case") has been selected for 5.10. I
guess it was to ease the backport of another one, right?
Looking at the 'series' file in 5.10 queue, it seems the new
"selftests-mptcp-more-stable-diag-tests" patch requires 5 other patches:

-> selftests-mptcp-more-stable-diag-tests.patch
 -> selftests-mptcp-fix-diag-instability.patch
  -> selftests-mptcp-launch-mptcp_connect-with-timeout.patch
   -> selftests-mptcp-add-add_addr-ipv6-test-cases.patch
    -> selftests-mptcp-add-link-failure-test-case.patch
     -> selftests-mptcp-add-add_addr-timeout-test-case.patch


When looking at these patches in more detail, it looks like "selftests:
mptcp: add ADD_ADDR IPv6 test cases" depends on a new feature only
available from v5.11: ADD_ADDR for IPv6.


Could it be possible to drop all these patches from v5.10 then please?


The two recent fixes for the "diag" selftest mainly helps on slow / busy
CI. I think it is not worth backporting them to v5.10.


(Note that if we want "selftests: mptcp: fix diag instability" patch, we
also need 2e580a63b5c2 ("selftests: mptcp: add cfg_do_w for cfg_remove")
and the top part of 8da6229b9524 ("selftests: mptcp: timeout testcases
for multi addresses"): the list starts to be long.)


One last thing: it looks like when Sasha adds patches to a stable queue,
a notification is sent to less people than when Greg adds patches. For
example here, I have not been notified for this patch when added to the
queue while I was one of the reviewers. I already got notifications from
Greg when I was a reviewer on other patches.
Is it normal? Do you only cc people who signed off on the patch?

It looks like you don't cc maintainers from the MAINTAINERS file but
that's probably on purpose. I didn't get cc for all MPTCP patches of the
series here but I guess I can always subscribe to 'stable' ML for that.


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

Re: [PATCH 5.10 51/84] selftests: mptcp: add ADD_ADDR timeout test case

[Greg Kroah-Hartman]

On Tue, Jul 05, 2022 at 05:59:22PM +0200, Matthieu Baerts wrote:
> Hi Greg, Sasha,
> 
> (+ MPTCP upstream ML)
> 
> First, thank you again for maintaining the stable branches!
> 
> On 05/07/2022 13:58, Greg Kroah-Hartman wrote:
> > From: Geliang Tang <geliangtang@gmail.com>
> > 
> > [ Upstream commit 8d014eaa9254a9b8e0841df40dd36782b451579a ]
> > 
> > This patch added the test case for retransmitting ADD_ADDR when timeout
> > occurs. It set NS1's add_addr_timeout to 1 second, and drop NS2's ADD_ADDR
> > echo packets.
> TL;DR: Could it be possible to drop all selftests MPTCP patches from
> v5.10 queue please?
> 
> 
> I was initially reacting on this patch because it looks like it depends on:
> 
>   93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")
> 
> and indirectly to:
> 
>   9ce7deff92e8 ("docs: networking: mptcp: Add MPTCP sysctl entries")
> 
> to have "net.mptcp.add_addr_timeout" sysctl knob needed for this new
> selftest.
> 
> But then I tried to understand why this current patch ("selftests:
> mptcp: add ADD_ADDR timeout test case") has been selected for 5.10. I
> guess it was to ease the backport of another one, right?
> Looking at the 'series' file in 5.10 queue, it seems the new
> "selftests-mptcp-more-stable-diag-tests" patch requires 5 other patches:
> 
> -> selftests-mptcp-more-stable-diag-tests.patch
>  -> selftests-mptcp-fix-diag-instability.patch
>   -> selftests-mptcp-launch-mptcp_connect-with-timeout.patch
>    -> selftests-mptcp-add-add_addr-ipv6-test-cases.patch
>     -> selftests-mptcp-add-link-failure-test-case.patch
>      -> selftests-mptcp-add-add_addr-timeout-test-case.patch
> 
> 
> When looking at these patches in more detail, it looks like "selftests:
> mptcp: add ADD_ADDR IPv6 test cases" depends on a new feature only
> available from v5.11: ADD_ADDR for IPv6.
> 
> 
> Could it be possible to drop all these patches from v5.10 then please?

Sure, but leave them in for 5.15.y and 5.18.y?

> The two recent fixes for the "diag" selftest mainly helps on slow / busy
> CI. I think it is not worth backporting them to v5.10.
> 
> 
> (Note that if we want "selftests: mptcp: fix diag instability" patch, we
> also need 2e580a63b5c2 ("selftests: mptcp: add cfg_do_w for cfg_remove")
> and the top part of 8da6229b9524 ("selftests: mptcp: timeout testcases
> for multi addresses"): the list starts to be long.)
> 
> 
> One last thing: it looks like when Sasha adds patches to a stable queue,
> a notification is sent to less people than when Greg adds patches. For
> example here, I have not been notified for this patch when added to the
> queue while I was one of the reviewers. I already got notifications from
> Greg when I was a reviewer on other patches.
> Is it normal? Do you only cc people who signed off on the patch?

I cc: everyone on the commit, Sasha should also do that but sometimes
his script acts up.

> It looks like you don't cc maintainers from the MAINTAINERS file but
> that's probably on purpose. I didn't get cc for all MPTCP patches of the
> series here but I guess I can always subscribe to 'stable' ML for that.

No, we don't use the MAINTAINERS file, that would just be too noisy as
ideally who ever the MAINTAINER was, they already saw this as the commit
is already in Linus's tree.

thanks,

greg k-h

Re: [PATCH 5.10 51/84] selftests: mptcp: add ADD_ADDR timeout test case

[Mat Martineau]

On Tue, 5 Jul 2022, Greg Kroah-Hartman wrote:

> On Tue, Jul 05, 2022 at 05:59:22PM +0200, Matthieu Baerts wrote:
>> Hi Greg, Sasha,
>>
>> (+ MPTCP upstream ML)
>>
>> First, thank you again for maintaining the stable branches!
>>
>> On 05/07/2022 13:58, Greg Kroah-Hartman wrote:
>>> From: Geliang Tang <geliangtang@gmail.com>
>>>
>>> [ Upstream commit 8d014eaa9254a9b8e0841df40dd36782b451579a ]
>>>
>>> This patch added the test case for retransmitting ADD_ADDR when timeout
>>> occurs. It set NS1's add_addr_timeout to 1 second, and drop NS2's ADD_ADDR
>>> echo packets.
>> TL;DR: Could it be possible to drop all selftests MPTCP patches from
>> v5.10 queue please?
>>
>>
>> I was initially reacting on this patch because it looks like it depends on:
>>
>>   93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")
>>
>> and indirectly to:
>>
>>   9ce7deff92e8 ("docs: networking: mptcp: Add MPTCP sysctl entries")
>>
>> to have "net.mptcp.add_addr_timeout" sysctl knob needed for this new
>> selftest.
>>
>> But then I tried to understand why this current patch ("selftests:
>> mptcp: add ADD_ADDR timeout test case") has been selected for 5.10. I
>> guess it was to ease the backport of another one, right?
>> Looking at the 'series' file in 5.10 queue, it seems the new
>> "selftests-mptcp-more-stable-diag-tests" patch requires 5 other patches:
>>
>> -> selftests-mptcp-more-stable-diag-tests.patch
>>  -> selftests-mptcp-fix-diag-instability.patch
>>   -> selftests-mptcp-launch-mptcp_connect-with-timeout.patch
>>    -> selftests-mptcp-add-add_addr-ipv6-test-cases.patch
>>     -> selftests-mptcp-add-link-failure-test-case.patch
>>      -> selftests-mptcp-add-add_addr-timeout-test-case.patch
>>
>>
>> When looking at these patches in more detail, it looks like "selftests:
>> mptcp: add ADD_ADDR IPv6 test cases" depends on a new feature only
>> available from v5.11: ADD_ADDR for IPv6.
>>
>>
>> Could it be possible to drop all these patches from v5.10 then please?
>
> Sure, but leave them in for 5.15.y and 5.18.y?
>

Hi Greg -

I'm the other MPTCP maintainer, jumping in here due to Matt's time zone.

Yes: leave selftests-mptcp-more-stable-diag-tests.patch in 5.15.y and 
5.18.y

--
Mat Martineau
Intel

Re: [PATCH 5.10 51/84] selftests: mptcp: add ADD_ADDR timeout test case

[Matthieu Baerts]

Hi Greg,

On 05/07/2022 18:31, Greg Kroah-Hartman wrote:
> On Tue, Jul 05, 2022 at 05:59:22PM +0200, Matthieu Baerts wrote:
>> Hi Greg, Sasha,
>>
>> (+ MPTCP upstream ML)
>>
>> First, thank you again for maintaining the stable branches!
>>
>> On 05/07/2022 13:58, Greg Kroah-Hartman wrote:
>>> From: Geliang Tang <geliangtang@gmail.com>
>>>
>>> [ Upstream commit 8d014eaa9254a9b8e0841df40dd36782b451579a ]
>>>
>>> This patch added the test case for retransmitting ADD_ADDR when timeout
>>> occurs. It set NS1's add_addr_timeout to 1 second, and drop NS2's ADD_ADDR
>>> echo packets.
>> TL;DR: Could it be possible to drop all selftests MPTCP patches from
>> v5.10 queue please?
>>
>>
>> I was initially reacting on this patch because it looks like it depends on:
>>
>>   93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")
>>
>> and indirectly to:
>>
>>   9ce7deff92e8 ("docs: networking: mptcp: Add MPTCP sysctl entries")
>>
>> to have "net.mptcp.add_addr_timeout" sysctl knob needed for this new
>> selftest.
>>
>> But then I tried to understand why this current patch ("selftests:
>> mptcp: add ADD_ADDR timeout test case") has been selected for 5.10. I
>> guess it was to ease the backport of another one, right?
>> Looking at the 'series' file in 5.10 queue, it seems the new
>> "selftests-mptcp-more-stable-diag-tests" patch requires 5 other patches:
>>
>> -> selftests-mptcp-more-stable-diag-tests.patch
>>  -> selftests-mptcp-fix-diag-instability.patch
>>   -> selftests-mptcp-launch-mptcp_connect-with-timeout.patch
>>    -> selftests-mptcp-add-add_addr-ipv6-test-cases.patch
>>     -> selftests-mptcp-add-link-failure-test-case.patch
>>      -> selftests-mptcp-add-add_addr-timeout-test-case.patch
>>
>>
>> When looking at these patches in more detail, it looks like "selftests:
>> mptcp: add ADD_ADDR IPv6 test cases" depends on a new feature only
>> available from v5.11: ADD_ADDR for IPv6.
>>
>>
>> Could it be possible to drop all these patches from v5.10 then please?
> 
> Sure, but leave them in for 5.15.y and 5.18.y?

(@Mat: Thank you for having replied to this part: yes, please leave them
there)

>> The two recent fixes for the "diag" selftest mainly helps on slow / busy
>> CI. I think it is not worth backporting them to v5.10.
>>
>>
>> (Note that if we want "selftests: mptcp: fix diag instability" patch, we
>> also need 2e580a63b5c2 ("selftests: mptcp: add cfg_do_w for cfg_remove")
>> and the top part of 8da6229b9524 ("selftests: mptcp: timeout testcases
>> for multi addresses"): the list starts to be long.)
>>
>>
>> One last thing: it looks like when Sasha adds patches to a stable queue,
>> a notification is sent to less people than when Greg adds patches. For
>> example here, I have not been notified for this patch when added to the
>> queue while I was one of the reviewers. I already got notifications from
>> Greg when I was a reviewer on other patches.
>> Is it normal? Do you only cc people who signed off on the patch?
> 
> I cc: everyone on the commit, Sasha should also do that but sometimes
> his script acts up.

All good, thank you!

>> It looks like you don't cc maintainers from the MAINTAINERS file but
>> that's probably on purpose. I didn't get cc for all MPTCP patches of the
>> series here but I guess I can always subscribe to 'stable' ML for that.
> 
> No, we don't use the MAINTAINERS file, that would just be too noisy as
> ideally who ever the MAINTAINER was, they already saw this as the commit
> is already in Linus's tree.

I understand, thank you for the explanation.

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