[MPTCP] Re: RFCv2 for netdev: what's missing?

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Florian Westphal]

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

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> > For RFC I think it depends on how much work you want to
> > get yourself into right now, as it would need to be squashed, sending
> > it as extra patch is strange.
> 
> In general. It is linked to my message I sent on the thread linked to
> the patch:

Ah, thanks for the pointer.

> > The main part of this patch is removing the parsing of the checksum
> > option. But in the commits where this is introduced, we also parse
> > other possible options we could have and we act differently if one
> > option is not supported. And in these commits, it makes sense to do
> > that.
> >
> > We could see that as "we are parsing stuff we don't need". But we are
> > already doing that for the "backup" bit for example. Or for the
> > "MP_JOIN" while we don't support it when subflow_request_sock
> > structure is introduced, etc.
> >
> > In other words, do we really need to remove this code linked to the
> > checksum? If we plan never to support it, it makes sense. But I guess
> > in the near future, we will want to support it, no?
> >
> > So should I continue applying this patch?

Oh, OK. I thought the consensus was to not support checksums.
If we're going to add it for the initial mptcp v1 submission, the code
should be kept.

OTOH, if we're going to add checksums later on -- after initial
upstreaming -- I think its better to remove it now and resurrect it later
once someone adds the feature.

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Matthieu Baerts]

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

Hi Florian,

On 03/10/2019 21:21, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
>> I think that:
>>
>>        tcp: clean ext on tx recycle
>>
>> does have some minimal mptcp dependencies that can't be removed without
>> making such patch a no-op, so perhaps we should also include some very
>> minimal MPTCP stub definitions:
> 
> Why?  It doesn't have any MPTCP dep.  Just zap all the extension space
> and netfilter conntrack entry.
> 
> AFAICS things might already go wrong in case we get recycle skb that
> has an ipsec secpath assigned, or am I missing something?
> 
> And unless there is something in place that removes skb->nfct before
> the skb is placed on the socket cache, rmmod nf_conntrack can block
> forever as such queued skb holds a reference on conntrack struct.
> 
>> I *think* we can also do a bolder step and send this next iteration as
>> _NOT_ RFC, aiming at inclusion. My [mis]understanding is that we
>> should post later chunks after the first one is merged, easily.
> 
> Hmm, we can try but I am not sure upstream would accept exposure of
> some tcp core functions without a in-tree user.

We discussed a bit about these two topics at yesterday's meeting. May
you have a look at the minutes I sent this morning please? I guess the
list we established yesterday can be reduced, right?

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: RFCv2 for netdev: what's missing?

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> I think that:
> 
>        tcp: clean ext on tx recycle
> 
> does have some minimal mptcp dependencies that can't be removed without
> making such patch a no-op, so perhaps we should also include some very
> minimal MPTCP stub definitions:

Why?  It doesn't have any MPTCP dep.  Just zap all the extension space
and netfilter conntrack entry.

AFAICS things might already go wrong in case we get recycle skb that
has an ipsec secpath assigned, or am I missing something?

And unless there is something in place that removes skb->nfct before
the skb is placed on the socket cache, rmmod nf_conntrack can block
forever as such queued skb holds a reference on conntrack struct.

> I *think* we can also do a bolder step and send this next iteration as
> _NOT_ RFC, aiming at inclusion. My [mis]understanding is that we
> should post later chunks after the first one is merged, easily.

Hmm, we can try but I am not sure upstream would accept exposure of
some tcp core functions without a in-tree user.

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Paolo Abeni]

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

Hi Mat,

On Wed, 2019-10-02 at 17:39 -0700, Mat Martineau wrote:
> On Wed, 2 Oct 2019, Mat Martineau wrote:
> > On Wed, 2 Oct 2019, Mat Martineau wrote:
> > > On Wed, 2 Oct 2019, Matthieu Baerts wrote:
> > > > On my side, I think our last "export" branch is ready: all the patches
> > > > we previously mentioned are merged, it is rebased on latest net-next,
> > > > tests are all good. I think it is ready to be sent! I just tagged
> > > > netdev-rfcv2 but feel free to modify it if it is not OK :)
> > > > 
> > > > With a bit of luck, if you have time to send it and if nothing is
> > > > missing, we might have the first feedbacks before the meeting tomorrow!
> > > > Or yeah, more likely for the week after. But we never know :)
> > > 
> > > I just sent the 45 (!) patch series to netdev. Thanks for all of your work 
> > > to prepare the branch, Matthieu.
> > > 
> > > I will also push the branch to my git.kernel.org repo to trigger a 
> > > LKP/0-day automated build.
> > > 
> > 
> > ...and I just figured out that I generated the .patch files from the wrong 
> > commits :(
> > 
> > Regenerating those and resending. Sorry!
> > 
> 
> Based on Dave's feedback, it looked like a bad idea to repost the correct 
> 43 patches today. Sorry again about the error on my part, we can discuss 
> how to partition the patch series (and possibly squash some more patches) 
> at the meeting tomorrow.

First things first,  _thank you_ so much for the several extra miles
you just did! - I'll ask my real boss if that makes you eligible for an
additional 17% discount for your first reservation in this nice Tuscany
B&B ;)

