Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Rao Shoaib]

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



On 02/28/2018 02:01 PM, Rao Shoaib wrote:
>
>
> On 02/23/2018 12:20 PM, Mat Martineau wrote:
>>
>> On Thu, 22 Feb 2018, rao.shoaib(a)oracle.com wrote:
>>
>>> From: Rao Shoaib <rao.shoaib(a)oracle.com>
>>>
>>> Following patches modify TCP code to enable implementation of MPTCP. 
>>> MPTCP implementation requires sharing of TCP code with minor 
>>> modification here and there. In order to keep the TCP code clean and 
>>> easy to maintain, common code has been moved to new functions for 
>>> use by both TCP and MPTCP. struct tcp_sock now has function pointers 
>>> and based on the socket type (TCP/MPTCP) appropriate function is 
>>> called.
>>>
>>> A basic implementation of MPTCP that works with IPv4/IPv6 and 
>>> supports join has been tested based on these changes.
>>>
>>> The changes are being submitted as an RFC to get feedback from the 
>>> community and to start a discussion on how to move forward.
>>>
>>
>> Hi Rao -
>>
>> I'd encourage you to closely read David Miller's response 
>> (https://www.spinics.net/lists/netdev/msg481868.html) to the TCP-MD5 
>> RFC patch set that Christoph posted. We proposed some targeted 
>> indirect calls to callbacks for handling TCP options. He said "Sorry, 
>> I'm really not thrilled about this", and was focused on the 
>> performance implications of those callbacks. Have you looked at the 
>> performance and memory impact of adding indirection to a few dozen 
>> core TCP function calls?
> Yes I have. There are several papers on this subject and the jury is 
> out. In the case of the frame work I have not looked at the code but 
> Dave says
>
> "I can already see in your patches that new overhead is added, new
> tests in the packet building fast paths that are completely
> unnecessary with the existing code, etc."
>
> In my changes there are no new tests and fast is completely untouched. 
> The only change is to call functions via a pointer instead of having 
> inline. The impact of that has been a topic of discussion for a long 
> time because of c++.
>
>>
>> To give some more background, when Christoph was crafting the MD5 
>> patch set I pushed to take out some static-branch code that needed 
>> some work but eliminated the impact of the added callbacks for normal 
>> TCP. David focused on exactly that issue and didn't dive in to other 
>> aspects of the code. I can't fault him for that - he sorts through a 
>> staggering volume of patches every day, and if there's an issue 
>> that's clear at a glance it will pull all attention from the rest of 
>> the proposal. Also consider that Christoph replied with a few more 
>> questions, but the discussion stalled.
>>
>> I do have more to say about the proposed architecture and how to 
>> structure the patch set for netdev - but I think the maintainer 
>> feedback we already have about indirect callbacks is most critical to 
>> consider and factor in to what we send to netdev in the future. We 
>> have the shared goal to get MPTCP in to the upstream kernel and it's 
>> going to take a coordinated community to get there!
> I agree that it will take a co-ordinated effort but we need to know 
> what upstream will accept. I have not seen any code or a detailed 
> proposal.
>
> I will be sending out modified MPTCP code that works with the IP changes.
>
> What are the other issues besides the direct calls ?
>
> Rao.
So I just went back and looked at the fast path:

In Rx -- tcp_v4_rcv and onwards do not have a single pointer in case 
MPTCP is not used.

In Tx the only indirection is in transmit_skb for options, this is no 
different than calling previous options functions that were not inline 
any ways.

Most indirect function are called in the control path and corner cases.
Leaving the fast path untouched was one of the main guiding point. Also 
I tried to make changes such that main tcp code could be enhanced 
without worrying about MPTCP.
I admit that my solution is not perfect but it's better than what we 
have now and is worth sharing upstream. In a private conversation with 
Dave and Eric I did mention that I was using function pointers and I did 
not hear any push back. Also note in our conversation they agreed to 
take a small performance hit.

Shoaib


>
>>
>>
>> Thanks,
>> Mat
>>
>> -- 
>> Mat Martineau
>> Intel OTC
>
> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Rao Shoaib]

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



On 04/08/2018 10:03 PM, Christoph Paasch wrote:
>> Please also remember our initial goal is to implement a very simple MPTCP
>> implementation and we are not trying to be on par with mptcp-dev.
> Yes, I couldn't agree more with this. We should do a simple and minimal
> implementation of MPTCP. Which is why in some of my comments I ask why a
> certain change is needed as we could simplify things.
That is great. Please submit patches that simplify MPTCP's interaction 
with TCP or at least creates a clean API. That is what I have tried to 
do but there is still a lot of work to be done.
>
>>> * The patches are holding the meta-lock when processing incoming packets or
>>>     timer-events. This will trigger a lot of RCU-warnings. Adressing this is
>>>     not trivial and will require significant architectural changes.
>>>
>> I do not want any significant architectural changes. I have not seen any RCU
>> warnings in my testing so I was not aware of that.
> You can simply enable LOCKDEP and then you will see plenty of them.
I have not addressed MPTCP  code yet. The patches that I have submitted 
for Peter will show that I have changed it a lot. I will try to test 
with LOCKDEP and see if I can resolve the issue.

Shoaib


[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 2099 bytes --]

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Christoph Paasch]

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

On 30/03/18 - 11:20:54, Rao Shoaib wrote:
> 
> 
> On 03/27/2018 03:37 AM, Christoph Paasch wrote:
> > Hello,
> > 
> > On 22/02/18 - 15:49:52, rao.shoaib(a)oracle.com wrote:
> > > From: Rao Shoaib <rao.shoaib(a)oracle.com>
> > > 
> > > Following patches modify TCP code to enable implementation of MPTCP. MPTCP implementation requires sharing of TCP code with minor modification here and there. In order to keep the TCP code clean and easy to maintain, common code has been moved to new functions for use by both TCP and MPTCP. struct tcp_sock now has function pointers and based on the socket type (TCP/MPTCP) appropriate function is called.
> > > 
> > > A basic implementation of MPTCP that works with IPv4/IPv6 and supports join has been tested based on these changes.
> > > 
> > > The changes are being submitted as an RFC to get feedback from the community and to start a discussion on how to move forward.
> > I did a full review of the patches.
> > 
> > Besides the details I commented on in the individual patches, I have a few
> > higher-level comments:
> > 
> > * The patchset introduces function-pointers to reduce the impact of MPTCP on
> >    the TCP-stack. Ignoring potential performance costs for a moment:
> > 
> >    The patchset should show why these function-pointers are needed for an
> >    MPTCP implementation. While reviewing the patches, it wasn't clear to me
> >    Although, I can guess it because I know the MPTCP-code. However, someone
> >    not familiar with MPTCP will have an even harder time to understand the
> >    reason for these function pointers.
> Your point is valid my bad.
> > 
> > * Another question that should be answered in the patchset is how these
> >    function pointers can be used by other TCP extensions (I am assuming that is
> >    the intention). The question that arises then is how could MPTCP be shared
> >    with these other TCP extensions?
> What extensions -- Give me an example.
> > 
> > * It should also be explained in the patchset how one switches the function
> >    pointers. It was unclear to me how MPTCP gets enabled.
> See the magic in tcp_init_sock. If unlikely(tp->op_ops->sock_init) is set
> that is where all the magic will happen. That is not part of this patch. If
> you want to see that check the MPTCP pages. This patch is only about
> refactoring TCP code so that MPTCP is less intrusive.
> > 
> >    In tcp_init_sock() tp->op_ops is currently set to tcp_default_op_ops.
> >    Thus, would that mean that MPTCP would get enabled on a system-level by
> >    changing tcp_default_op_ops?
> Kind of -- we simple change the pointer. MPTCP has to be compiled in and
> enabled, but eventually MPTCP will have an option to be either deployed
> system wide or just on selected sockets. That part is orthogonal and
> independent to the main TCP code that upstream is concerned about.
> 
> Please also remember our initial goal is to implement a very simple MPTCP
> implementation and we are not trying to be on par with mptcp-dev.

