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

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

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