At least we good some real feedback based on the code. 

I do agree with Florian suggestion:

> I would suggest to make another RFC submission on Friday or Monday,
> consisting only of the non-mptcp patches, i.e.:
>
>      net: Make sock protocol value checks more specific
>      sock: Make sk_protocol a 16-bit value
>      tcp: Define IPPROTO_MPTCP
>      tcp, ulp: Add clone operation to tcp_ulp_ops
>      tcp: Prevent coalesce/collapse when skb has MPTCP extensions
>      tcp: Export low-level TCP functions
>      tcp: Check for filled TCP option space before SACK
>      tcp: clean ext on tx recycle
>      tcp: Expose tcp struct and routine for MPTCP

I think that:

       tcp: clean ext on tx recycle

does have some minimal mptcp dependencies that can't be removed without
making such patch a no-op, so perhaps we should also include some very
minimal MPTCP stub definitions:

       tcp: Add MPTCP option number
       mptcp: Add MPTCP to skb extensions

I *think* we can also do a bolder step and send this next iteration as
_NOT_ RFC, aiming at inclusion. My [mis]understanding is that we
should post later chunks after the first one is merged, easily.

Anyhow it looks like we will have some big topic for today's mtg! :)

Cheers,

Paolo

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Florian Westphal]

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

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> > Regenerating those and resending. Sorry!
> > 
> 
> Based on Dave's feedback, it looked like a bad idea to repost the correct 43
> patches today.

Yes, I agree it was good to not send it.

> Sorry again about the error on my part, we can discuss how to
> partition the patch series (and possibly squash some more patches) at the
> meeting tomorrow.

Unfortunately I won't make the meeting today.

I would suggest to make another RFC submission on Friday or Monday,
consisting only of the non-mptcp patches, i.e.:

      net: Make sock protocol value checks more specific
      sock: Make sk_protocol a 16-bit value
      tcp: Define IPPROTO_MPTCP
      tcp, ulp: Add clone operation to tcp_ulp_ops
      tcp: Prevent coalesce/collapse when skb has MPTCP extensions
      tcp: Export low-level TCP functions
      tcp: Check for filled TCP option space before SACK
      tcp: clean ext on tx recycle
      tcp: Expose tcp struct and routine for MPTCP


Wrt to 
      tcp: clean ext on tx recycle

I think this is in fact a bug fix, I think this should
call both skb_ext_reset+nf_reset_ct.

(I am not 100% sure the nf_ct pointer is always cleared already
 on skbs that we put back on per sk cache).

Same for secpath.

Cheers,
Florian

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Mat Martineau]

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


On Wed, 2 Oct 2019, Mat Martineau wrote:

> On Wed, 2 Oct 2019, Mat Martineau wrote:
>
>> 
>> On Wed, 2 Oct 2019, Matthieu Baerts wrote:
>> 
>>> On my side, I think our last "export" branch is ready: all the patches
>>> we previously mentioned are merged, it is rebased on latest net-next,
>>> tests are all good. I think it is ready to be sent! I just tagged
>>> netdev-rfcv2 but feel free to modify it if it is not OK :)
>>> 
>>> With a bit of luck, if you have time to send it and if nothing is
>>> missing, we might have the first feedbacks before the meeting tomorrow!
>>> Or yeah, more likely for the week after. But we never know :)
>> 
>> 
>> I just sent the 45 (!) patch series to netdev. Thanks for all of your work 
>> to prepare the branch, Matthieu.
>> 
>> I will also push the branch to my git.kernel.org repo to trigger a 
>> LKP/0-day automated build.
>> 
>
> ...and I just figured out that I generated the .patch files from the wrong 
> commits :(
>
> Regenerating those and resending. Sorry!
>

Based on Dave's feedback, it looked like a bad idea to repost the correct 
43 patches today. Sorry again about the error on my part, we can discuss 
how to partition the patch series (and possibly squash some more patches) 
at the meeting tomorrow.

--
Mat Martineau
Intel

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Mat Martineau]

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

On Wed, 2 Oct 2019, Mat Martineau wrote:

