[MPTCP] Re: [PATCH v2 0/12] pm netlink: followup and fixes

[MPTCP] Re: [PATCH v2 0/12] pm netlink: followup and fixes

[Mat Martineau]

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


On Thu, 5 Mar 2020, Paolo Abeni wrote:

> This series bring some fixes and improvement related to the PM netlink,
> specifically:
>
> - renames 4th_ack subflow field to 'fully_established':
>  this is hopefully more accurate
> - implements support to enforce the maximum number of subflows per msk
> - implements MP_JOIN 3rd ack retransmission and 4th ack delivery:
>  this [ab]use the TCP delack timer. I personally think the idea is nice and
>  avoids changes to the core TCP stack, but I guess it could be disputed
> - more accurare 'fully established' accounting as per RFC
> - drop the msk level 'fully_established' field, as we can use the first subflow
>  status without additional locking instead. This simplify core PM event
>  handling
> - adds more MP_JOIN related mib counters
> - update the PM netlink to enforce a configurable maximum number of subflows
>  per msk
> - extend the pm netlink self-tests to check the above APIs
> - uses amost all the above to fix the currently disable mp join self-tests and
> re-enable them
>
> Note: mp join self tests are still unstable, e.g. can fail randomly, due to an
> old race between mptcp_token_update_accept() and mptcp_token_get_sock(): e.g.
> a client may legitly try to create a subflow after that the first one is fully
> established but before the server has accepted the initial socket:
> mptcp_token_get_sock() will fail and MP_JOIN will fail too, incrementing
> MPTCP_MIB_JOINNOTOKEN.
> I think the above should be addressed in a separate series - otherwise this one
> will be too big.
>
> The up2date branch for this series (v2) is available here:
>
> https://github.com/pabeni/mptcp/tree/mptcp_net-next_part3_7
>
> v1 -> v2:
> - comments and readability changes in patch 04
>

Thanks for the fixes - I don't have any further comments, looks good to 
integrate in the topgit tree.

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH v2 0/12] pm netlink: followup and fixes

[Paolo Abeni]

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

On Wed, 2020-03-11 at 20:59 +0100, Matthieu Baerts wrote:
> *Paolo* : there is one comment for you below:
> 
> 
> - the first patch has been applied as a new topic (exported commit) at 
> the beginning of the series. → t/mptcp-rename-fourth-ack-field
> - 5a5f6637f84f: conflict in 
> t/mptcp-Add-handling-of-incoming-MP_JOIN-requests
> - b5b033c18651..a6e8ad2a3769: result
> 
> - 1e9a3e8639eb: "squashed" patch 2/12 in "mptcp: Add path manager 
> interface" (not on top of "mptcp: Add ADD_ADDR handling")
> - 02ebff8a2889: conflict in 
> t/mptcp-Implement-path-manager-interface-commands
> - a6e8ad2a3769..b9678d1875a3: result
> 
> - 0e515e7616d3: "squashed" patch 3/12 in "mptcp: Add handling of 
> incoming MP_JOIN requests"
> → *Paolo*: I removed the new comment "passive connection, attach to msk 
> socket" as you did in your branch. Was it OK?

Yep, look good to me!

I'll have a better look at the code while working at more follow-up
patches to fix sporadic self-tests failures.

Thank you for your great work!

Paolo

[MPTCP] Re: [PATCH v2 0/12] pm netlink: followup and fixes

[Matthieu Baerts]

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

Hi Mat, Paolo,

On 06/03/2020 02:37, Mat Martineau wrote:
> 
> On Thu, 5 Mar 2020, Paolo Abeni wrote:

[...]

>> The up2date branch for this series (v2) is available here:
>>
>> https://github.com/pabeni/mptcp/tree/mptcp_net-next_part3_7
>>
>> v1 -> v2:
>> - comments and readability changes in patch 04
>>
> 
> Thanks for the fixes - I don't have any further comments, looks good to 
> integrate in the topgit tree.

Thank you for the patches and the review!


*Paolo* : there is one comment for you below:


- the first patch has been applied as a new topic (exported commit) at 
the beginning of the series. → t/mptcp-rename-fourth-ack-field
- 5a5f6637f84f: conflict in 
t/mptcp-Add-handling-of-incoming-MP_JOIN-requests
- b5b033c18651..a6e8ad2a3769: result

- 1e9a3e8639eb: "squashed" patch 2/12 in "mptcp: Add path manager 
interface" (not on top of "mptcp: Add ADD_ADDR handling")
- 02ebff8a2889: conflict in 
t/mptcp-Implement-path-manager-interface-commands
- a6e8ad2a3769..b9678d1875a3: result

- 0e515e7616d3: "squashed" patch 3/12 in "mptcp: Add handling of 
incoming MP_JOIN requests"
→ *Paolo*: I removed the new comment "passive connection, attach to msk 
socket" as you did in your branch. Was it OK?
- a8393200f126: conflict in 
t/subflow-place-further-subflows-on-new-join_list
- b9678d1875a3..16774063f32f: result

- d1a5f94551a0: "squashed" patch 4/12 in "mptcp: Add handling of 
outgoing MP_JOIN requests"
- 16774063f32f..98f82f13c815: result

- 90aea70d750f: "squashed" patch 5/12 (with conflicts) in "mptcp: 
Implement path manager interface commands"
- 98f82f13c815..9f20f16976d5: result

- 55fcf97eb8a2: "squashed" patch 6/12 in "mptcp: allow dumping subflow 
context to userspace"
- bac8494b909d: "Signed-off-by" + "Co-developed-by"
- 9f20f16976d5..bd72cf4868c2: result

- ab0626ee746d: "squashed" patch 7/12 in "mptcp: add MIB counter 
infrastructure"
- 4938832cda6f: "Signed-off-by" + "Co-developed-by"
- bd72cf4868c2..9d4cccd2f4f2: result

- e73584a30099: "squashed" patch 8/12 in "mptcp: increment MIB counters 
in a few places"
- 9d4cccd2f4f2..565f38422c7e: result

- b163eff329fe: "squashed" patch 9/12 in "subflow: place further 
subflows on new 'join_list'"
- 565f38422c7e..6f8e4ee41d9b: result

- e51fa26aad11: "squashed" patch 10/12 in "mptcp: add netlink based PM"
- 6f8e4ee41d9b..4e1be8fa7025: result

- 02c781aa2467: "squashed" patch 11/12 in "selftests: add PM netlink 
functional tests"
- 4e1be8fa7025..284d58a7ac6d: result

- 4db8e0bf06fd: "squashed" patch 12/12 (with conflicts) in "selftests: 
add test-cases for MPTCP MP_JOIN"
- 284d58a7ac6d..96df58122739: result

Tests + export will be launched soon.

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