diff mbox series

[mptcp-next,v2,3/9] mptcp: add MAPPING_INFINITE mapping status

Message ID 592427b5a91d5e64e4b96c4c8b8d06264197f1c4.1631188109.git.geliangtang@xiaomi.com (mailing list archive)
State Superseded, archived
Headers show
Series The infinite mapping support | expand

Commit Message

Geliang Tang Sept. 9, 2021, 11:51 a.m. UTC 
From: Geliang Tang <geliangtang@xiaomi.com>

This patch added a new mapping status named MAPPING_INFINITE. If the
MPTCP_INFINITE_DONE flag is set in get_mapping_status, return this new
status. And in subflow_check_data_avail, if this status is set, goto the
'infinite' lable to fallback.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 net/mptcp/subflow.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Paolo Abeni Sept. 9, 2021, 1:39 p.m. UTC | #1 
On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> This patch added a new mapping status named MAPPING_INFINITE. If the
> MPTCP_INFINITE_DONE flag is set in get_mapping_status, return this new
> status. And in subflow_check_data_avail, if this status is set, goto the
> 'infinite' lable to fallback.
> 
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  net/mptcp/subflow.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 951aafb6021e..ad8efe56eab6 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -798,6 +798,7 @@ enum mapping_status {
>  	MAPPING_INVALID,
>  	MAPPING_EMPTY,
>  	MAPPING_DATA_FIN,
> +	MAPPING_INFINITE,
>  	MAPPING_DUMMY
>  };
>  
> @@ -938,6 +939,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
>  	if (!skb)
>  		return MAPPING_EMPTY;
>  
> +	if (mptcp_check_infinite(ssk))
> +		return MAPPING_INFINITE;
> +
>  	if (mptcp_check_fallback(ssk))
>  		return MAPPING_DUMMY;
>  
> @@ -1121,6 +1125,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  
>  		status = get_mapping_status(ssk, msk);
>  		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
> +		if (unlikely(status == MAPPING_INFINITE))
> +			goto infinite;
> +
>  		if (unlikely(status == MAPPING_INVALID))
>  			goto fallback;
>  
> @@ -1192,6 +1199,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  		return false;
>  	}
>  
> +infinite:
>  	__mptcp_do_fallback(msk);
>  	skb = skb_peek(&ssk->sk_receive_queue);
>  	subflow->map_valid = 1;


It looks like MAPPING_INFINITE has almost the same behavior
of MAPPING_DUMMY.

I think we can avoid the new conditional in get_mapping_status().
Eventually we could do all the error checking after the 'fallback:'
label only if the msk has not fallen back yet:

fallback:
	if (!__mptcp_check_fallback(msk)) {
		/* RFC 8684 section 3.7. */
	        if (subflow->send_mp_fail) {
			...
		if (subflow->mp_join || subflow->fully_established) {
			...
		__mptcp_do_fallback(msk);
	}

	skb = skb_peek(&ssk->sk_receive_queue);
	...
WDYT?
Mat Martineau Sept. 10, 2021, 12:21 a.m. UTC | #2 
On Thu, 9 Sep 2021, Paolo Abeni wrote:

> On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
>> From: Geliang Tang <geliangtang@xiaomi.com>
>>
>> This patch added a new mapping status named MAPPING_INFINITE. If the
>> MPTCP_INFINITE_DONE flag is set in get_mapping_status, return this new
>> status. And in subflow_check_data_avail, if this status is set, goto the
>> 'infinite' lable to fallback.
>>
>> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
>> ---
>>  net/mptcp/subflow.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 951aafb6021e..ad8efe56eab6 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -798,6 +798,7 @@ enum mapping_status {
>>  	MAPPING_INVALID,
>>  	MAPPING_EMPTY,
>>  	MAPPING_DATA_FIN,
>> +	MAPPING_INFINITE,
>>  	MAPPING_DUMMY
>>  };
>>
>> @@ -938,6 +939,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
>>  	if (!skb)
>>  		return MAPPING_EMPTY;
>>
>> +	if (mptcp_check_infinite(ssk))
>> +		return MAPPING_INFINITE;
>> +
>>  	if (mptcp_check_fallback(ssk))
>>  		return MAPPING_DUMMY;
>>
>> @@ -1121,6 +1125,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>
>>  		status = get_mapping_status(ssk, msk);
>>  		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
>> +		if (unlikely(status == MAPPING_INFINITE))
>> +			goto infinite;
>> +
>>  		if (unlikely(status == MAPPING_INVALID))
>>  			goto fallback;
>>
>> @@ -1192,6 +1199,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>  		return false;
>>  	}
>>
>> +infinite:
>>  	__mptcp_do_fallback(msk);
>>  	skb = skb_peek(&ssk->sk_receive_queue);
>>  	subflow->map_valid = 1;
>
>
> It looks like MAPPING_INFINITE has almost the same behavior
> of MAPPING_DUMMY.

This is something else I asked for in the v1 review, to avoid reusing 
MAPPING_INVALID in a confusing way :)

