[MPTCP] Re: Checklist before sending to netdev

[MPTCP] Re: Checklist before sending to netdev

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]

On 13/12/2019 10:41, Paolo Abeni wrote:
> On Fri, 2019-12-13 at 10:34 +0100, Matthieu Baerts wrote:
>> On 13/12/2019 00:36, Matthieu Baerts wrote:
>>> Here are the items we need to do before this Friday evening (US time):
>>>
>>> - Test patches → *all*
>>
>>       - Compilations: [ OK ]
>>           - each commit with/without MPTCP, x86_64
>>           - i386
>>           - with/without IPv6
>>       - kselftests, with and without KASAN + PROVE_LOCKING: [ OK ]
>>           - on "mptcp: add basic kselftest for mptcp" commit
>>           - on "mptcp: process MP_CAPABLE data option." commit (new)
>>           - at the end of the series ("export" branch)
> 
> Thank you for all the effort!

Thank you for all the work you did too!
Same for all of you guys :-)

>>
>>> - Share cover-letter for part 3 → *Christoph*
>>
>> Done! Thank you!
>>
>>> - Review cover-letters → *all*
>>
>> I just added 2 new items on the list, please see:
>>
>>     https://annuel2.framapad.org/p/mptcp_upstreaming_20191220
>>
>>
>> [ ] Review commit messages → *all*:
>>       [X] Mat: sent some patches modifying the .topmsg
>>       [ ] Review patches
>>       [ ] Apply patches
>>
>>
>> [ ] Do we need a cover-letter for the cover-letters?:
>>       - I mean: a short introduction to explain what we are going to send:
>>           - The protocol is what it is and having a minimal set of
>> features already signifies a lot of code.
>>           - This minimal set of features includes: the possibility for
>> the userspace to establish and properly terminate MPTCP connections but
>> limited to one subflow while still being able to transfer data in both
>> directions
>>           - We decided to split everything in 3 parts to ease the reviews:
>>               - Part 1: Prerequisite TCP core changes
>>               - Part 2: MPTCP minimal set of features including selftests
>>               - Part 3: a switch from MPTCPv0 (RFC 6824) to MPTCPv1 (new
>> RFC 8684 going to obsolete the previous one in the coming days)
>>           - The idea would be to apply everything in once (?)
>>       - we can also include that in the cover letter of Part 1
> 
> Sounds good to me! I agree we could add this kind of info to the cover
> letter for part 1. Applying at once is not requried, I think. I mean:
> part 1 can be applied first, part 2 and 3 later on.

Sorry, what I mean is: should we ask that everything is applied in the 
same "merge window"?

Maybe in the cover letter of part 3, we should give more details about 
MPTCPv1: what it is, why we want to support it on drop the previous 
support, why we did it like that (some deployments only support MPTCPv0 
and didn't switch to the new version yet). I can comment about that on 
the dedicated email thread.

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: Checklist before sending to netdev

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2077 bytes --]


On Fri, 13 Dec 2019, Matthieu Baerts wrote:

> Hello,
>
> On 13/12/2019 00:36, Matthieu Baerts wrote:
>> Here are the items we need to do before this Friday evening (US time):
>> 
>> - Test patches → *all*
>
>    - Compilations: [ OK ]
>        - each commit with/without MPTCP, x86_64
>        - i386
>        - with/without IPv6
>    - kselftests, with and without KASAN + PROVE_LOCKING: [ OK ]
>        - on "mptcp: add basic kselftest for mptcp" commit
>        - on "mptcp: process MP_CAPABLE data option." commit (new)
>        - at the end of the series ("export" branch)
>
>> - Share cover-letter for part 3 → *Christoph*
>
> Done! Thank you!
>
>> - Review cover-letters → *all*
>
> I just added 2 new items on the list, please see:
>
>  https://annuel2.framapad.org/p/mptcp_upstreaming_20191220
>
>
> [ ] Review commit messages → *all*:
>    [X] Mat: sent some patches modifying the .topmsg
>    [ ] Review patches
>    [ ] Apply patches
>
>
> [ ] Do we need a cover-letter for the cover-letters?:
>    - I mean: a short introduction to explain what we are going to send:
>        - The protocol is what it is and having a minimal set of features 
> already signifies a lot of code.
>        - This minimal set of features includes: the possibility for the 
> userspace to establish and properly terminate MPTCP connections but limited 
> to one subflow while still being able to transfer data in both directions
>        - We decided to split everything in 3 parts to ease the reviews:
>            - Part 1: Prerequisite TCP core changes
>            - Part 2: MPTCP minimal set of features including selftests
>            - Part 3: a switch from MPTCPv0 (RFC 6824) to MPTCPv1 (new RFC 
> 8684 going to obsolete the previous one in the coming days)
>        - The idea would be to apply everything in once (?)
>    - we can also include that in the cover letter of Part 1
>
>
> What do you think?
>