Yes, I couldn't agree more with this. We should do a simple and minimal
implementation of MPTCP. Which is why in some of my comments I ask why a
certain change is needed as we could simplify things.

> > 
> > * The patches are holding the meta-lock when processing incoming packets or
> >    timer-events. This will trigger a lot of RCU-warnings. Adressing this is
> >    not trivial and will require significant architectural changes.
> > 
> I do not want any significant architectural changes. I have not seen any RCU
> warnings in my testing so I was not aware of that.

You can simply enable LOCKDEP and then you will see plenty of them.


Christoph

>   As I have said
> previously I will deal with them later.
> 
> Thank you for your comments. If I have not answered any comment to your
> satisfaction please raise it again.
> 
> Shoaib.
> 

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Rao Shoaib]

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



On 03/30/2018 10:54 AM, Krystad, Peter wrote:
> Hi Rao -
>
> Review complete. I've supplied just a few comments on specific items.
>
> Overall I think the patchset shows a re-factoring of TCP that is more
> structured than the existing Linux implementations "changes all over
> TCP" approach (my apologies to the contributors) but does not really
> address any of that implementations other weaknesses or non-
> upstreamable nature.
>
> Without any MPTCP code or explanation to provide context to netdev
> maintainers it can't stand as a patchset to provide an overall view of
> what the MPTCP implementation will look like. It could provide material
>   to pose the question "is re-factoring TCP like this acceptable?".
And that is the whole point of this exercise, to ask upstream if we are 
on the right track. You mention "implementations other weaknesses". Can 
you be more specific. I will review your comments and see if that 
answers my question. Please also note that MPTCP code was not ready but 
I provided for completeness. I do not think upstream initially really 
cares about what happens in MPTCP.

Shoaib

>
> Regards,
>
> Peter.
>
>
> On Thu, 2018-02-22 at 15:49 -0800, rao.shoaib(a)oracle.com wrote:
>> From: Rao Shoaib <rao.shoaib(a)oracle.com>
>>
>> Following patches modify TCP code to enable implementation of MPTCP. MPTCP implementation requires sharing of TCP code with minor modification here and there. In order to keep the TCP code clean and easy to maintain, common code has been moved to new functions for use by both TCP and MPTCP. struct tcp_sock now has function pointers and based on the socket type (TCP/MPTCP) appropriate function is called.
>>
>> A basic implementation of MPTCP that works with IPv4/IPv6 and supports join has been tested based on these changes.
>>
>> The changes are being submitted as an RFC to get feedback from the community and to start a discussion on how to move forward.
>>
>> Rao Shoaib (9):
>>    Modify tcp structures to support function pointers
>>    Introduce MPTCP specific elements that will co-exist with TCP even
>>      when MPTCP is not compiled
>>    Introduce MPTCP specific elements that can be under #ifdef
>>      MPTCP_CONFIG
>>    Populate function pointers -- few (5) will be populated later
>>    Switch code to use function pointers
>>    Make TCP options processing abstract
>>    Restructure syncookie code to use pointers
>>    Restructure TCP code so that it can be shared primarily with MPTCP
>>    Add MPTCP specific code to core TCP code
>>
>>   crypto/md5.c                    |   3 -
>>   include/crypto/md5.h            |   2 +
>>   include/linux/tcp.h             |  91 ++++++++++++
>>   include/net/inet_common.h       |   2 +
>>   include/net/inet_sock.h         |   6 +-
>>   include/net/net_namespace.h     |   6 +
>>   include/net/secure_seq.h        |   9 +-
>>   include/net/sock.h              |   1 +
>>   include/net/tcp.h               | 321 ++++++++++++++++++++++++++++++++++++++--
>>   include/net/tcp_states.h        |   4 +-
>>   include/net/transp_v6.h         |   3 -
>>   include/uapi/linux/bpf.h        |   4 +-
>>   include/uapi/linux/if.h         |   5 +
>>   include/uapi/linux/tcp.h        |   1 +
>>   net/core/secure_seq.c           |  70 +++++++++
>>   net/ipv4/af_inet.c              |  16 +-
>>   net/ipv4/inet_connection_sock.c |  17 ++-
>>   net/ipv4/ip_sockglue.c          |  20 +++
>>   net/ipv4/syncookies.c           | 112 ++++++++++----
>>   net/ipv4/tcp.c                  | 221 +++++++++++++++++++++------
>>   net/ipv4/tcp_input.c            | 195 ++++++++++++++----------
>>   net/ipv4/tcp_ipv4.c             | 112 ++++++++++----
>>   net/ipv4/tcp_minisocks.c        |  56 ++++++-
>>   net/ipv4/tcp_output.c           | 206 +++++++++++++++-----------
>>   net/ipv4/tcp_timer.c            |  55 +++++--
>>   net/ipv6/af_inet6.c             |   4 +-
>>   net/ipv6/ipv6_sockglue.c        |  14 ++
>>   net/ipv6/syncookies.c           |  40 +----
>>   net/ipv6/tcp_ipv6.c             | 163 +++++++++++++-------
>>   29 files changed, 1360 insertions(+), 399 deletions(-)
>>
> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Rao Shoaib]

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