How about using a switch statement after get_mapping_status() instead of 4 
if's in a row?

>
> I think we can avoid the new conditional in get_mapping_status().
> Eventually we could do all the error checking after the 'fallback:'
> label only if the msk has not fallen back yet:
>
> fallback:
> 	if (!__mptcp_check_fallback(msk)) {
> 		/* RFC 8684 section 3.7. */
> 	        if (subflow->send_mp_fail) {
> 			...
> 		if (subflow->mp_join || subflow->fully_established) {

This condition also needs to check for the infinite mapping case, which is 
why it seemed useful to have a separate MAPPING_ enum. Fallback is being 
triggered here in response to the infinite mapping, so the subflow should 
not be forced to close.

> 			...
> 		__mptcp_do_fallback(msk);
> 	}
>
> 	skb = skb_peek(&ssk->sk_receive_queue);
> 	...
> WDYT?



--
Mat Martineau
Intel
Paolo Abeni Sept. 10, 2021, 3:23 p.m. UTC | #3 
On Thu, 2021-09-09 at 17:21 -0700, Mat Martineau wrote:
> On Thu, 9 Sep 2021, Paolo Abeni wrote:
> 
> > On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> > > From: Geliang Tang <geliangtang@xiaomi.com>
> > > 
> > > This patch added a new mapping status named MAPPING_INFINITE. If the
> > > MPTCP_INFINITE_DONE flag is set in get_mapping_status, return this new
> > > status. And in subflow_check_data_avail, if this status is set, goto the
> > > 'infinite' lable to fallback.
> > > 
> > > Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> > > ---
> > >  net/mptcp/subflow.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > > index 951aafb6021e..ad8efe56eab6 100644
> > > --- a/net/mptcp/subflow.c
> > > +++ b/net/mptcp/subflow.c
> > > @@ -798,6 +798,7 @@ enum mapping_status {
> > >  	MAPPING_INVALID,
> > >  	MAPPING_EMPTY,
> > >  	MAPPING_DATA_FIN,
> > > +	MAPPING_INFINITE,
> > >  	MAPPING_DUMMY
> > >  };
> > > 
> > > @@ -938,6 +939,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
> > >  	if (!skb)
> > >  		return MAPPING_EMPTY;
> > > 
> > > +	if (mptcp_check_infinite(ssk))
> > > +		return MAPPING_INFINITE;
> > > +
> > >  	if (mptcp_check_fallback(ssk))
> > >  		return MAPPING_DUMMY;
> > > 
> > > @@ -1121,6 +1125,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
> > > 
> > >  		status = get_mapping_status(ssk, msk);
> > >  		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
> > > +		if (unlikely(status == MAPPING_INFINITE))
> > > +			goto infinite;
> > > +
> > >  		if (unlikely(status == MAPPING_INVALID))
> > >  			goto fallback;
> > > 
> > > @@ -1192,6 +1199,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
> > >  		return false;
> > >  	}
> > > 
> > > +infinite:
> > >  	__mptcp_do_fallback(msk);
> > >  	skb = skb_peek(&ssk->sk_receive_queue);
> > >  	subflow->map_valid = 1;
> > 
> > It looks like MAPPING_INFINITE has almost the same behavior
> > of MAPPING_DUMMY.
> 
> This is something else I asked for in the v1 review, to avoid reusing 
> MAPPING_INVALID in a confusing way :)

