[MPTCP] Re: [PATCH v2] mptcp: Basic single-subflow DATA_FIN

* [MPTCP] Re: [PATCH v2] mptcp: Basic single-subflow DATA_FIN
@ 2019-11-19 18:35 Mat Martineau
  0 siblings, 0 replies; 2+ messages in thread
From: Mat Martineau @ 2019-11-19 18:35 UTC (permalink / raw)
  To: mptcp

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

On Tue, 19 Nov 2019, Paolo Abeni wrote:

> Hi,
>
> I'm sorry for lagging behind with the reviews...
>
> On Mon, 2019-11-18 at 16:31 -0800, Mat Martineau wrote:
>> @@ -405,6 +435,10 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
>>  			dss_size = map_size;
>>  			if (mpext)
>>  				opts->ext_copy = *mpext;
>> +
>> +			if (skb && tcp_fin &&
>> +			    subflow->conn->sk_state != TCP_ESTABLISHED)
>> +				mptcp_write_data_fin(subflow, &opts->ext_copy);
>>  		} else {
>>  			opts->ext_copy.use_map = 0;
>>  			WARN_ONCE(1, "MPTCP: Map dropped");
>
> Don't we need some additional check to avoid sending multiple DATA_FIN
> on all subflows ?!? e.g. fiddling with some bit status bit on msk.

Sending DATA_FIN on multiple subflows is deliberate - what if one of them 
is not behaving well?

I ran a test between two multipath-tcp.org v0.95 kernels (VM A: 2 
interfaces, VM B: 1 interface, "fullmesh" path manager on both), and it 
sent DATA_FIN only on one subflow. Sending on multiple subflows should be 
fine, right? The first DATA_FIN received will modify the MPTCP connection 
state (when that's implemented) so duplicate DATA_FINs can be ignored.

>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 1a432abfb176..910cf26037b7 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -1284,6 +1284,10 @@ static int mptcp_shutdown(struct socket *sock, int how)
>>  	pr_debug("sk=%p, how=%d", msk, how);
>>
>>  	lock_sock(sock->sk);
>> +
>> +	if (how == SHUT_WR || how == SHUT_RDWR)
>> +		inet_sk_state_store(sock->sk, TCP_FIN_WAIT1);
>> +
>>  	ssock = __mptcp_fallback_get_ref(msk);
>>  	if (ssock) {
>>  		release_sock(sock->sk);
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index ff38d54392cd..89e6533c97b6 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -422,6 +422,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk)
>>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>>  	struct mptcp_ext *mpext;
>>  	struct sk_buff *skb;
>> +	u16 data_len;
>>  	u64 map_seq;
>>
>>  	skb = skb_peek(&ssk->sk_receive_queue);
>> @@ -446,26 +447,39 @@ static enum mapping_status get_mapping_status(struct sock *ssk)
>>
>>  		if (!subflow->map_valid)
>>  			return MAPPING_INVALID;
>> +
>>  		goto validate_seq;
>>  	}
>>
>> -	pr_debug("seq=%llu is64=%d ssn=%u data_len=%u", mpext->data_seq,
>> -		 mpext->dsn64, mpext->subflow_seq, mpext->data_len);
>> +	pr_debug("seq=%llu is64=%d ssn=%u data_len=%u data_fin=%d",
>> +		 mpext->data_seq, mpext->dsn64, mpext->subflow_seq,
>> +		 mpext->data_len, mpext->data_fin);
>>
>> -	if (mpext->data_len == 0) {
>> +	data_len = mpext->data_len;
>> +	if (data_len == 0) {
>>  		pr_err("Infinite mapping not handled");
>>  		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX);
>>  		return MAPPING_INVALID;
>> -	} else if (mpext->subflow_seq == 0 &&
>> -		   mpext->data_fin == 1) {
>> -		if (WARN_ON_ONCE(mpext->data_len != 1))
>> -			return false;
>> +	}
>>
>> -		/* do not try hard to handle this any better, till we have
>> -		 * real data_fin support
>> -		 */
>> -		pr_debug("DATA_FIN with no payload");
>> -		return MAPPING_DATA_FIN;
>> +	if (mpext->data_fin == 1) {
>> +		if (data_len == 1) {
>> +			pr_debug("DATA_FIN with no payload");
>> +			if (subflow->map_valid) {
>> +				/* A DATA_FIN might arrive in a DSS
>> +				 * option before the previous mapping
>> +				 * has been fully consumed. Continue
>> +				 * handling the existing mapping.
>> +				 */
>> +				skb_ext_del(skb, SKB_EXT_MPTCP);
>> +				return MAPPING_OK;
>> +			} else {
>> +				return MAPPING_DATA_FIN;
>> +			}
>
> Can we instead:
> 	data_len--;
> 	WRITE_ONCE(msk->data_fin_set, 1);
> 	WRITE_ONCE(msk->data_fin_seq, mpext->data_seq + data_len);
>
> and than in mptcp_recvmsg():
>
> 	if (READ_ONCE(msk->data_fin_seq) &&
> 	    msk->ack_seq == msk->data_fin_seq)
> 		msk->ack_seq++;
>
> (or something along these lines)
>
> ?
>

In an earlier thread I mentioned I would get the DATA_FIN sending part 
finished first, then approach the ack as a second step. As you point out, 
it may be a small step!

--
Mat Martineau
Intel

^ permalink raw reply	[flat|nested] 2+ messages in thread