On 03/27/2018 03:37 AM, Christoph Paasch wrote:
> Hello,
>
> On 22/02/18 - 15:49:52, rao.shoaib(a)oracle.com wrote:
>> From: Rao Shoaib <rao.shoaib(a)oracle.com>
>>
>> Following patches modify TCP code to enable implementation of MPTCP. MPTCP implementation requires sharing of TCP code with minor modification here and there. In order to keep the TCP code clean and easy to maintain, common code has been moved to new functions for use by both TCP and MPTCP. struct tcp_sock now has function pointers and based on the socket type (TCP/MPTCP) appropriate function is called.
>>
>> A basic implementation of MPTCP that works with IPv4/IPv6 and supports join has been tested based on these changes.
>>
>> The changes are being submitted as an RFC to get feedback from the community and to start a discussion on how to move forward.
> I did a full review of the patches.
>
> Besides the details I commented on in the individual patches, I have a few
> higher-level comments:
>
> * The patchset introduces function-pointers to reduce the impact of MPTCP on
>    the TCP-stack. Ignoring potential performance costs for a moment:
>
>    The patchset should show why these function-pointers are needed for an
>    MPTCP implementation. While reviewing the patches, it wasn't clear to me
>    Although, I can guess it because I know the MPTCP-code. However, someone
>    not familiar with MPTCP will have an even harder time to understand the
>    reason for these function pointers.
Your point is valid my bad.
>
> * Another question that should be answered in the patchset is how these
>    function pointers can be used by other TCP extensions (I am assuming that is
>    the intention). The question that arises then is how could MPTCP be shared
>    with these other TCP extensions?
What extensions -- Give me an example.
>
> * It should also be explained in the patchset how one switches the function
>    pointers. It was unclear to me how MPTCP gets enabled.
See the magic in tcp_init_sock. If unlikely(tp->op_ops->sock_init) is 
set that is where all the magic will happen. That is not part of this 
patch. If you want to see that check the MPTCP pages. This patch is only 
about refactoring TCP code so that MPTCP is less intrusive.
>
>    In tcp_init_sock() tp->op_ops is currently set to tcp_default_op_ops.
>    Thus, would that mean that MPTCP would get enabled on a system-level by
>    changing tcp_default_op_ops?
Kind of -- we simple change the pointer. MPTCP has to be compiled in and 
enabled, but eventually MPTCP will have an option to be either deployed 
system wide or just on selected sockets. That part is orthogonal and 
independent to the main TCP code that upstream is concerned about.

Please also remember our initial goal is to implement a very simple 
MPTCP implementation and we are not trying to be on par with mptcp-dev.
>
> * The patches are holding the meta-lock when processing incoming packets or
>    timer-events. This will trigger a lot of RCU-warnings. Adressing this is
>    not trivial and will require significant architectural changes.
>
I do not want any significant architectural changes. I have not seen any 
RCU warnings in my testing so I was not aware of that.  As I have said 
previously I will deal with them later.

Thank you for your comments. If I have not answered any comment to your 
satisfaction please raise it again.

Shoaib.

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Krystad, Peter]

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


Hi Rao -

Review complete. I've supplied just a few comments on specific items.

Overall I think the patchset shows a re-factoring of TCP that is more
structured than the existing Linux implementations "changes all over
TCP" approach (my apologies to the contributors) but does not really
address any of that implementations other weaknesses or non-
upstreamable nature. 

Without any MPTCP code or explanation to provide context to netdev
maintainers it can't stand as a patchset to provide an overall view of
what the MPTCP implementation will look like. It could provide material
 to pose the question "is re-factoring TCP like this acceptable?".

Regards,

Peter.


On Thu, 2018-02-22 at 15:49 -0800, rao.shoaib(a)oracle.com wrote:
> From: Rao Shoaib <rao.shoaib(a)oracle.com>
> 
> Following patches modify TCP code to enable implementation of MPTCP. MPTCP implementation requires sharing of TCP code with minor modification here and there. In order to keep the TCP code clean and easy to maintain, common code has been moved to new functions for use by both TCP and MPTCP. struct tcp_sock now has function pointers and based on the socket type (TCP/MPTCP) appropriate function is called.
> 
> A basic implementation of MPTCP that works with IPv4/IPv6 and supports join has been tested based on these changes.
> 
> The changes are being submitted as an RFC to get feedback from the community and to start a discussion on how to move forward.
> 
> Rao Shoaib (9):
>   Modify tcp structures to support function pointers
>   Introduce MPTCP specific elements that will co-exist with TCP even
>     when MPTCP is not compiled
>   Introduce MPTCP specific elements that can be under #ifdef
>     MPTCP_CONFIG
>   Populate function pointers -- few (5) will be populated later
>   Switch code to use function pointers
>   Make TCP options processing abstract
>   Restructure syncookie code to use pointers
>   Restructure TCP code so that it can be shared primarily with MPTCP
>   Add MPTCP specific code to core TCP code
> 
>  crypto/md5.c                    |   3 -
>  include/crypto/md5.h            |   2 +
>  include/linux/tcp.h             |  91 ++++++++++++
>  include/net/inet_common.h       |   2 +
>  include/net/inet_sock.h         |   6 +-
>  include/net/net_namespace.h     |   6 +
>  include/net/secure_seq.h        |   9 +-
>  include/net/sock.h              |   1 +
>  include/net/tcp.h               | 321 ++++++++++++++++++++++++++++++++++++++--
>  include/net/tcp_states.h        |   4 +-
>  include/net/transp_v6.h         |   3 -
>  include/uapi/linux/bpf.h        |   4 +-
>  include/uapi/linux/if.h         |   5 +
>  include/uapi/linux/tcp.h        |   1 +
>  net/core/secure_seq.c           |  70 +++++++++
>  net/ipv4/af_inet.c              |  16 +-
>  net/ipv4/inet_connection_sock.c |  17 ++-
>  net/ipv4/ip_sockglue.c          |  20 +++
>  net/ipv4/syncookies.c           | 112 ++++++++++----
>  net/ipv4/tcp.c                  | 221 +++++++++++++++++++++------
>  net/ipv4/tcp_input.c            | 195 ++++++++++++++----------
>  net/ipv4/tcp_ipv4.c             | 112 ++++++++++----
>  net/ipv4/tcp_minisocks.c        |  56 ++++++-
>  net/ipv4/tcp_output.c           | 206 +++++++++++++++-----------
>  net/ipv4/tcp_timer.c            |  55 +++++--
>  net/ipv6/af_inet6.c             |   4 +-
>  net/ipv6/ipv6_sockglue.c        |  14 ++
>  net/ipv6/syncookies.c           |  40 +----
>  net/ipv6/tcp_ipv6.c             | 163 +++++++++++++-------
>  29 files changed, 1360 insertions(+), 399 deletions(-)
> 

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Christoph Paasch]

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

Hello,