I read that ;) I thought 'MAPPING_DUMMY' would be less confusing.

> How about using a switch statement after get_mapping_status() instead of 4 
> if's in a row?

Will be mostly the same, from generate code perspective, I think.

What if we rename MAPPING_DUMMY into MAPPING_INFINITE, and we use only
a single value? MAPPING_DUMMY is currently used after a fallback to
implement the sort of infinite mapping we use there.

> > I think we can avoid the new conditional in get_mapping_status().
> > Eventually we could do all the error checking after the 'fallback:'
> > label only if the msk has not fallen back yet:
> > 
> > fallback:
> > 	if (!__mptcp_check_fallback(msk)) {
> > 		/* RFC 8684 section 3.7. */
> > 	        if (subflow->send_mp_fail) {
> > 			...
> > 		if (subflow->mp_join || subflow->fully_established) {
> 
> This condition also needs to check for the infinite mapping case, which is 
> why it seemed useful to have a separate MAPPING_ enum. Fallback is being 
> triggered here in response to the infinite mapping, so the subflow should 
> not be forced to close.

My point is all about avoiding additional conditionals in the 'fast-
path' / default receive path.

I think we still could do that, and being able to discriminate here
between infinite mapping received or not:

- get_mapping_status() returns the same value in the dummy and infinite
mapping case.
- in the infinite mapping case, get_mapping_status() additionally sets
  subflow->map_data_len to 0, 
- in the above code - under 'if (!__mptcp_check_fallback(msk)) {' -
 subflow->map_data_len == 0 implies infinite mapping received, 

WDYT?

Thanks!

Paolo
Mat Martineau Sept. 10, 2021, 5:22 p.m. UTC | #4 
On Fri, 10 Sep 2021, Paolo Abeni wrote:

> On Thu, 2021-09-09 at 17:21 -0700, Mat Martineau wrote:
>> On Thu, 9 Sep 2021, Paolo Abeni wrote:
>>
>>> On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
>>>> From: Geliang Tang <geliangtang@xiaomi.com>
>>>>
>>>> This patch added a new mapping status named MAPPING_INFINITE. If the
>>>> MPTCP_INFINITE_DONE flag is set in get_mapping_status, return this new
>>>> status. And in subflow_check_data_avail, if this status is set, goto the
>>>> 'infinite' lable to fallback.
>>>>
>>>> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
>>>> ---
>>>>  net/mptcp/subflow.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>>>> index 951aafb6021e..ad8efe56eab6 100644
>>>> --- a/net/mptcp/subflow.c
>>>> +++ b/net/mptcp/subflow.c
>>>> @@ -798,6 +798,7 @@ enum mapping_status {
>>>>  	MAPPING_INVALID,
>>>>  	MAPPING_EMPTY,
>>>>  	MAPPING_DATA_FIN,
>>>> +	MAPPING_INFINITE,
>>>>  	MAPPING_DUMMY
>>>>  };
>>>>
>>>> @@ -938,6 +939,9 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
>>>>  	if (!skb)
>>>>  		return MAPPING_EMPTY;
>>>>
>>>> +	if (mptcp_check_infinite(ssk))
>>>> +		return MAPPING_INFINITE;
>>>> +
>>>>  	if (mptcp_check_fallback(ssk))
>>>>  		return MAPPING_DUMMY;
>>>>
>>>> @@ -1121,6 +1125,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>>>
>>>>  		status = get_mapping_status(ssk, msk);
>>>>  		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
>>>> +		if (unlikely(status == MAPPING_INFINITE))
>>>> +			goto infinite;
>>>> +
>>>>  		if (unlikely(status == MAPPING_INVALID))
>>>>  			goto fallback;
>>>>
>>>> @@ -1192,6 +1199,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>>>  		return false;
>>>>  	}
>>>>
>>>> +infinite:
>>>>  	__mptcp_do_fallback(msk);
>>>>  	skb = skb_peek(&ssk->sk_receive_queue);
>>>>  	subflow->map_valid = 1;
>>>
>>> It looks like MAPPING_INFINITE has almost the same behavior
>>> of MAPPING_DUMMY.
>>
>> This is something else I asked for in the v1 review, to avoid reusing
>> MAPPING_INVALID in a confusing way :)
>
> I read that ;) I thought 'MAPPING_DUMMY' would be less confusing.
>
>> How about using a switch statement after get_mapping_status() instead of 4
>> if's in a row?
>
> Will be mostly the same, from generate code perspective, I think.
>
> What if we rename MAPPING_DUMMY into MAPPING_INFINITE, and we use only
> a single value? MAPPING_DUMMY is currently used after a fallback to
> implement the sort of infinite mapping we use there.
>

