Re: [MPTCP] [PATCH] mptcp: do not warn when replacing an identical mapping.

Re: [MPTCP] [PATCH] mptcp: do not warn when replacing an identical mapping.

[Matthieu Baerts]

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

Hi Paolo,

On 05/06/2019 15:53, Paolo Abeni wrote:
> Said event can happen due GSO/TSO, do not warn then.
> 
> Fixes: af798ed636e2 ("mptcp: Implement MPTCP receive path")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> --
> Notes:
> - I'm ok squashing this commit into the fixed one
> - I can't reproduce the warn locally, need someone else to verify 
>   this one

Thank you for the patch! It looks good to me but I would prefer to wait
for Mat's review on that as he identified the cause.

I tested it locally with the virtme' script and I no longer have the
warning in dmesg!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: do not warn when replacing an identical mapping.

[Matthieu Baerts]

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

Hi Paolo, Mat,

On 05/06/2019 23:31, Mat Martineau wrote:
> 
> On Wed, 5 Jun 2019, Paolo Abeni wrote:
> 
>> Said event can happen due GSO/TSO, do not warn then.
>>
>> Fixes: af798ed636e2 ("mptcp: Implement MPTCP receive path")
>> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
>> -- 
>> Notes:
>> - I'm ok squashing this commit into the fixed one
>> - I can't reproduce the warn locally, need someone else to verify
>>  this one
> 
> This patch eliminates the warnings for me. I think it's good to merge
> (and squash) for now.
> 
> Thanks for the fix, Paolo.

Thank you again for the patch and thank you for the review.

I just added it in our repo:
- 5c7c220aae31: "squash"
- 54cae76d4cfc: the Signed-Off was already there
- da94dd36fcab..78190143f7fb: result

Builds are OK, tests are OK, no more warnings!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: do not warn when replacing an identical mapping.

[Mat Martineau]

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


On Wed, 5 Jun 2019, Paolo Abeni wrote:

> Said event can happen due GSO/TSO, do not warn then.
>
> Fixes: af798ed636e2 ("mptcp: Implement MPTCP receive path")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> --
> Notes:
> - I'm ok squashing this commit into the fixed one
> - I can't reproduce the warn locally, need someone else to verify
>  this one

This patch eliminates the warnings for me. I think it's good to merge (and 
squash) for now.

Thanks for the fix, Paolo.


Mat


> ---
> net/mptcp/protocol.c | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 4e809719dcfd..65ad881daf0d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -291,6 +291,7 @@ static enum mapping_status mptcp_get_mapping(struct sock *ssk)
> 	struct mptcp_ext *mpext;
> 	enum mapping_status ret;
> 	struct sk_buff *skb;
> +	u64 map_seq;
>
> 	skb = skb_peek(&ssk->sk_receive_queue);
> 	if (!skb) {
> @@ -325,18 +326,25 @@ static enum mapping_status mptcp_get_mapping(struct sock *ssk)
> 		goto del_out;
> 	}
>
> -	if (subflow->map_valid)
> -		pr_warn("Replaced mapping before it was done");
> -
> 	if (!mpext->dsn64) {
> -		subflow->map_seq = expand_seq(subflow->map_seq,
> -					      subflow->map_data_len,
> -					      mpext->data_seq);
> +		map_seq = expand_seq(subflow->map_seq, subflow->map_data_len,
> +				     mpext->data_seq);
> 		pr_debug("expanded seq=%llu", subflow->map_seq);
> 	} else {
> -		subflow->map_seq = mpext->data_seq;
> +		map_seq = mpext->data_seq;
> +	}
> +
> +	if (subflow->map_valid) {
> +		/* due to GSO/TSO we can receive the same mapping multiple
> +		 * times, before it's expiration.
> +		 */
> +		if (subflow->map_seq != map_seq ||
> +		    subflow->map_subflow_seq != mpext->subflow_seq ||
> +		    subflow->map_data_len != mpext->data_len)
> +			pr_warn("Replaced mapping before it was done");
> 	}
>
> +	subflow->map_seq = map_seq;
> 	subflow->map_subflow_seq = mpext->subflow_seq;
> 	subflow->map_data_len = mpext->data_len;
> 	subflow->map_valid = 1;
> -- 
> 2.20.1
>
> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp
>

--
Mat Martineau
Intel

[MPTCP] [PATCH] mptcp: do not warn when replacing an identical mapping.

[Paolo Abeni]

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

Said event can happen due GSO/TSO, do not warn then.

Fixes: af798ed636e2 ("mptcp: Implement MPTCP receive path")
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
--
Notes:
- I'm ok squashing this commit into the fixed one
- I can't reproduce the warn locally, need someone else to verify 
  this one
---
 net/mptcp/protocol.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 4e809719dcfd..65ad881daf0d 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -291,6 +291,7 @@ static enum mapping_status mptcp_get_mapping(struct sock *ssk)
 	struct mptcp_ext *mpext;
 	enum mapping_status ret;
 	struct sk_buff *skb;
+	u64 map_seq;
 
 	skb = skb_peek(&ssk->sk_receive_queue);
 	if (!skb) {
@@ -325,18 +326,25 @@ static enum mapping_status mptcp_get_mapping(struct sock *ssk)
 		goto del_out;
 	}
 
-	if (subflow->map_valid)
-		pr_warn("Replaced mapping before it was done");
-
 	if (!mpext->dsn64) {
-		subflow->map_seq = expand_seq(subflow->map_seq,
-					      subflow->map_data_len,
-					      mpext->data_seq);
+		map_seq = expand_seq(subflow->map_seq, subflow->map_data_len,
+				     mpext->data_seq);
 		pr_debug("expanded seq=%llu", subflow->map_seq);
 	} else {
-		subflow->map_seq = mpext->data_seq;
+		map_seq = mpext->data_seq;
+	}
+
+	if (subflow->map_valid) {
+		/* due to GSO/TSO we can receive the same mapping multiple
+		 * times, before it's expiration.
+		 */
+		if (subflow->map_seq != map_seq ||
+		    subflow->map_subflow_seq != mpext->subflow_seq ||
+		    subflow->map_data_len != mpext->data_len)
+			pr_warn("Replaced mapping before it was done");
 	}
 
+	subflow->map_seq = map_seq;
 	subflow->map_subflow_seq = mpext->subflow_seq;
 	subflow->map_data_len = mpext->data_len;
 	subflow->map_valid = 1;
-- 
2.20.1