[MPTCP] Re: [RFC net] mptcp: Initialize map_seq upon subflow establishment

[MPTCP] Re: [RFC net] mptcp: Initialize map_seq upon subflow establishment

[Christoph Paasch]

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

On 11/05/20 - 10:30:51, Paolo Abeni wrote:
> On Sat, 2020-05-09 at 16:39 -0700, Christoph Paasch wrote:
> > When the other MPTCP-peer uses 32-bit data-sequence numbers, we rely on
> > map_seq to indicate how to expand to a 64-bit data-sequence number in
> > expand_seq() when receiving data.
> > 
> > For new subflows, this field is not initialized, thus results in an
> > "invalid" mapping being discarded.
> > 
> > Fix this by initializing map_seq upon subflow establishment time.
> > 
> > Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
> > Signed-off-by: Christoph Paasch <cpaasch(a)apple.com>
> > ---
> >  net/mptcp/protocol.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index e1f23016ed3f..86ed1e55a59b 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1635,6 +1635,8 @@ bool mptcp_finish_join(struct sock *sk)
> >  			list_add_tail(&subflow->node, &msk->join_list);
> >  		spin_unlock_bh(&msk->join_list_lock);
> >  	}
> > +
> > +	subflow->map_seq = msk->ack_seq;
> >  	return ret;
> >  }
> >  
> 
> The patch LGTM, thanks! 
> 
> Perhaps we can move the assignment under the 'if (ret) {' conditional,
> just for clarity?

Yes, I can move it there.

> May I guess the above is the root cause for the retransmissions we get
> while interopertating with mptcp.org? Is that all, or there are still
> retransmissions?

Yes, that was all there is. Because data didn't get acknowledged on the
other subflows, thus MPTCP retransmitted until it tried the initial subflow, where
it finally was being acknowledged.

My next problem now is ADD_ADDR-format that is incorrect in out-of-tree.



On a different note - while digging through the code I found the naming a
bit surprising. msk->ack_seq is basically a "TCP rcv_nxt" variable. Same for
the subflow's map_seq is the subflow's view of "MPTCP-level rcv_nxt". Right?

Would the code be easier to understand if it would be called
msk->rcv_nxt and subflow->rcv_nxt_dsn ?

Not really important or urgent - just wondering if it makes sense to make
MPTCP's data-processing more look TCP-like to make it easier to follow as the
concepts of Sequence-numbers and ACK-numbers are the same in MPTCP and TCP.


Christoph

[MPTCP] Re: [RFC net] mptcp: Initialize map_seq upon subflow establishment

[Paolo Abeni]

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

On Sat, 2020-05-09 at 16:39 -0700, Christoph Paasch wrote:
> When the other MPTCP-peer uses 32-bit data-sequence numbers, we rely on
> map_seq to indicate how to expand to a 64-bit data-sequence number in
> expand_seq() when receiving data.
> 
> For new subflows, this field is not initialized, thus results in an
> "invalid" mapping being discarded.
> 
> Fix this by initializing map_seq upon subflow establishment time.
> 
> Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
> Signed-off-by: Christoph Paasch <cpaasch(a)apple.com>
> ---
>  net/mptcp/protocol.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e1f23016ed3f..86ed1e55a59b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1635,6 +1635,8 @@ bool mptcp_finish_join(struct sock *sk)
>  			list_add_tail(&subflow->node, &msk->join_list);
>  		spin_unlock_bh(&msk->join_list_lock);
>  	}
> +
> +	subflow->map_seq = msk->ack_seq;
>  	return ret;
>  }
>  

The patch LGTM, thanks! 

Perhaps we can move the assignment under the 'if (ret) {' conditional,
just for clarity?

May I guess the above is the root cause for the retransmissions we get
while interopertating with mptcp.org? Is that all, or there are still
retransmissions?

Thanks

Paolo