>
> On Wed, 2 Oct 2019, Matthieu Baerts wrote:
>
>> On my side, I think our last "export" branch is ready: all the patches
>> we previously mentioned are merged, it is rebased on latest net-next,
>> tests are all good. I think it is ready to be sent! I just tagged
>> netdev-rfcv2 but feel free to modify it if it is not OK :)
>> 
>> With a bit of luck, if you have time to send it and if nothing is
>> missing, we might have the first feedbacks before the meeting tomorrow!
>> Or yeah, more likely for the week after. But we never know :)
>
>
> I just sent the 45 (!) patch series to netdev. Thanks for all of your work to 
> prepare the branch, Matthieu.
>
> I will also push the branch to my git.kernel.org repo to trigger a LKP/0-day 
> automated build.
>

...and I just figured out that I generated the .patch files from the wrong 
commits :(

Regenerating those and resending. Sorry!


--
Mat Martineau
Intel

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Mat Martineau]

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


On Wed, 2 Oct 2019, Matthieu Baerts wrote:

> On my side, I think our last "export" branch is ready: all the patches
> we previously mentioned are merged, it is rebased on latest net-next,
> tests are all good. I think it is ready to be sent! I just tagged
> netdev-rfcv2 but feel free to modify it if it is not OK :)
>
> With a bit of luck, if you have time to send it and if nothing is
> missing, we might have the first feedbacks before the meeting tomorrow!
> Or yeah, more likely for the week after. But we never know :)


I just sent the 45 (!) patch series to netdev. Thanks for all of your work 
to prepare the branch, Matthieu.

I will also push the branch to my git.kernel.org repo to trigger a 
LKP/0-day automated build.

--
Mat Martineau
Intel

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Matthieu Baerts]

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

Hi Mat,