On 22/02/18 - 15:49:52, rao.shoaib(a)oracle.com wrote:
> From: Rao Shoaib <rao.shoaib(a)oracle.com>
> 
> Following patches modify TCP code to enable implementation of MPTCP. MPTCP implementation requires sharing of TCP code with minor modification here and there. In order to keep the TCP code clean and easy to maintain, common code has been moved to new functions for use by both TCP and MPTCP. struct tcp_sock now has function pointers and based on the socket type (TCP/MPTCP) appropriate function is called.
> 
> A basic implementation of MPTCP that works with IPv4/IPv6 and supports join has been tested based on these changes.
> 
> The changes are being submitted as an RFC to get feedback from the community and to start a discussion on how to move forward.

I did a full review of the patches.

Besides the details I commented on in the individual patches, I have a few
higher-level comments:

* The patchset introduces function-pointers to reduce the impact of MPTCP on
  the TCP-stack. Ignoring potential performance costs for a moment:

  The patchset should show why these function-pointers are needed for an
  MPTCP implementation. While reviewing the patches, it wasn't clear to me
  Although, I can guess it because I know the MPTCP-code. However, someone
  not familiar with MPTCP will have an even harder time to understand the
  reason for these function pointers.

* Another question that should be answered in the patchset is how these
  function pointers can be used by other TCP extensions (I am assuming that is
  the intention). The question that arises then is how could MPTCP be shared
  with these other TCP extensions?

* It should also be explained in the patchset how one switches the function
  pointers. It was unclear to me how MPTCP gets enabled.

  In tcp_init_sock() tp->op_ops is currently set to tcp_default_op_ops.
  Thus, would that mean that MPTCP would get enabled on a system-level by
  changing tcp_default_op_ops?

* The patches are holding the meta-lock when processing incoming packets or
  timer-events. This will trigger a lot of RCU-warnings. Adressing this is
  not trivial and will require significant architectural changes.


Cheers,
Christoph

> 
> Rao Shoaib (9):
>   Modify tcp structures to support function pointers
>   Introduce MPTCP specific elements that will co-exist with TCP even
>     when MPTCP is not compiled
>   Introduce MPTCP specific elements that can be under #ifdef
>     MPTCP_CONFIG
>   Populate function pointers -- few (5) will be populated later
>   Switch code to use function pointers
>   Make TCP options processing abstract
>   Restructure syncookie code to use pointers
>   Restructure TCP code so that it can be shared primarily with MPTCP
>   Add MPTCP specific code to core TCP code
> 
>  crypto/md5.c                    |   3 -
>  include/crypto/md5.h            |   2 +
>  include/linux/tcp.h             |  91 ++++++++++++
>  include/net/inet_common.h       |   2 +
>  include/net/inet_sock.h         |   6 +-
>  include/net/net_namespace.h     |   6 +
>  include/net/secure_seq.h        |   9 +-
>  include/net/sock.h              |   1 +
>  include/net/tcp.h               | 321 ++++++++++++++++++++++++++++++++++++++--
>  include/net/tcp_states.h        |   4 +-
>  include/net/transp_v6.h         |   3 -
>  include/uapi/linux/bpf.h        |   4 +-
>  include/uapi/linux/if.h         |   5 +
>  include/uapi/linux/tcp.h        |   1 +
>  net/core/secure_seq.c           |  70 +++++++++
>  net/ipv4/af_inet.c              |  16 +-
>  net/ipv4/inet_connection_sock.c |  17 ++-
>  net/ipv4/ip_sockglue.c          |  20 +++
>  net/ipv4/syncookies.c           | 112 ++++++++++----
>  net/ipv4/tcp.c                  | 221 +++++++++++++++++++++------
>  net/ipv4/tcp_input.c            | 195 ++++++++++++++----------
>  net/ipv4/tcp_ipv4.c             | 112 ++++++++++----
>  net/ipv4/tcp_minisocks.c        |  56 ++++++-
>  net/ipv4/tcp_output.c           | 206 +++++++++++++++-----------
>  net/ipv4/tcp_timer.c            |  55 +++++--
>  net/ipv6/af_inet6.c             |   4 +-
>  net/ipv6/ipv6_sockglue.c        |  14 ++
>  net/ipv6/syncookies.c           |  40 +----
>  net/ipv6/tcp_ipv6.c             | 163 +++++++++++++-------
>  29 files changed, 1360 insertions(+), 399 deletions(-)
> 
> -- 
> 2.7.4
> 
> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Rao Shoaib]

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



On 03/01/2018 11:18 AM, Christoph Paasch wrote:
> Rao,
>
> On 28/02/18 - 14:57:08, Rao Shoaib wrote:
>> Hi Christoph,
>>
>> On 02/28/2018 02:32 PM, Christoph Paasch wrote:
>>>>> Hi Rao -
>>>>>
>>>>> I'd encourage you to closely read David Miller's response
>>>>> (https://www.spinics.net/lists/netdev/msg481868.html) to the TCP-MD5 RFC
>>>>> patch set that Christoph posted. We proposed some targeted indirect
>>>>> calls to callbacks for handling TCP options. He said "Sorry, I'm really
>>>>> not thrilled about this", and was focused on the performance
>>>>> implications of those callbacks. Have you looked at the performance and
>>>>> memory impact of adding indirection to a few dozen core TCP function
>>>>> calls?
>>>> Yes I have. There are several papers on this subject and the jury is out. In
>>>> the case of the frame work I have not looked at the code but Dave says
>>>>
>>>> "I can already see in your patches that new overhead is added, new
>>>> tests in the packet building fast paths that are completely
>>>> unnecessary with the existing code, etc."
>>>>
>>>> In my changes there are no new tests and fast is completely untouched. The
>>>> only change is to call functions via a pointer instead of having inline. The
>>>> impact of that has been a topic of discussion for a long time because of
>>>> c++.
>>> The problem is in patch 5/9. For example, the change in
>>> __tcp_push_pending_frames.
>>>
>>> Accessing the function-pointer makes two memory-accesses which potentially
>>> can have a cache-miss.
>>>
>>> That's the fast-path cost that upstream is worried about.
>> Anything can cause a cache miss, the code for the function may not be there.
>> If this is a fast path, the chances are very likely everything will be in
>> the CPU cache. I have to look at the options framework on which all these
>> comments are based on.
>>>>> To give some more background, when Christoph was crafting the MD5 patch
>>>>> set I pushed to take out some static-branch code that needed some work
>>>>> but eliminated the impact of the added callbacks for normal TCP. David
>>>>> focused on exactly that issue and didn't dive in to other aspects of the
>>>>> code. I can't fault him for that - he sorts through a staggering volume
>>>>> of patches every day, and if there's an issue that's clear at a glance
>>>>> it will pull all attention from the rest of the proposal. Also consider
>>>>> that Christoph replied with a few more questions, but the discussion
>>>>> stalled.
>>>>>
>>>>> I do have more to say about the proposed architecture and how to
>>>>> structure the patch set for netdev - but I think the maintainer feedback
>>>>> we already have about indirect callbacks is most critical to consider
>>>>> and factor in to what we send to netdev in the future. We have the
>>>>> shared goal to get MPTCP in to the upstream kernel and it's going to
>>>>> take a coordinated community to get there!
>>>> I agree that it will take a co-ordinated effort but we need to know what
>>>> upstream will accept. I have not seen any code or a detailed proposal.
>>>>
>>>> I will be sending out modified MPTCP code that works with the IP changes.
>>>>
>>>> What are the other issues besides the direct calls ?
>>> The list of function pointers is huge.
>>>
>>> If we post this on netdev, it won't cast a good light on the MPTCP
>>> upstreaming effort. DaveM wants the design to fit neatly into the TCP-stack
>>> with minimal/zero performance impact.
>> That is not exactly correct. When we talked to them they were willing to
>> accept some regression. Also I recently exchanged some email with them about
>> how to submit a patch in which I did mention I was using pointers, I did not
>> hear an concern. All they said was submit the patch and lets us look at the
>> code.
>>> We have to try to achieve that before we post on netdev.
>> Well how do we determine that ?
>>
>>>
>>> The problem is also that the patches don't provide any context to netdev
>>> as to why this is needed (e.g., why is a function-pointer for tcp_urg
>>> needed?). Without the context, it will be hard for people to provide
>>> feedback.
>> That is fine. Let the community reject the patch. I see no harm in asking
>> the community. I am ready for my patch to be rejected, but what if they say
>> yes fine ?
> The harm can be that they put MPTCP on a "mental spam-filter" because we are
> not taking their feedback into account.
>
>
> Christoph
>
What feedback -- I have read Dave's reply. How you concluded anything 
specific is besides me.

Rao

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Christoph Paasch]

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