More context in the part 1 cover letter makes sense, I'll work on that.

--
Mat Martineau
Intel

[MPTCP] Re: Checklist before sending to netdev

[Davide Caratti]

[-- Attachment #1: Type: text/plain, Size: 167 bytes --]

On Fri, 2019-12-13 at 12:38 +0100, Paolo Abeni wrote:
> @Davide: packtdrill here would save our day to test!!!

v1 support is on the way, stay tuned!
-- 
davide

[MPTCP] Re: Checklist before sending to netdev

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 2949 bytes --]

On Fri, 2019-12-13 at 00:36 +0100, Matthieu Baerts wrote:
> Hello,
> 
> A few hours ago, I recreated the TopGit tree using Paolo's branch.
> 
> Now I just applied all pending patches except the ones adding the 
> MAINTAINERS file (because it is late and not affecting tests :) )
> 
> My CI detected that when running the kselftests on the latest "mptcp: 
> add basic kselftest for mptcp" commit (MPTCPv0), we have an issue. Not 
> when running them on "mptcp: process MP_CAPABLE data option." (MPTCPv1) 
> or at the end of the series.
> It seems that the issue has been introduced after having applied patches 
> cleaning the fallback handling.
> Paolo is already looking at that in cave mode.
> 
> Please also note that now the CI is testing the kselftests with 3 
> different version:
> 
> - on "mptcp: add basic kselftest for mptcp" commit
> - on "mptcp: process MP_CAPABLE data option." commit (new)
> - at the end of the series ("export" branch)
> 
> 
> 
> Here are the items we need to do before this Friday evening (US time):
> 
> - Test patches → *all*
> 
> - Share cover-letter for part 3 → *Christoph*
> - Review cover-letters → *all*
> 
> - Remove the fix in KSelftest framework → *Matth*
> - Apply the fix in KSelftest framework only for CI → *Matth*
> 
> - Apply patches related to the MAINTAINERS file → *Matth*
> - Add our mailing-list in the MAINTAINERS file → *Matth*
> 
> - Share patch(es) fixing kselftests when introduced → *Paolo*
> - Review this/these patch(es) → *EU-based people* (and others later)
> - Apply these patches → *Matth*
> 
> - Share patches fixing remaining checkpatch issues → *Davide*
> - Review these patches → *EU-based people* (and others later)
> - Apply these patches → *Matth*
> 
> - Add Mat (part 1 & 2) and Christoph' (part 3) signed-off → *Matth*
> 
> - Share patches like we are going to send → *Matth*
> - Review these patches → *all*
> 
> - Send Part 1 → *Mat*
> - Send Part 2 → *Mat*
> - Send Part 3 → *Christoph*
> 
> Did I miss anything? Anything wrong?

I know all of you will love the following, but we have a not so nice at
all issue with v1 support when falling back to tcp after the tcp
subflow goes into established status:

syn+ MP_CAPABLE ->
               <- syn, ack + MP_CAPABLE

ack, seq = 3    -> 
	// OoO packet is accepted because in-sequence
	// passive socket is created, tentatively as MP_CAPABLE
	// and user space does e.g. recvmsg() -> mptcp_recevmsg
	//  no fallback

ack, seq = 2     ->
	// no MP_CAPABLE, subflow should fallback to TCP
	// but msk->fallback == NULL (we don't have a fallback
	// struct socket at all)

	// disaster :((( deadlock, and/or stream corruption.

Thanks to Florian's help we have a tentative idea to address the above,
will keep you posted.

@Davide: packtdrill here would save our day to test!!!

Thanks,

Paolo

[MPTCP] Re: Checklist before sending to netdev

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 2252 bytes --]