On 02/10/2019 01:07, Mat Martineau wrote:
> 
> On Tue, 1 Oct 2019, Paolo Abeni wrote:
> 
>> On Tue, 2019-10-01 at 21:40 +0200, Matthieu Baerts wrote:
>>> On 01/10/2019 19:21, Mat Martineau wrote:
>>>> On Tue, 1 Oct 2019, Matthieu Baerts wrote:
>>>>
>>>>> What do we still need to do?
>>>>> - the cover letter: Mat is it still OK to do it on your side?
>>>>
>>>> Here's the cover letter draft:
>>>
>>> Thank you for having prepared the new draft!
>>
>> +1! ;)
>>
>>>> """
>>>> The MPTCP upstreaming community has prepared a net-next RFCv2 patch set
>>>> for review.
>>>>
>>>> Clone/fetch:
>>>> https://github.com/multipath-tcp/mptcp_net-next.git (tag: netdev-rfcv2)
>>>>
>>>> Browse:
>>>> https://github.com/multipath-tcp/mptcp_net-next/tree/netdev-rfcv2
>>>>
>>>> With CONFIG_MPTCP=y, a socket created with IPPROTO_MPTCP will
>>>> attempt to
>>>> create an MPTCP connection but remains compatible with regular
>>>> TCP. IPPROTO_TCP socket behavior is unchanged.
>>>>
>>>> This implementation makes use of ULP between the userspace-facing MPTCP
>>>> socket and the set of in-kernel TCP sockets it controls. ULP has been
>>>> extended for use with listening sockets. skb_ext is used to carry MPTCP
>>>> metadata.
>>>>
>>>> The patch set includes self-tests to exercise MPTCP in various
>>>> connection and routing scenarios.
>>>>
>>>> We have more work planned to reach the initial feature set for merging,
>>>> notably:
>>>>
>>>> * IPv6
>>>>
>>>> * Comply with MPTCPv1 (RFC6824bis). This patch set supports only
>>>>   MPTCPv0 (RFC 6824 / experimental)
>>>>
>>>> * Proper MPTCP-level connection closing with DATA_FIN
>>>>
>>>> * Couple receive windows across sibling subflow TCP sockets as required
>>>>   by RFC 6824
>>>>
>>>> * Limit subflow ULP visibility to kernel space
>>>>
>>>> * Simple transmit scheduler that respects subflow 'backup' flags
>>>
>>> If we want to push a smaller patch upstream, will we implement all of
>>> these features?
>>>
>>> Should we also mention that in this potential first part, if OK for
>>> upstream, we would expect to have everything up to commit introducing
>>> the kselftests + a new one introducing IPv6 support?
>>
>> I think it would be a very good idea to try split all the above in 2 or
>> more slices. If there is agreement about that plan we could rephrase
>> with something alike:
>>
>> """
>> We have more work planned to reach the initial feature set notably:
>> """
>>
>> [same list as above]
>>
>> """
>> In order to simplify both the code review and the develpment process we
>> propose to split the above in smaller chunks, the first one including
>> everything presented here up to selftest, with the addition of basic
>> IPv6 support.
>> """
>>
>> WDYT?
> 
> I think that's a good way to frame it, I'd adjust it a little bit:
> 
> """
> In order to simplify both code review and the development process, we
> propose splitting the patch set in to smaller chunks. The first patch
> set for merging would include patches 1 to (TBD) and basic IPv6 support.
> """

Sounds good to me. I guess we can mention the commit introducing the
selftest support as a first idea.

We can still change the target or the order of the commits later. I
added a topic about that in the list of topics to discuss tomorrow. But
that would be good to have some feedbacks from the maintainers about
this target :)


On my side, I think our last "export" branch is ready: all the patches
we previously mentioned are merged, it is rebased on latest net-next,
tests are all good. I think it is ready to be sent! I just tagged
netdev-rfcv2 but feel free to modify it if it is not OK :)

With a bit of luck, if you have time to send it and if nothing is
missing, we might have the first feedbacks before the meeting tomorrow!
Or yeah, more likely for the week after. But we never know :)


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: RFCv2 for netdev: what's missing?

[Mat Martineau]

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


On Tue, 1 Oct 2019, Paolo Abeni wrote:

> On Tue, 2019-10-01 at 21:40 +0200, Matthieu Baerts wrote:
>> On 01/10/2019 19:21, Mat Martineau wrote:
>>> On Tue, 1 Oct 2019, Matthieu Baerts wrote:
>>>
>>>> What do we still need to do?
>>>> - the cover letter: Mat is it still OK to do it on your side?
>>>
>>> Here's the cover letter draft:
>>
>> Thank you for having prepared the new draft!
>
> +1! ;)
>
>>> """
>>> The MPTCP upstreaming community has prepared a net-next RFCv2 patch set
>>> for review.
>>>
>>> Clone/fetch:
>>> https://github.com/multipath-tcp/mptcp_net-next.git (tag: netdev-rfcv2)
>>>
>>> Browse:
>>> https://github.com/multipath-tcp/mptcp_net-next/tree/netdev-rfcv2
>>>
>>> With CONFIG_MPTCP=y, a socket created with IPPROTO_MPTCP will attempt to
>>> create an MPTCP connection but remains compatible with regular
>>> TCP. IPPROTO_TCP socket behavior is unchanged.
>>>
>>> This implementation makes use of ULP between the userspace-facing MPTCP
>>> socket and the set of in-kernel TCP sockets it controls. ULP has been
>>> extended for use with listening sockets. skb_ext is used to carry MPTCP
>>> metadata.
>>>
>>> The patch set includes self-tests to exercise MPTCP in various
>>> connection and routing scenarios.
>>>
>>> We have more work planned to reach the initial feature set for merging,
>>> notably:
>>>
>>> * IPv6
>>>
>>> * Comply with MPTCPv1 (RFC6824bis). This patch set supports only
>>>   MPTCPv0 (RFC 6824 / experimental)
>>>
>>> * Proper MPTCP-level connection closing with DATA_FIN
>>>
>>> * Couple receive windows across sibling subflow TCP sockets as required
>>>   by RFC 6824
>>>
>>> * Limit subflow ULP visibility to kernel space
>>>
>>> * Simple transmit scheduler that respects subflow 'backup' flags
>>
>> If we want to push a smaller patch upstream, will we implement all of
>> these features?
>>
>> Should we also mention that in this potential first part, if OK for
>> upstream, we would expect to have everything up to commit introducing
>> the kselftests + a new one introducing IPv6 support?
>
> I think it would be a very good idea to try split all the above in 2 or
> more slices. If there is agreement about that plan we could rephrase
> with something alike:
>
> """
> We have more work planned to reach the initial feature set notably:
> """
>
> [same list as above]
>
> """
> In order to simplify both the code review and the develpment process we
> propose to split the above in smaller chunks, the first one including
> everything presented here up to selftest, with the addition of basic
> IPv6 support.
> """
>
> WDYT?

I think that's a good way to frame it, I'd adjust it a little bit:

"""
In order to simplify both code review and the development process, we 
propose splitting the patch set in to smaller chunks. The first patch set 
for merging would include patches 1 to (TBD) and basic IPv6 support.
"""


--
Mat Martineau
Intel

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Paolo Abeni]

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

On Tue, 2019-10-01 at 21:40 +0200, Matthieu Baerts wrote:
> On 01/10/2019 19:21, Mat Martineau wrote:
> > On Tue, 1 Oct 2019, Matthieu Baerts wrote:
> > 
> > > What do we still need to do?
> > > - the cover letter: Mat is it still OK to do it on your side?
> > 
> > Here's the cover letter draft:
> 
> Thank you for having prepared the new draft!

+1! ;)