Rao,

On 28/02/18 - 14:57:08, Rao Shoaib wrote:
> 
> Hi Christoph,
> 
> On 02/28/2018 02:32 PM, Christoph Paasch wrote:
> > 
> > > > Hi Rao -
> > > > 
> > > > I'd encourage you to closely read David Miller's response
> > > > (https://www.spinics.net/lists/netdev/msg481868.html) to the TCP-MD5 RFC
> > > > patch set that Christoph posted. We proposed some targeted indirect
> > > > calls to callbacks for handling TCP options. He said "Sorry, I'm really
> > > > not thrilled about this", and was focused on the performance
> > > > implications of those callbacks. Have you looked at the performance and
> > > > memory impact of adding indirection to a few dozen core TCP function
> > > > calls?
> > > Yes I have. There are several papers on this subject and the jury is out. In
> > > the case of the frame work I have not looked at the code but Dave says
> > > 
> > > "I can already see in your patches that new overhead is added, new
> > > tests in the packet building fast paths that are completely
> > > unnecessary with the existing code, etc."
> > > 
> > > In my changes there are no new tests and fast is completely untouched. The
> > > only change is to call functions via a pointer instead of having inline. The
> > > impact of that has been a topic of discussion for a long time because of
> > > c++.
> > The problem is in patch 5/9. For example, the change in
> > __tcp_push_pending_frames.
> > 
> > Accessing the function-pointer makes two memory-accesses which potentially
> > can have a cache-miss.
> > 
> > That's the fast-path cost that upstream is worried about.
> Anything can cause a cache miss, the code for the function may not be there.
> If this is a fast path, the chances are very likely everything will be in
> the CPU cache. I have to look at the options framework on which all these
> comments are based on.
> > 
> > > > To give some more background, when Christoph was crafting the MD5 patch
> > > > set I pushed to take out some static-branch code that needed some work
> > > > but eliminated the impact of the added callbacks for normal TCP. David
> > > > focused on exactly that issue and didn't dive in to other aspects of the
> > > > code. I can't fault him for that - he sorts through a staggering volume
> > > > of patches every day, and if there's an issue that's clear at a glance
> > > > it will pull all attention from the rest of the proposal. Also consider
> > > > that Christoph replied with a few more questions, but the discussion
> > > > stalled.
> > > > 
> > > > I do have more to say about the proposed architecture and how to
> > > > structure the patch set for netdev - but I think the maintainer feedback
> > > > we already have about indirect callbacks is most critical to consider
> > > > and factor in to what we send to netdev in the future. We have the
> > > > shared goal to get MPTCP in to the upstream kernel and it's going to
> > > > take a coordinated community to get there!
> > > I agree that it will take a co-ordinated effort but we need to know what
> > > upstream will accept. I have not seen any code or a detailed proposal.
> > > 
> > > I will be sending out modified MPTCP code that works with the IP changes.
> > > 
> > > What are the other issues besides the direct calls ?
> > The list of function pointers is huge.
> > 
> > If we post this on netdev, it won't cast a good light on the MPTCP
> > upstreaming effort. DaveM wants the design to fit neatly into the TCP-stack
> > with minimal/zero performance impact.
> That is not exactly correct. When we talked to them they were willing to
> accept some regression. Also I recently exchanged some email with them about
> how to submit a patch in which I did mention I was using pointers, I did not
> hear an concern. All they said was submit the patch and lets us look at the
> code.
> > 
> > We have to try to achieve that before we post on netdev.
> Well how do we determine that ?
> 
> > 
> > 
> > The problem is also that the patches don't provide any context to netdev
> > as to why this is needed (e.g., why is a function-pointer for tcp_urg
> > needed?). Without the context, it will be hard for people to provide
> > feedback.
> That is fine. Let the community reject the patch. I see no harm in asking
> the community. I am ready for my patch to be rejected, but what if they say
> yes fine ?

The harm can be that they put MPTCP on a "mental spam-filter" because we are
not taking their feedback into account.


Christoph

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Rao Shoaib]

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



On 02/28/2018 02:59 PM, Mat Martineau wrote:
> I have some more recommendations:
>
>  * Include the new Kconfig modification to add CONFIG_MPTCP early in 
> the patch set
>
>  * Make sure that all patches will build with the config option 
> enabled or disabled (all include files and symbols present, etc.)
>
>  * Handle conditional compilation according to 
> https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation 
>
>
>  * Don't define callbacks in the data structure until there's code 
> that uses them
>
>  * Craft commit messages and subject lines that will help the 
> reviewers understand the changes and what drives tham
>
>
> Even for RFC patch sets, the expectation is that thay will meet 
> submission guidelines:
>
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> https://www.kernel.org/doc/html/latest/process/coding-style.html
> https://www.kernel.org/doc/html/latest/process/submit-checklist.html
>
>
> I think you'll help MPTCP ultimately arrive upstream by putting some 
> more work in to how this TCP architecture proposal is presented to the 
> netdev maintainers.
>
>
Thanks Mat. I will try to cleanup as much as I can and post it upstream.

Shoaib.

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Mat Martineau]

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


