On 04/30/2018 09:34 AM, Christoph Paasch wrote:
On 26/04/18 - 17:43:44, Rao Shoaib wrote:


On 04/26/2018 03:20 PM, Christoph Paasch wrote:

On 26/04/18 - 14:25:24, Rao Shoaib wrote:

On 04/26/2018 01:52 PM, Christoph Paasch wrote:

This kind of lock-taking also causes trouble with RCU LOCKDEP debugging - as
I mentioned in a previous e-mail.

I have not looked into it so I can not comment. I am sure we will find a way
to address it.

And beyond that, it requires that everytime a TCP-change is being done, one
needs to take MPTCP into account. E.g., when upstream added the SOCK_DESTROY
interface (and Samsung backported it to v4.4), there were panics on Android
devices (https://github.com/multipath-tcp/mptcp/issues/170).

Avoid taking the meta-socket lock on subflow-work allows for much easier
maintenance in the long-term.

The bug that you have pointed out is a run of the mill bug that we see
everyday, there is nothing special about it.

Euh, you mean the issue #170 is happening frequently?

It shouldn't! If it does, that's a big concern.

No we have not encountered the bug. What I meant to say was that when
changes or enhancements are made to the code it is quite common that
something breaks.

This is exactly what should be avoided. See my below comment on
maintainability.


That is the case in the bug that you pointed.


          

            
Taking meta lock actually simplifies things a lot and has reduced
maintainable cost, not taking the meta lock will create timing issues left
and right. So we are just trading one headache with another.

Holding meta lock should not be piped up unnecessarily. Yes, if possible if
should be changed, plus our patch has only 6 cases in TCP.


          

          
I think, you are taking the MPTCP point-of-view here. Yes, taking the
meta-level lock does reduce maintainability of MPTCP (which is why we did it
that way in the first place ;-))

However, for upstreaming we have to think the other way around. TCP is the
common-case, and MPTCP is the exotic corner-case only few care about.

So, TCP-maintainability is of outmost importance. Way more important than
MPTCP's.


        

        
I agree that TCP stability and maintainability is of extreme importantance.
However, this code causes no instability or maintainability issues to TCP
code. Consider the bug that you pointed out, MPTCP fails not TCP. We have to
accept the fact that TCP changes will break  MPTCP no matter what the
implementation is because TCP maintainers may or may not test MPTCP. So this
is nothing out of the extraordinary.
Our patch tries to maintain the stability of TCP. We have reduced MPTCP code
within TCP and anything that is left is under #ifdef MPTCP so that TCP
developers can enhance, build and test TCP without worrying about MPTCP. Not
only that, we have tried to limit any performance impact to pure TCP when
MPTCP is enabled.


      

      
The point-of-view you take here is that introducing bugs in MPTCP is more
acceptable. Unfortunately, that's not the case once the code is upstream.
Because, the upstream maintainers are responsible that the code is stable
and has no security holes.

    

    No, that is not what I said, it is how you interpreted it. I said

    

    "implementation is because TCP maintainers may or may not test
      MPTCP. So this is nothing out of the extraordinary."

    

    Maybe maintainers should be "submitters".

    

    If the submission rules require that all features should be tested,
    MPTCP will be tested and MPTCP will be just fine.

    It is very common to see a change break a feature, such breakage is
    caught during review or testing and in some cases after the release.
    There is nothing special here. Following are some examples on how
    things get broken/caught/fixed.

    

    https://www.spinics.net/lists/netdev/msg470389.html

    https://www.spinics.net/lists/netdev/msg474372.html

    https://www.spinics.net/lists/netdev/msg469607.html

    https://marc.info/?l=linux-netdev&m=152453932929411&w=2

    https://marc.info/?l=linux-netdev&m=152345771702029&w=2

    

      
So, the MPTCP code should be designed in such a way that it does not require
extensive MPTCP-testing for simple TCP-changes. That's what I mean with TCP
maintainability is of high importance.
#ifdefs don't protect against any such issues as distributions typically
enable all kernel-configs.

    

    Simple is subjective. If there is an MPTCP test suite, running it is
    the most easiest way to ensure MPTCP is not broken.

    However a locking change is not simple with or without MPTCP and
    even a test suite may not expose the bug.

    

    This is again coming down to the usual nit picking and that I know
    what the correct thing to do is.

    

    I prefer upstream make the call, however that would require sharing
    the code and asking.

    

    Shoaib

    

    

      


Christoph




      

        
I really do not see any maintainability or stability issues for TCP. TCP
will work and behave as it does today without MPTCP.

Rao.



        

          
Christoph



          

            
Shoaib.

[1] If MPTCP is built on top of TCP, any change in TCP will always have to
worry about MPTCP particularly in the control path. There is a overhead of
adding MPTCP on TCP and no one can argue against it.



            

              
Christoph