> > """
> > The MPTCP upstreaming community has prepared a net-next RFCv2 patch set
> > for review.
> > 
> > Clone/fetch:
> > https://github.com/multipath-tcp/mptcp_net-next.git (tag: netdev-rfcv2)
> > 
> > Browse:
> > https://github.com/multipath-tcp/mptcp_net-next/tree/netdev-rfcv2
> > 
> > With CONFIG_MPTCP=y, a socket created with IPPROTO_MPTCP will attempt to
> > create an MPTCP connection but remains compatible with regular
> > TCP. IPPROTO_TCP socket behavior is unchanged.
> > 
> > This implementation makes use of ULP between the userspace-facing MPTCP
> > socket and the set of in-kernel TCP sockets it controls. ULP has been
> > extended for use with listening sockets. skb_ext is used to carry MPTCP
> > metadata.
> > 
> > The patch set includes self-tests to exercise MPTCP in various
> > connection and routing scenarios.
> > 
> > We have more work planned to reach the initial feature set for merging,
> > notably:
> > 
> > * IPv6
> > 
> > * Comply with MPTCPv1 (RFC6824bis). This patch set supports only
> >   MPTCPv0 (RFC 6824 / experimental)
> > 
> > * Proper MPTCP-level connection closing with DATA_FIN
> > 
> > * Couple receive windows across sibling subflow TCP sockets as required
> >   by RFC 6824
> > 
> > * Limit subflow ULP visibility to kernel space
> > 
> > * Simple transmit scheduler that respects subflow 'backup' flags
> 
> If we want to push a smaller patch upstream, will we implement all of
> these features?
> 
> Should we also mention that in this potential first part, if OK for
> upstream, we would expect to have everything up to commit introducing
> the kselftests + a new one introducing IPv6 support?

I think it would be a very good idea to try split all the above in 2 or
more slices. If there is agreement about that plan we could rephrase
with something alike:

"""
We have more work planned to reach the initial feature set notably:
"""

[same list as above]

"""
In order to simplify both the code review and the develpment process we
propose to split the above in smaller chunks, the first one including
everything presented here up to selftest, with the addition of basic
IPv6 support.
"""

WDYT?

Thanks,

Paolo

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Matthieu Baerts]

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

Hi Mat,

On 01/10/2019 19:21, Mat Martineau wrote:
> 
> On Tue, 1 Oct 2019, Matthieu Baerts wrote:
> 
>> What do we still need to do?
>> - the cover letter: Mat is it still OK to do it on your side?
> 
> Here's the cover letter draft:

Thank you for having prepared the new draft!

> """
> The MPTCP upstreaming community has prepared a net-next RFCv2 patch set
> for review.
> 
> Clone/fetch:
> https://github.com/multipath-tcp/mptcp_net-next.git (tag: netdev-rfcv2)
> 
> Browse:
> https://github.com/multipath-tcp/mptcp_net-next/tree/netdev-rfcv2
> 
> With CONFIG_MPTCP=y, a socket created with IPPROTO_MPTCP will attempt to
> create an MPTCP connection but remains compatible with regular
> TCP. IPPROTO_TCP socket behavior is unchanged.
> 
> This implementation makes use of ULP between the userspace-facing MPTCP
> socket and the set of in-kernel TCP sockets it controls. ULP has been
> extended for use with listening sockets. skb_ext is used to carry MPTCP
> metadata.
> 
> The patch set includes self-tests to exercise MPTCP in various
> connection and routing scenarios.
> 
> We have more work planned to reach the initial feature set for merging,
> notably:
> 
> * IPv6
> 
> * Comply with MPTCPv1 (RFC6824bis). This patch set supports only
>   MPTCPv0 (RFC 6824 / experimental)
> 
> * Proper MPTCP-level connection closing with DATA_FIN
> 
> * Couple receive windows across sibling subflow TCP sockets as required
>   by RFC 6824
> 
> * Limit subflow ULP visibility to kernel space
> 
> * Simple transmit scheduler that respects subflow 'backup' flags

If we want to push a smaller patch upstream, will we implement all of
these features?

Should we also mention that in this potential first part, if OK for
upstream, we would expect to have everything up to commit introducing
the kselftests + a new one introducing IPv6 support?

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: RFCv2 for netdev: what's missing?

[Matthieu Baerts]

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

Hi,