On Wed, 28 Feb 2018, Christoph Paasch wrote:

> Hello Rao,
>
> On 28/02/18 - 14:01:09, Rao Shoaib wrote:
>> On 02/23/2018 12:20 PM, Mat Martineau wrote:
>>> On Thu, 22 Feb 2018, rao.shoaib(a)oracle.com wrote:
>>>> From: Rao Shoaib <rao.shoaib(a)oracle.com>
>>>>
>>>> Following patches modify TCP code to enable implementation of MPTCP.
>>>> MPTCP implementation requires sharing of TCP code with minor
>>>> modification here and there. In order to keep the TCP code clean and
>>>> easy to maintain, common code has been moved to new functions for
>>>> use by both TCP and MPTCP. struct tcp_sock now has function pointers
>>>> and based on the socket type (TCP/MPTCP) appropriate function is
>>>> called.
>>>>
>>>> A basic implementation of MPTCP that works with IPv4/IPv6 and
>>>> supports join has been tested based on these changes.
>>>>
>>>> The changes are being submitted as an RFC to get feedback from the
>>>> community and to start a discussion on how to move forward.
>>>>
>>>
>>> Hi Rao -
>>>
>>> I'd encourage you to closely read David Miller's response
>>> (https://www.spinics.net/lists/netdev/msg481868.html) to the TCP-MD5 RFC
>>> patch set that Christoph posted. We proposed some targeted indirect
>>> calls to callbacks for handling TCP options. He said "Sorry, I'm really
>>> not thrilled about this", and was focused on the performance
>>> implications of those callbacks. Have you looked at the performance and
>>> memory impact of adding indirection to a few dozen core TCP function
>>> calls?
>> Yes I have. There are several papers on this subject and the jury is out. In
>> the case of the frame work I have not looked at the code but Dave says
>>
>> "I can already see in your patches that new overhead is added, new
>> tests in the packet building fast paths that are completely
>> unnecessary with the existing code, etc."
>>
>> In my changes there are no new tests and fast is completely untouched. The
>> only change is to call functions via a pointer instead of having inline. The
>> impact of that has been a topic of discussion for a long time because of
>> c++.
>
> The problem is in patch 5/9. For example, the change in
> __tcp_push_pending_frames.
>
> Accessing the function-pointer makes two memory-accesses which potentially
> can have a cache-miss.
>
> That's the fast-path cost that upstream is worried about.
>
>>> To give some more background, when Christoph was crafting the MD5 patch
>>> set I pushed to take out some static-branch code that needed some work
>>> but eliminated the impact of the added callbacks for normal TCP. David
>>> focused on exactly that issue and didn't dive in to other aspects of the
>>> code. I can't fault him for that - he sorts through a staggering volume
>>> of patches every day, and if there's an issue that's clear at a glance
>>> it will pull all attention from the rest of the proposal. Also consider
>>> that Christoph replied with a few more questions, but the discussion
>>> stalled.
>>>
>>> I do have more to say about the proposed architecture and how to
>>> structure the patch set for netdev - but I think the maintainer feedback
>>> we already have about indirect callbacks is most critical to consider
>>> and factor in to what we send to netdev in the future. We have the
>>> shared goal to get MPTCP in to the upstream kernel and it's going to
>>> take a coordinated community to get there!
>> I agree that it will take a co-ordinated effort but we need to know what
>> upstream will accept. I have not seen any code or a detailed proposal.
>>
>> I will be sending out modified MPTCP code that works with the IP changes.
>>
>> What are the other issues besides the direct calls ?
>
> The list of function pointers is huge.
>
> If we post this on netdev, it won't cast a good light on the MPTCP
> upstreaming effort. DaveM wants the design to fit neatly into the TCP-stack
> with minimal/zero performance impact.
>
> We have to try to achieve that before we post on netdev.

Well said, Christoph.

> The problem is also that the patches don't provide any context to netdev
> as to why this is needed (e.g., why is a function-pointer for tcp_urg
> needed?). Without the context, it will be hard for people to provide
> feedback.

I have some more recommendations:

  * Include the new Kconfig modification to add CONFIG_MPTCP early in the 
patch set

  * Make sure that all patches will build with the config option enabled or 
disabled (all include files and symbols present, etc.)

  * Handle conditional compilation according to 
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation

  * Don't define callbacks in the data structure until there's code that 
uses them

  * Craft commit messages and subject lines that will help the reviewers 
understand the changes and what drives tham


Even for RFC patch sets, the expectation is that thay will meet 
submission guidelines:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html
https://www.kernel.org/doc/html/latest/process/coding-style.html
https://www.kernel.org/doc/html/latest/process/submit-checklist.html


I think you'll help MPTCP ultimately arrive upstream by putting some more 
work in to how this TCP architecture proposal is presented to the netdev 
maintainers.


Regards,

--
Mat Martineau
Intel OTC

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Rao Shoaib]

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


Hi Christoph,

On 02/28/2018 02:32 PM, Christoph Paasch wrote:
>
>>> Hi Rao -
>>>
>>> I'd encourage you to closely read David Miller's response
>>> (https://www.spinics.net/lists/netdev/msg481868.html) to the TCP-MD5 RFC
>>> patch set that Christoph posted. We proposed some targeted indirect
>>> calls to callbacks for handling TCP options. He said "Sorry, I'm really
>>> not thrilled about this", and was focused on the performance
>>> implications of those callbacks. Have you looked at the performance and
>>> memory impact of adding indirection to a few dozen core TCP function
>>> calls?
>> Yes I have. There are several papers on this subject and the jury is out. In
>> the case of the frame work I have not looked at the code but Dave says
>>
>> "I can already see in your patches that new overhead is added, new
>> tests in the packet building fast paths that are completely
>> unnecessary with the existing code, etc."
>>
>> In my changes there are no new tests and fast is completely untouched. The
>> only change is to call functions via a pointer instead of having inline. The
>> impact of that has been a topic of discussion for a long time because of
>> c++.
> The problem is in patch 5/9. For example, the change in
> __tcp_push_pending_frames.
>
> Accessing the function-pointer makes two memory-accesses which potentially
> can have a cache-miss.
>
> That's the fast-path cost that upstream is worried about.
Anything can cause a cache miss, the code for the function may not be 
there. If this is a fast path, the chances are very likely everything 
will be in the CPU cache. I have to look at the options framework on 
which all these comments are based on.
>
>>> To give some more background, when Christoph was crafting the MD5 patch
>>> set I pushed to take out some static-branch code that needed some work
>>> but eliminated the impact of the added callbacks for normal TCP. David
>>> focused on exactly that issue and didn't dive in to other aspects of the
>>> code. I can't fault him for that - he sorts through a staggering volume
>>> of patches every day, and if there's an issue that's clear at a glance
>>> it will pull all attention from the rest of the proposal. Also consider
>>> that Christoph replied with a few more questions, but the discussion
>>> stalled.
>>>
>>> I do have more to say about the proposed architecture and how to
>>> structure the patch set for netdev - but I think the maintainer feedback
>>> we already have about indirect callbacks is most critical to consider
>>> and factor in to what we send to netdev in the future. We have the
>>> shared goal to get MPTCP in to the upstream kernel and it's going to
>>> take a coordinated community to get there!
>> I agree that it will take a co-ordinated effort but we need to know what
>> upstream will accept. I have not seen any code or a detailed proposal.
>>
>> I will be sending out modified MPTCP code that works with the IP changes.
>>
>> What are the other issues besides the direct calls ?
> The list of function pointers is huge.
>
> If we post this on netdev, it won't cast a good light on the MPTCP
> upstreaming effort. DaveM wants the design to fit neatly into the TCP-stack
> with minimal/zero performance impact.
That is not exactly correct. When we talked to them they were willing to 
accept some regression. Also I recently exchanged some email with them 
about how to submit a patch in which I did mention I was using pointers, 
I did not hear an concern. All they said was submit the patch and lets 
us look at the code.
>
> We have to try to achieve that before we post on netdev.
Well how do we determine that ?

>
>
> The problem is also that the patches don't provide any context to netdev
> as to why this is needed (e.g., why is a function-pointer for tcp_urg
> needed?). Without the context, it will be hard for people to provide
> feedback.
That is fine. Let the community reject the patch. I see no harm in 
asking the community. I am ready for my patch to be rejected, but what 
if they say yes fine ?

Shoaib
>
>
> Christoph
>

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Christoph Paasch]

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