On Fri, 2019-12-13 at 10:34 +0100, Matthieu Baerts wrote:
> On 13/12/2019 00:36, Matthieu Baerts wrote:
> > Here are the items we need to do before this Friday evening (US time):
> > 
> > - Test patches → *all*
> 
>      - Compilations: [ OK ]
>          - each commit with/without MPTCP, x86_64
>          - i386
>          - with/without IPv6
>      - kselftests, with and without KASAN + PROVE_LOCKING: [ OK ]
>          - on "mptcp: add basic kselftest for mptcp" commit
>          - on "mptcp: process MP_CAPABLE data option." commit (new)
>          - at the end of the series ("export" branch)

Thank you for all the effort!
> 
> > - Share cover-letter for part 3 → *Christoph*
> 
> Done! Thank you!
> 
> > - Review cover-letters → *all*
> 
> I just added 2 new items on the list, please see:
> 
>    https://annuel2.framapad.org/p/mptcp_upstreaming_20191220
> 
> 
> [ ] Review commit messages → *all*:
>      [X] Mat: sent some patches modifying the .topmsg
>      [ ] Review patches
>      [ ] Apply patches
> 
> 
> [ ] Do we need a cover-letter for the cover-letters?:
>      - I mean: a short introduction to explain what we are going to send:
>          - The protocol is what it is and having a minimal set of 
> features already signifies a lot of code.
>          - This minimal set of features includes: the possibility for 
> the userspace to establish and properly terminate MPTCP connections but 
> limited to one subflow while still being able to transfer data in both 
> directions
>          - We decided to split everything in 3 parts to ease the reviews:
>              - Part 1: Prerequisite TCP core changes
>              - Part 2: MPTCP minimal set of features including selftests
>              - Part 3: a switch from MPTCPv0 (RFC 6824) to MPTCPv1 (new 
> RFC 8684 going to obsolete the previous one in the coming days)
>          - The idea would be to apply everything in once (?)
>      - we can also include that in the cover letter of Part 1

Sounds good to me! I agree we could add this kind of info to the cover
letter for part 1. Applying at once is not requried, I think. I mean:
part 1 can be applied first, part 2 and 3 later on.

Cheers,

Paolo

[MPTCP] Re: Checklist before sending to netdev

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]

Hello,

On 13/12/2019 00:36, Matthieu Baerts wrote:
> Here are the items we need to do before this Friday evening (US time):
> 
> - Test patches → *all*

     - Compilations: [ OK ]
         - each commit with/without MPTCP, x86_64
         - i386
         - with/without IPv6
     - kselftests, with and without KASAN + PROVE_LOCKING: [ OK ]
         - on "mptcp: add basic kselftest for mptcp" commit
         - on "mptcp: process MP_CAPABLE data option." commit (new)
         - at the end of the series ("export" branch)

> - Share cover-letter for part 3 → *Christoph*

Done! Thank you!

> - Review cover-letters → *all*

I just added 2 new items on the list, please see:

   https://annuel2.framapad.org/p/mptcp_upstreaming_20191220


[ ] Review commit messages → *all*:
     [X] Mat: sent some patches modifying the .topmsg
     [ ] Review patches
     [ ] Apply patches


[ ] Do we need a cover-letter for the cover-letters?:
     - I mean: a short introduction to explain what we are going to send:
         - The protocol is what it is and having a minimal set of 
features already signifies a lot of code.
         - This minimal set of features includes: the possibility for 
the userspace to establish and properly terminate MPTCP connections but 
limited to one subflow while still being able to transfer data in both 
directions
         - We decided to split everything in 3 parts to ease the reviews:
             - Part 1: Prerequisite TCP core changes
             - Part 2: MPTCP minimal set of features including selftests
             - Part 3: a switch from MPTCPv0 (RFC 6824) to MPTCPv1 (new 
RFC 8684 going to obsolete the previous one in the coming days)
         - The idea would be to apply everything in once (?)
     - we can also include that in the cover letter of Part 1


What do you think?

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: Checklist before sending to netdev

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 2796 bytes --]

Hi Mat,