That's fine.

>>> I think we can avoid the new conditional in get_mapping_status().
>>> Eventually we could do all the error checking after the 'fallback:'
>>> label only if the msk has not fallen back yet:
>>>
>>> fallback:
>>> 	if (!__mptcp_check_fallback(msk)) {
>>> 		/* RFC 8684 section 3.7. */
>>> 	        if (subflow->send_mp_fail) {
>>> 			...
>>> 		if (subflow->mp_join || subflow->fully_established) {
>>
>> This condition also needs to check for the infinite mapping case, which is
>> why it seemed useful to have a separate MAPPING_ enum. Fallback is being
>> triggered here in response to the infinite mapping, so the subflow should
>> not be forced to close.
>
> My point is all about avoiding additional conditionals in the 'fast-
> path' / default receive path.
>
> I think we still could do that, and being able to discriminate here
> between infinite mapping received or not:
>
> - get_mapping_status() returns the same value in the dummy and infinite
> mapping case.
> - in the infinite mapping case, get_mapping_status() additionally sets
>  subflow->map_data_len to 0,

  - and in other cases, set it to nonzero.

> - in the above code - under 'if (!__mptcp_check_fallback(msk)) {' -
> subflow->map_data_len == 0 implies infinite mapping received,
>
> WDYT?

I think it works. I'm slightly uneasy with overloading 
subflow->map_data_len from a code readability and maintenance perspective, 
but if the optimization pays off it's manageable with some good comments.

--
Mat Martineau
Intel
diff mbox series

Patch

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 951aafb6021e..ad8efe56eab6 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -798,6 +798,7 @@  enum mapping_status {
 	MAPPING_INVALID,
 	MAPPING_EMPTY,
 	MAPPING_DATA_FIN,
+	MAPPING_INFINITE,
 	MAPPING_DUMMY
 };
 
@@ -938,6 +939,9 @@  static enum mapping_status get_mapping_status(struct sock *ssk,
 	if (!skb)
 		return MAPPING_EMPTY;
 
+	if (mptcp_check_infinite(ssk))
+		return MAPPING_INFINITE;
+
 	if (mptcp_check_fallback(ssk))
 		return MAPPING_DUMMY;
 
@@ -1121,6 +1125,9 @@  static bool subflow_check_data_avail(struct sock *ssk)
 
 		status = get_mapping_status(ssk, msk);
 		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
+		if (unlikely(status == MAPPING_INFINITE))
+			goto infinite;
+
 		if (unlikely(status == MAPPING_INVALID))
 			goto fallback;
 
@@ -1192,6 +1199,7 @@  static bool subflow_check_data_avail(struct sock *ssk)
 		return false;
 	}
 
+infinite:
 	__mptcp_do_fallback(msk);
 	skb = skb_peek(&ssk->sk_receive_queue);
 	subflow->map_valid = 1;