Hello Rao,

On 28/02/18 - 14:01:09, Rao Shoaib wrote:
> On 02/23/2018 12:20 PM, Mat Martineau wrote:
> > On Thu, 22 Feb 2018, rao.shoaib(a)oracle.com wrote:
> > > From: Rao Shoaib <rao.shoaib(a)oracle.com>
> > > 
> > > Following patches modify TCP code to enable implementation of MPTCP.
> > > MPTCP implementation requires sharing of TCP code with minor
> > > modification here and there. In order to keep the TCP code clean and
> > > easy to maintain, common code has been moved to new functions for
> > > use by both TCP and MPTCP. struct tcp_sock now has function pointers
> > > and based on the socket type (TCP/MPTCP) appropriate function is
> > > called.
> > > 
> > > A basic implementation of MPTCP that works with IPv4/IPv6 and
> > > supports join has been tested based on these changes.
> > > 
> > > The changes are being submitted as an RFC to get feedback from the
> > > community and to start a discussion on how to move forward.
> > > 
> > 
> > Hi Rao -
> > 
> > I'd encourage you to closely read David Miller's response
> > (https://www.spinics.net/lists/netdev/msg481868.html) to the TCP-MD5 RFC
> > patch set that Christoph posted. We proposed some targeted indirect
> > calls to callbacks for handling TCP options. He said "Sorry, I'm really
> > not thrilled about this", and was focused on the performance
> > implications of those callbacks. Have you looked at the performance and
> > memory impact of adding indirection to a few dozen core TCP function
> > calls?
> Yes I have. There are several papers on this subject and the jury is out. In
> the case of the frame work I have not looked at the code but Dave says
> 
> "I can already see in your patches that new overhead is added, new
> tests in the packet building fast paths that are completely
> unnecessary with the existing code, etc."
> 
> In my changes there are no new tests and fast is completely untouched. The
> only change is to call functions via a pointer instead of having inline. The
> impact of that has been a topic of discussion for a long time because of
> c++.

The problem is in patch 5/9. For example, the change in
__tcp_push_pending_frames.

Accessing the function-pointer makes two memory-accesses which potentially
can have a cache-miss.

That's the fast-path cost that upstream is worried about.

> > To give some more background, when Christoph was crafting the MD5 patch
> > set I pushed to take out some static-branch code that needed some work
> > but eliminated the impact of the added callbacks for normal TCP. David
> > focused on exactly that issue and didn't dive in to other aspects of the
> > code. I can't fault him for that - he sorts through a staggering volume
> > of patches every day, and if there's an issue that's clear at a glance
> > it will pull all attention from the rest of the proposal. Also consider
> > that Christoph replied with a few more questions, but the discussion
> > stalled.
> > 
> > I do have more to say about the proposed architecture and how to
> > structure the patch set for netdev - but I think the maintainer feedback
> > we already have about indirect callbacks is most critical to consider
> > and factor in to what we send to netdev in the future. We have the
> > shared goal to get MPTCP in to the upstream kernel and it's going to
> > take a coordinated community to get there!
> I agree that it will take a co-ordinated effort but we need to know what
> upstream will accept. I have not seen any code or a detailed proposal.
> 
> I will be sending out modified MPTCP code that works with the IP changes.
> 
> What are the other issues besides the direct calls ?

The list of function pointers is huge.

If we post this on netdev, it won't cast a good light on the MPTCP
upstreaming effort. DaveM wants the design to fit neatly into the TCP-stack
with minimal/zero performance impact.

We have to try to achieve that before we post on netdev.


The problem is also that the patches don't provide any context to netdev
as to why this is needed (e.g., why is a function-pointer for tcp_urg
needed?). Without the context, it will be hard for people to provide
feedback.


Christoph

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Rao Shoaib]

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