On 13/12/2019 00:47, Mat Martineau wrote:
> 
> On Fri, 13 Dec 2019, Matthieu Baerts wrote:
> 
>> Hello,
>>
>> A few hours ago, I recreated the TopGit tree using Paolo's branch.
>>
>> Now I just applied all pending patches except the ones adding the 
>> MAINTAINERS file (because it is late and not affecting tests :) )
>>
>> My CI detected that when running the kselftests on the latest "mptcp: 
>> add basic kselftest for mptcp" commit (MPTCPv0), we have an issue. Not 
>> when running them on "mptcp: process MP_CAPABLE data option." 
>> (MPTCPv1) or at the end of the series.
>> It seems that the issue has been introduced after having applied 
>> patches cleaning the fallback handling.
>> Paolo is already looking at that in cave mode.
>>
>> Please also note that now the CI is testing the kselftests with 3 
>> different version:
>>
>> - on "mptcp: add basic kselftest for mptcp" commit
>> - on "mptcp: process MP_CAPABLE data option." commit (new)
>> - at the end of the series ("export" branch)
>>
>>
>>
>> Here are the items we need to do before this Friday evening (US time):
>>
>> - Test patches → *all*
>>
>> - Share cover-letter for part 3 → *Christoph*
>> - Review cover-letters → *all*
>>
>> - Remove the fix in KSelftest framework → *Matth*
>> - Apply the fix in KSelftest framework only for CI → *Matth*
>>
>> - Apply patches related to the MAINTAINERS file → *Matth*
>> - Add our mailing-list in the MAINTAINERS file → *Matth*
>>
>> - Share patch(es) fixing kselftests when introduced → *Paolo*
>> - Review this/these patch(es) → *EU-based people* (and others later)
>> - Apply these patches → *Matth*
>>
>> - Share patches fixing remaining checkpatch issues → *Davide*
>> - Review these patches → *EU-based people* (and others later)
>> - Apply these patches → *Matth*
>>
>> - Add Mat (part 1 & 2) and Christoph' (part 3) signed-off → *Matth*
>>
>> - Share patches like we are going to send → *Matth*
>> - Review these patches → *all*
>>
>> - Send Part 1 → *Mat*
>> - Send Part 2 → *Mat*
>> - Send Part 3 → *Christoph*
>>
>> Did I miss anything? Anything wrong?
>>
> 
> Addition:
> 
>   - Tag part1/part2/part3 and push to github
> 
> It's fine if you leave that for me in case there are last-minute tweaks 
> affecting commit hashes.

Good point!

I will tag and if needed, a force push can be done ;-)

I just put the checklist on a pad: 
https://annuel2.framapad.org/p/mptcp_upstreaming_20191220

Feel free to update it there when something is done ;-)

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: Checklist before sending to netdev

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]


On Fri, 13 Dec 2019, Matthieu Baerts wrote:

> Hello,
>
> A few hours ago, I recreated the TopGit tree using Paolo's branch.
>
> Now I just applied all pending patches except the ones adding the MAINTAINERS 
> file (because it is late and not affecting tests :) )
>
> My CI detected that when running the kselftests on the latest "mptcp: add 
> basic kselftest for mptcp" commit (MPTCPv0), we have an issue. Not when 
> running them on "mptcp: process MP_CAPABLE data option." (MPTCPv1) or at the 
> end of the series.
> It seems that the issue has been introduced after having applied patches 
> cleaning the fallback handling.
> Paolo is already looking at that in cave mode.
>
> Please also note that now the CI is testing the kselftests with 3 different 
> version:
>
> - on "mptcp: add basic kselftest for mptcp" commit
> - on "mptcp: process MP_CAPABLE data option." commit (new)
> - at the end of the series ("export" branch)
>
>
>
> Here are the items we need to do before this Friday evening (US time):
>
> - Test patches → *all*
>
> - Share cover-letter for part 3 → *Christoph*
> - Review cover-letters → *all*
>
> - Remove the fix in KSelftest framework → *Matth*
> - Apply the fix in KSelftest framework only for CI → *Matth*
>
> - Apply patches related to the MAINTAINERS file → *Matth*
> - Add our mailing-list in the MAINTAINERS file → *Matth*
>
> - Share patch(es) fixing kselftests when introduced → *Paolo*
> - Review this/these patch(es) → *EU-based people* (and others later)
> - Apply these patches → *Matth*
>
> - Share patches fixing remaining checkpatch issues → *Davide*
> - Review these patches → *EU-based people* (and others later)
> - Apply these patches → *Matth*
>
> - Add Mat (part 1 & 2) and Christoph' (part 3) signed-off → *Matth*
>
> - Share patches like we are going to send → *Matth*
> - Review these patches → *all*
>
> - Send Part 1 → *Mat*
> - Send Part 2 → *Mat*
> - Send Part 3 → *Christoph*
>
> Did I miss anything? Anything wrong?
>

Addition:

  - Tag part1/part2/part3 and push to github

It's fine if you leave that for me in case there are last-minute tweaks 
affecting commit hashes.

--
Mat Martineau
Intel