On 01/10/2019 18:23, Mat Martineau wrote:
> 
> On Tue, 1 Oct 2019, Florian Westphal wrote:
> 
>> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>> At the last meeting, we said it would be good to send a new RFCv2 to
>>> netdev before the next meeting while windows are closed. I guess today
>>> would be good, no?
>>
>> Thanks for bringing this up.
>>
>>> What do we still need to do?
>>> - the cover letter: Mat is it still OK to do it on your side?
>>> - any other patches to apply before this?
>>
>> I think current export branch is fine for RFCv2.
>>
>>> Here are the pending patches:
>> [..]
>>
>>> - mptcp: Remove all traces of checksum support: do we want it?
>>
>> Do you mean in general or for RFC?
>>
>> For RFC I think it depends on how much work you want to
>> get yourself into right now, as it would need to be squashed, sending
>> it as extra patch is strange.
>>
>>> - mptcp: add MIB counter infrastructure: Waiting for accept (last
>>> review)
>>> - mptcp: allow MPTCP sockets by default: can be applied
>>> - mptcp: prefix mptcp_ to exposed pm_ routines.
>>
>> These three would be nice to have for sure.
>>
>>> For the moment, I am blocked with "mptcp: Remove all traces of checksum
>>> support" but I can drop this rebase to work on other patches if others
>>> are required for the RFCv2.
>>>
>>> What's your point of view on this?
>>
>> I think merging in the checksum support remove before RFCv2 and the
>> other three (MIB, mptcp-enable-by-default, mptcp_pm_ prefix) would be
>> great.  I think its even ok to delay the series for a few days to get
>> those in.
>>
>> Let me know if i can help.
> 
> I agree with Florian on the patch set priority and that it's ok to delay
> a little. I'll update the cover letter and send a draft.

Thank you all for having shared your point of view!

I will then apply the remaining pending patches:
- mptcp: Remove all traces of checksum support
- mptcp: allow dumping subflow context to userspace
- mptcp: add MIB counter infrastructure
- mptcp: allow MPTCP sockets by default
- mptcp: prefix mptcp_ to exposed pm_ routines.

And maybe these ones too if they are accepted before tomorrow afternoon.
- mptcp: Interim Path Manager: Waiting for accept (last review)
- mptcp_poll should not block on each subflow: Waiting for review
- selftests: prepare for mptcp ipv6 support: Waiting for accept (last
review)

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: RFCv2 for netdev: what's missing?

[Mat Martineau]

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


On Tue, 1 Oct 2019, Matthieu Baerts wrote:

> What do we still need to do?
> - the cover letter: Mat is it still OK to do it on your side?

Here's the cover letter draft:


"""
The MPTCP upstreaming community has prepared a net-next RFCv2 patch set
for review.

Clone/fetch:
https://github.com/multipath-tcp/mptcp_net-next.git (tag: netdev-rfcv2)

Browse:
https://github.com/multipath-tcp/mptcp_net-next/tree/netdev-rfcv2

With CONFIG_MPTCP=y, a socket created with IPPROTO_MPTCP will attempt to
create an MPTCP connection but remains compatible with regular
TCP. IPPROTO_TCP socket behavior is unchanged.

This implementation makes use of ULP between the userspace-facing MPTCP
socket and the set of in-kernel TCP sockets it controls. ULP has been
extended for use with listening sockets. skb_ext is used to carry MPTCP
metadata.

The patch set includes self-tests to exercise MPTCP in various
connection and routing scenarios.

We have more work planned to reach the initial feature set for merging,
notably:

* IPv6

* Comply with MPTCPv1 (RFC6824bis). This patch set supports only
   MPTCPv0 (RFC 6824 / experimental)

* Proper MPTCP-level connection closing with DATA_FIN

* Couple receive windows across sibling subflow TCP sockets as required
   by RFC 6824

* Limit subflow ULP visibility to kernel space

* Simple transmit scheduler that respects subflow 'backup' flags

Thank you for your review. You can find us at mptcp(a)lists.01.org and
https://is.gd/mptcp_upstream

"""


--
Mat Martineau
Intel

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Mat Martineau]

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


On Tue, 1 Oct 2019, Florian Westphal wrote:

> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>> At the last meeting, we said it would be good to send a new RFCv2 to
>> netdev before the next meeting while windows are closed. I guess today
>> would be good, no?
>
> Thanks for bringing this up.
>
>> What do we still need to do?
>> - the cover letter: Mat is it still OK to do it on your side?
>> - any other patches to apply before this?
>
> I think current export branch is fine for RFCv2.
>
>> Here are the pending patches:
> [..]
>
>> - mptcp: Remove all traces of checksum support: do we want it?
>
> Do you mean in general or for RFC?
>
> For RFC I think it depends on how much work you want to
> get yourself into right now, as it would need to be squashed, sending
> it as extra patch is strange.
>
>> - mptcp: add MIB counter infrastructure: Waiting for accept (last review)
>> - mptcp: allow MPTCP sockets by default: can be applied
>> - mptcp: prefix mptcp_ to exposed pm_ routines.
>
> These three would be nice to have for sure.
>
>> For the moment, I am blocked with "mptcp: Remove all traces of checksum
>> support" but I can drop this rebase to work on other patches if others
>> are required for the RFCv2.
>>
>> What's your point of view on this?
>
> I think merging in the checksum support remove before RFCv2 and the
> other three (MIB, mptcp-enable-by-default, mptcp_pm_ prefix) would be
> great.  I think its even ok to delay the series for a few days to get
> those in.
>
> Let me know if i can help.