On 02/23/2018 12:20 PM, Mat Martineau wrote:
>
> On Thu, 22 Feb 2018, rao.shoaib(a)oracle.com wrote:
>
>> From: Rao Shoaib <rao.shoaib(a)oracle.com>
>>
>> Following patches modify TCP code to enable implementation of MPTCP. 
>> MPTCP implementation requires sharing of TCP code with minor 
>> modification here and there. In order to keep the TCP code clean and 
>> easy to maintain, common code has been moved to new functions for use 
>> by both TCP and MPTCP. struct tcp_sock now has function pointers and 
>> based on the socket type (TCP/MPTCP) appropriate function is called.
>>
>> A basic implementation of MPTCP that works with IPv4/IPv6 and 
>> supports join has been tested based on these changes.
>>
>> The changes are being submitted as an RFC to get feedback from the 
>> community and to start a discussion on how to move forward.
>>
>
> Hi Rao -
>
> I'd encourage you to closely read David Miller's response 
> (https://www.spinics.net/lists/netdev/msg481868.html) to the TCP-MD5 
> RFC patch set that Christoph posted. We proposed some targeted 
> indirect calls to callbacks for handling TCP options. He said "Sorry, 
> I'm really not thrilled about this", and was focused on the 
> performance implications of those callbacks. Have you looked at the 
> performance and memory impact of adding indirection to a few dozen 
> core TCP function calls?
Yes I have. There are several papers on this subject and the jury is 
out. In the case of the frame work I have not looked at the code but 
Dave says

"I can already see in your patches that new overhead is added, new
tests in the packet building fast paths that are completely
unnecessary with the existing code, etc."

In my changes there are no new tests and fast is completely untouched. 
The only change is to call functions via a pointer instead of having 
inline. The impact of that has been a topic of discussion for a long 
time because of c++.

>
> To give some more background, when Christoph was crafting the MD5 
> patch set I pushed to take out some static-branch code that needed 
> some work but eliminated the impact of the added callbacks for normal 
> TCP. David focused on exactly that issue and didn't dive in to other 
> aspects of the code. I can't fault him for that - he sorts through a 
> staggering volume of patches every day, and if there's an issue that's 
> clear at a glance it will pull all attention from the rest of the 
> proposal. Also consider that Christoph replied with a few more 
> questions, but the discussion stalled.
>
> I do have more to say about the proposed architecture and how to 
> structure the patch set for netdev - but I think the maintainer 
> feedback we already have about indirect callbacks is most critical to 
> consider and factor in to what we send to netdev in the future. We 
> have the shared goal to get MPTCP in to the upstream kernel and it's 
> going to take a coordinated community to get there!
I agree that it will take a co-ordinated effort but we need to know what 
upstream will accept. I have not seen any code or a detailed proposal.

I will be sending out modified MPTCP code that works with the IP changes.

What are the other issues besides the direct calls ?

Rao.

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

Re: [MPTCP] [RFC 0/9] Changes for implementing MPTCP

[Mat Martineau]

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


On Thu, 22 Feb 2018, rao.shoaib(a)oracle.com wrote:

> From: Rao Shoaib <rao.shoaib(a)oracle.com>
>
> Following patches modify TCP code to enable implementation of MPTCP. 
> MPTCP implementation requires sharing of TCP code with minor 
> modification here and there. In order to keep the TCP code clean and 
> easy to maintain, common code has been moved to new functions for use by 
> both TCP and MPTCP. struct tcp_sock now has function pointers and based 
> on the socket type (TCP/MPTCP) appropriate function is called.
>
> A basic implementation of MPTCP that works with IPv4/IPv6 and supports 
> join has been tested based on these changes.
>
> The changes are being submitted as an RFC to get feedback from the 
> community and to start a discussion on how to move forward.
>

Hi Rao -

I'd encourage you to closely read David Miller's response 
(https://www.spinics.net/lists/netdev/msg481868.html) to the TCP-MD5 RFC 
patch set that Christoph posted. We proposed some targeted indirect calls 
to callbacks for handling TCP options. He said "Sorry, I'm really not 
thrilled about this", and was focused on the performance implications of 
those callbacks. Have you looked at the performance and memory impact of 
adding indirection to a few dozen core TCP function calls?

To give some more background, when Christoph was crafting the MD5 patch 
set I pushed to take out some static-branch code that needed some work but 
eliminated the impact of the added callbacks for normal TCP. David focused 
on exactly that issue and didn't dive in to other aspects of the code. I 
can't fault him for that - he sorts through a staggering volume of patches 
every day, and if there's an issue that's clear at a glance it will pull 
all attention from the rest of the proposal. Also consider that Christoph 
replied with a few more questions, but the discussion stalled.

I do have more to say about the proposed architecture and how to structure 
the patch set for netdev - but I think the maintainer feedback we already 
have about indirect callbacks is most critical to consider and factor in 
to what we send to netdev in the future. We have the shared goal to get 
MPTCP in to the upstream kernel and it's going to take a coordinated 
community to get there!


Thanks,
Mat

--
Mat Martineau
Intel OTC

[MPTCP] [RFC 0/9] Changes for implementing MPTCP

[rao.shoaib]

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

From: Rao Shoaib <rao.shoaib(a)oracle.com>

Following patches modify TCP code to enable implementation of MPTCP. MPTCP implementation requires sharing of TCP code with minor modification here and there. In order to keep the TCP code clean and easy to maintain, common code has been moved to new functions for use by both TCP and MPTCP. struct tcp_sock now has function pointers and based on the socket type (TCP/MPTCP) appropriate function is called.

A basic implementation of MPTCP that works with IPv4/IPv6 and supports join has been tested based on these changes.

The changes are being submitted as an RFC to get feedback from the community and to start a discussion on how to move forward.

Rao Shoaib (9):
  Modify tcp structures to support function pointers
  Introduce MPTCP specific elements that will co-exist with TCP even
    when MPTCP is not compiled
  Introduce MPTCP specific elements that can be under #ifdef
    MPTCP_CONFIG
  Populate function pointers -- few (5) will be populated later
  Switch code to use function pointers
  Make TCP options processing abstract
  Restructure syncookie code to use pointers
  Restructure TCP code so that it can be shared primarily with MPTCP
  Add MPTCP specific code to core TCP code

 crypto/md5.c                    |   3 -
 include/crypto/md5.h            |   2 +
 include/linux/tcp.h             |  91 ++++++++++++
 include/net/inet_common.h       |   2 +
 include/net/inet_sock.h         |   6 +-
 include/net/net_namespace.h     |   6 +
 include/net/secure_seq.h        |   9 +-
 include/net/sock.h              |   1 +
 include/net/tcp.h               | 321 ++++++++++++++++++++++++++++++++++++++--
 include/net/tcp_states.h        |   4 +-
 include/net/transp_v6.h         |   3 -
 include/uapi/linux/bpf.h        |   4 +-
 include/uapi/linux/if.h         |   5 +
 include/uapi/linux/tcp.h        |   1 +
 net/core/secure_seq.c           |  70 +++++++++
 net/ipv4/af_inet.c              |  16 +-
 net/ipv4/inet_connection_sock.c |  17 ++-
 net/ipv4/ip_sockglue.c          |  20 +++
 net/ipv4/syncookies.c           | 112 ++++++++++----
 net/ipv4/tcp.c                  | 221 +++++++++++++++++++++------
 net/ipv4/tcp_input.c            | 195 ++++++++++++++----------
 net/ipv4/tcp_ipv4.c             | 112 ++++++++++----
 net/ipv4/tcp_minisocks.c        |  56 ++++++-
 net/ipv4/tcp_output.c           | 206 +++++++++++++++-----------
 net/ipv4/tcp_timer.c            |  55 +++++--
 net/ipv6/af_inet6.c             |   4 +-
 net/ipv6/ipv6_sockglue.c        |  14 ++
 net/ipv6/syncookies.c           |  40 +----
 net/ipv6/tcp_ipv6.c             | 163 +++++++++++++-------
 29 files changed, 1360 insertions(+), 399 deletions(-)

-- 
2.7.4