I agree with Florian on the patch set priority and that it's ok to delay a 
little. I'll update the cover letter and send a draft.

--
Mat Martineau
Intel

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Peter Krystad]

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

On Tue, 2019-10-01 at 17:34 +0200, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> > > For RFC I think it depends on how much work you want to
> > > get yourself into right now, as it would need to be squashed, sending
> > > it as extra patch is strange.
> > 
> > In general. It is linked to my message I sent on the thread linked to
> > the patch:
> 
> Ah, thanks for the pointer.
> 
> > > The main part of this patch is removing the parsing of the checksum
> > > option. But in the commits where this is introduced, we also parse
> > > other possible options we could have and we act differently if one
> > > option is not supported. And in these commits, it makes sense to do
> > > that.
> > > 
> > > We could see that as "we are parsing stuff we don't need". But we are
> > > already doing that for the "backup" bit for example. Or for the
> > > "MP_JOIN" while we don't support it when subflow_request_sock
> > > structure is introduced, etc.
> > > 
> > > In other words, do we really need to remove this code linked to the
> > > checksum? If we plan never to support it, it makes sense. But I guess
> > > in the near future, we will want to support it, no?
> > > 
> > > So should I continue applying this patch?
> 
> Oh, OK. I thought the consensus was to not support checksums.
> If we're going to add it for the initial mptcp v1 submission, the code
> should be kept.

I concur, my understanding was that we would not support checksums in the
initial upstreaming, and no one volunteered that they might add it later, but
only suggested that it wasn't a useful feature.

This is why I prepared the patch. Sorry it doesn't squash more easily....

Peter.



> OTOH, if we're going to add checksums later on -- after initial
> upstreaming -- I think its better to remove it now and resurrect it later
> once someone adds the feature.
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Paolo Abeni]

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

On Tue, 2019-10-01 at 17:07 +0200, Matthieu Baerts wrote:
> On 01/10/2019 16:58, Paolo Abeni wrote:
> > Hi,
> > 
> > Thank you for raising this topic!
> > 
> > On Tue, 2019-10-01 at 16:37 +0200, Matthieu Baerts wrote:
> > > At the last meeting, we said it would be good to send a new RFCv2 to
> > > netdev before the next meeting while windows are closed. I guess today
> > > would be good, no?
> > 
> > Agreed ;)
> > 
> > > What do we still need to do?
> > > - the cover letter: Mat is it still OK to do it on your side?
> > > - any other patches to apply before this?
> > 
> > I personally think we can go even with the current git tree.
> > If we want to flush part of the pending backlog I would go only for
> > already accepted patches:
> > 
> > [...]
> > > - mptcp: allow dumping subflow context to userspace: can be applied
> > > - mptcp: allow MPTCP sockets by default: can be applied
> > 
> > The above ones: ^^^
> > 
> > The above to avoid possible last minute issues.
> 
> Make sense, thank you!
> So better for you to wait before removing traces of the Checksum then?

No strong opinion on this point, I'm ok either way. Likely, if are not
going to include csum support in the short term, it would be better
drop it down.

Cheers,

Paolo

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Matthieu Baerts]

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

Hi Florian,

Thank you for your answer!

On 01/10/2019 17:00, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>> At the last meeting, we said it would be good to send a new RFCv2 to
>> netdev before the next meeting while windows are closed. I guess today
>> would be good, no?
> 
> Thanks for bringing this up.
> 
>> What do we still need to do?
>> - the cover letter: Mat is it still OK to do it on your side?
>> - any other patches to apply before this?
> 
> I think current export branch is fine for RFCv2.
> 
>> Here are the pending patches:
> [..]
> 
>> - mptcp: Remove all traces of checksum support: do we want it?
> 
> Do you mean in general or for RFC?
> 
> For RFC I think it depends on how much work you want to
> get yourself into right now, as it would need to be squashed, sending
> it as extra patch is strange.

In general. It is linked to my message I sent on the thread linked to
the patch:

====

> On my side, I was looking at applying this patch. And after having
> split it in 5 smaller patches and starting applying half of them, I
> don't know if it is a good idea to fully drop the checksum (partial)
> support.
>
> The main part of this patch is removing the parsing of the checksum
> option. But in the commits where this is introduced, we also parse
> other possible options we could have and we act differently if one
> option is not supported. And in these commits, it makes sense to do
> that.
>
> We could see that as "we are parsing stuff we don't need". But we are
> already doing that for the "backup" bit for example. Or for the
> "MP_JOIN" while we don't support it when subflow_request_sock
> structure is introduced, etc.
>
> In other words, do we really need to remove this code linked to the
> checksum? If we plan never to support it, it makes sense. But I guess
> in the near future, we will want to support it, no?
>
> So should I continue applying this patch?

====

But I can continue to apply it, I already did the splitting work. But I
preferred to raise the question now to avoid a revert of all of this :)

> 
>> - mptcp: add MIB counter infrastructure: Waiting for accept (last review)
>> - mptcp: allow MPTCP sockets by default: can be applied
>> - mptcp: prefix mptcp_ to exposed pm_ routines.
> 
> These three would be nice to have for sure.

I can work on that! (Once the first one is accepted :) )

>> For the moment, I am blocked with "mptcp: Remove all traces of checksum
>> support" but I can drop this rebase to work on other patches if others
>> are required for the RFCv2.
>>
>> What's your point of view on this?
> 
> I think merging in the checksum support remove before RFCv2 and the
> other three (MIB, mptcp-enable-by-default, mptcp_pm_ prefix) would be
> great.  I think its even ok to delay the series for a few days to get
> those in.

If we have an agreement on the patch removing the checksum support, I
can apply everything needed tomorrow and the whole patch-set can be sent
during the evening/night for us if it is OK for Mat!

> Let me know if i can help.

Thank you!
It should be fine for me to apply the different patches :)

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: RFCv2 for netdev: what's missing?

[Matthieu Baerts]

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

Hi Paolo,

Thank you for your answer!

On 01/10/2019 16:58, Paolo Abeni wrote:
> Hi,
> 
> Thank you for raising this topic!
> 
> On Tue, 2019-10-01 at 16:37 +0200, Matthieu Baerts wrote:
>> At the last meeting, we said it would be good to send a new RFCv2 to
>> netdev before the next meeting while windows are closed. I guess today
>> would be good, no?
> 
> Agreed ;)
> 
>> What do we still need to do?
>> - the cover letter: Mat is it still OK to do it on your side?
>> - any other patches to apply before this?
> 
> I personally think we can go even with the current git tree.
> If we want to flush part of the pending backlog I would go only for
> already accepted patches:
> 
> [...]
>> - mptcp: allow dumping subflow context to userspace: can be applied
>> - mptcp: allow MPTCP sockets by default: can be applied
> 
> The above ones: ^^^
> 
> The above to avoid possible last minute issues.

Make sense, thank you!
So better for you to wait before removing traces of the Checksum then?

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: RFCv2 for netdev: what's missing?

[Florian Westphal]

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

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> At the last meeting, we said it would be good to send a new RFCv2 to
> netdev before the next meeting while windows are closed. I guess today
> would be good, no?

Thanks for bringing this up.

> What do we still need to do?
> - the cover letter: Mat is it still OK to do it on your side?
> - any other patches to apply before this?

I think current export branch is fine for RFCv2.

> Here are the pending patches:
[..]

> - mptcp: Remove all traces of checksum support: do we want it?

Do you mean in general or for RFC?

For RFC I think it depends on how much work you want to
get yourself into right now, as it would need to be squashed, sending
it as extra patch is strange.

> - mptcp: add MIB counter infrastructure: Waiting for accept (last review)
> - mptcp: allow MPTCP sockets by default: can be applied
> - mptcp: prefix mptcp_ to exposed pm_ routines.

These three would be nice to have for sure.

> For the moment, I am blocked with "mptcp: Remove all traces of checksum
> support" but I can drop this rebase to work on other patches if others
> are required for the RFCv2.
> 
> What's your point of view on this?

I think merging in the checksum support remove before RFCv2 and the
other three (MIB, mptcp-enable-by-default, mptcp_pm_ prefix) would be
great.  I think its even ok to delay the series for a few days to get
those in.

Let me know if i can help.

[MPTCP] Re: RFCv2 for netdev: what's missing?

[Paolo Abeni]

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

Hi,

Thank you for raising this topic!

On Tue, 2019-10-01 at 16:37 +0200, Matthieu Baerts wrote:
> At the last meeting, we said it would be good to send a new RFCv2 to
> netdev before the next meeting while windows are closed. I guess today
> would be good, no?

Agreed ;)

> What do we still need to do?
> - the cover letter: Mat is it still OK to do it on your side?
> - any other patches to apply before this?

I personally think we can go even with the current git tree.
If we want to flush part of the pending backlog I would go only for
already accepted patches:

[...]
> - mptcp: allow dumping subflow context to userspace: can be applied
> - mptcp: allow MPTCP sockets by default: can be applied

The above ones: ^^^

The above to avoid possible last minute issues.

Other opinions more than welcome!

Cheers,

Paolo