[MPTCP][PATCH mptcp-next] Squash to "mptcp: generate the data checksum"

[MPTCP][PATCH mptcp-next] Squash to "mptcp: generate the data checksum"

[Geliang Tang]

Move the csum_replace2 trunk into the later patch "mptcp: receive
checksum for MP_CAPABLE with data".

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/options.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index beac01f58cba..99fc21406168 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -519,9 +519,6 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
 		 */
 		ext->data_fin = 1;
 		ext->data_len++;
-
-		/* the pseudo header has changed, update the csum accordingly */
-		csum_replace2(&ext->csum, htons(ext->data_len - 1), htons(ext->data_len));
 	}
 }
 
-- 
2.31.1

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: generate the data checksum"

[Mat Martineau]

On Fri, 7 May 2021, Geliang Tang wrote:

> Move the csum_replace2 trunk into the later patch "mptcp: receive
> checksum for MP_CAPABLE with data".
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/options.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index beac01f58cba..99fc21406168 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -519,9 +519,6 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
> 		 */
> 		ext->data_fin = 1;
> 		ext->data_len++;
> -
> -		/* the pseudo header has changed, update the csum accordingly */
> -		csum_replace2(&ext->csum, htons(ext->data_len - 1), htons(ext->data_len));

The commit message above says this is is moved to another patch, but the 
posted squash-to patch for "mptcp: receive checksum for MP_CAPABLE with 
data" doesn't add this functionality back. Is a hunk or another squash-to 
patch missing?


Thanks,

--
Mat Martineau
Intel

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: generate the data checksum"

[Geliang Tang]

Hi Mat,

On Mon, May 10, 2021 at 05:45:48PM -0700, Mat Martineau wrote:
> On Fri, 7 May 2021, Geliang Tang wrote:
> 
> > Move the csum_replace2 trunk into the later patch "mptcp: receive
> > checksum for MP_CAPABLE with data".
> > 
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> > net/mptcp/options.c | 3 ---
> > 1 file changed, 3 deletions(-)
> > 
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index beac01f58cba..99fc21406168 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -519,9 +519,6 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
> > 		 */
> > 		ext->data_fin = 1;
> > 		ext->data_len++;
> > -
> > -		/* the pseudo header has changed, update the csum accordingly */
> > -		csum_replace2(&ext->csum, htons(ext->data_len - 1), htons(ext->data_len));
> 
> The commit message above says this is is moved to another patch, but the
> posted squash-to patch for "mptcp: receive checksum for MP_CAPABLE with
> data" doesn't add this functionality back. Is a hunk or another squash-to
> patch missing?

Sorry, I didn't describe it clearly.

No squash-to patch is missing here, this truck will be moved to another
patch automatically when resolving the conflict.

Apply this squash-to patch (Squash to "mptcp: generate the data checksum")
and rebase, we will get a conflict with "mptcp: receive checksum for
MP_CAPABLE with data":

$ git rebase --continue
Auto-merging net/mptcp/protocol.h
Auto-merging net/mptcp/options.c
CONFLICT (content): Merge conflict in net/mptcp/options.c
error: could not apply d56805c0f705... mptcp: receive checksum for MP_CAPABLE with data

Resolve this conflict like this:

'''
<<<<<<< HEAD
=======

                /* the pseudo header has changed, update the csum accordingly */
                if (ext->csum_reqd)
                        csum_replace2(&ext->csum, htons(ext->data_len - 1),
                                      htons(ext->data_len));
>>>>>>> d56805c0f705... mptcp: receive checksum for MP_CAPABLE with data
        }
}
'''

 ->

'''

                /* the pseudo header has changed, update the csum accordingly */
                if (ext->csum_reqd)
                        csum_replace2(&ext->csum, htons(ext->data_len - 1),
                                      htons(ext->data_len));
        }
}
'''

Then the csum_replace2 trunk have been moved to the patch "mptcp: receive
checksum for MP_CAPABLE with data" automatically.

Thanks.

-Geliang

> 
> 
> Thanks,
> 
> --
> Mat Martineau
> Intel

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: generate the data checksum"

[Mat Martineau]

On Tue, 11 May 2021, Geliang Tang wrote:

> Hi Mat,
>
> On Mon, May 10, 2021 at 05:45:48PM -0700, Mat Martineau wrote:
>> On Fri, 7 May 2021, Geliang Tang wrote:
>>
>>> Move the csum_replace2 trunk into the later patch "mptcp: receive
>>> checksum for MP_CAPABLE with data".
>>>
>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>> ---
>>> net/mptcp/options.c | 3 ---
>>> 1 file changed, 3 deletions(-)
>>>
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index beac01f58cba..99fc21406168 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -519,9 +519,6 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
>>> 		 */
>>> 		ext->data_fin = 1;
>>> 		ext->data_len++;
>>> -
>>> -		/* the pseudo header has changed, update the csum accordingly */
>>> -		csum_replace2(&ext->csum, htons(ext->data_len - 1), htons(ext->data_len));
>>
>> The commit message above says this is is moved to another patch, but the
>> posted squash-to patch for "mptcp: receive checksum for MP_CAPABLE with
>> data" doesn't add this functionality back. Is a hunk or another squash-to
>> patch missing?
>
> Sorry, I didn't describe it clearly.
>
> No squash-to patch is missing here, this truck will be moved to another
> patch automatically when resolving the conflict.
>
> Apply this squash-to patch (Squash to "mptcp: generate the data checksum")
> and rebase, we will get a conflict with "mptcp: receive checksum for
> MP_CAPABLE with data":
>
> $ git rebase --continue
> Auto-merging net/mptcp/protocol.h
> Auto-merging net/mptcp/options.c
> CONFLICT (content): Merge conflict in net/mptcp/options.c
> error: could not apply d56805c0f705... mptcp: receive checksum for MP_CAPABLE with data
>
> Resolve this conflict like this:
>
> '''
> <<<<<<< HEAD
> =======
>
>                /* the pseudo header has changed, update the csum accordingly */
>                if (ext->csum_reqd)
>                        csum_replace2(&ext->csum, htons(ext->data_len - 1),
>                                      htons(ext->data_len));
>>>>>>>> d56805c0f705... mptcp: receive checksum for MP_CAPABLE with data
>        }
> }
> '''
>
> ->
>
> '''
>
>                /* the pseudo header has changed, update the csum accordingly */
>                if (ext->csum_reqd)
>                        csum_replace2(&ext->csum, htons(ext->data_len - 1),
>                                      htons(ext->data_len));
>        }
> }
> '''
>
> Then the csum_replace2 trunk have been moved to the patch "mptcp: receive
> checksum for MP_CAPABLE with data" automatically.
>

Ok, thanks for the information. In the future if there's a 
conflict-creating squash patch like this it does help to have specific 
conflict resolution notes.

It looks like this chunk of code is not needed at all, though - 
mptcp_make_csum() hasn't been called yet so the pseudoheader has not been 
included in the checksum.

--
Mat Martineau
Intel

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: generate the data checksum"

[Geliang Tang]

Hi Mat,

Mat Martineau <mathew.j.martineau@linux.intel.com> 于2021年5月12日周三 上午7:17写道：
>
> On Tue, 11 May 2021, Geliang Tang wrote:
>
> > Hi Mat,
> >
> > On Mon, May 10, 2021 at 05:45:48PM -0700, Mat Martineau wrote:
> >> On Fri, 7 May 2021, Geliang Tang wrote:
> >>
> >>> Move the csum_replace2 trunk into the later patch "mptcp: receive
> >>> checksum for MP_CAPABLE with data".
> >>>
> >>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> >>> ---
> >>> net/mptcp/options.c | 3 ---
> >>> 1 file changed, 3 deletions(-)
> >>>
> >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>> index beac01f58cba..99fc21406168 100644
> >>> --- a/net/mptcp/options.c
> >>> +++ b/net/mptcp/options.c
> >>> @@ -519,9 +519,6 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
> >>>              */
> >>>             ext->data_fin = 1;
> >>>             ext->data_len++;
> >>> -
> >>> -           /* the pseudo header has changed, update the csum accordingly */
> >>> -           csum_replace2(&ext->csum, htons(ext->data_len - 1), htons(ext->data_len));
> >>
> >> The commit message above says this is is moved to another patch, but the
> >> posted squash-to patch for "mptcp: receive checksum for MP_CAPABLE with
> >> data" doesn't add this functionality back. Is a hunk or another squash-to
> >> patch missing?
> >
> > Sorry, I didn't describe it clearly.
> >
> > No squash-to patch is missing here, this truck will be moved to another
> > patch automatically when resolving the conflict.
> >
> > Apply this squash-to patch (Squash to "mptcp: generate the data checksum")
> > and rebase, we will get a conflict with "mptcp: receive checksum for
> > MP_CAPABLE with data":
> >
> > $ git rebase --continue
> > Auto-merging net/mptcp/protocol.h
> > Auto-merging net/mptcp/options.c
> > CONFLICT (content): Merge conflict in net/mptcp/options.c
> > error: could not apply d56805c0f705... mptcp: receive checksum for MP_CAPABLE with data
> >
> > Resolve this conflict like this:
> >
> > '''
> > <<<<<<< HEAD
> > =======
> >
> >                /* the pseudo header has changed, update the csum accordingly */
> >                if (ext->csum_reqd)
> >                        csum_replace2(&ext->csum, htons(ext->data_len - 1),
> >                                      htons(ext->data_len));
> >>>>>>>> d56805c0f705... mptcp: receive checksum for MP_CAPABLE with data
> >        }
> > }
> > '''
> >
> > ->
> >
> > '''
> >
> >                /* the pseudo header has changed, update the csum accordingly */
> >                if (ext->csum_reqd)
> >                        csum_replace2(&ext->csum, htons(ext->data_len - 1),
> >                                      htons(ext->data_len));
> >        }
> > }
> > '''
> >
> > Then the csum_replace2 trunk have been moved to the patch "mptcp: receive
> > checksum for MP_CAPABLE with data" automatically.
> >
>
> Ok, thanks for the information. In the future if there's a
> conflict-creating squash patch like this it does help to have specific
> conflict resolution notes.
>
> It looks like this chunk of code is not needed at all, though -
> mptcp_make_csum() hasn't been called yet so the pseudoheader has not been
> included in the checksum.

Yes, indeed, I just sent another squash-to patch (Squash to "mptcp: receive
checksum for MP_CAPABLE with data") to drop this chunk.

Thanks.


-Geliang

>
> --
> Mat Martineau
> Intel

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: generate the data checksum"

[Matthieu Baerts]

Hi Geliang, Mat,

On 07/05/2021 06:18, Geliang Tang wrote:
> Move the csum_replace2 trunk into the later patch "mptcp: receive
> checksum for MP_CAPABLE with data".
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>

Thank you for the patch and the review!

Now in our tree!

- 04461ed21558: Squash to "mptcp: generate the data checksum"
- 2f6a4aa17ccb: conflict in
t/mptcp-receive-checksum-for-MP_CAPABLE-with-data
- Results: 6628a7b4ea41..f8de7f4600ea

No tag, we need 'Squash to "mptcp: receive checksum for MP_CAPABLE with
data"'.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: generate the data checksum"

[Matthieu Baerts]

Hi Geliang, Mat,

On 18/05/2021 09:06, Geliang Tang wrote:
> Move this line "__wsum csum = ~csum_unfold(mpext->csum);" from "mptcp:
> validate the data checksum" to "mptcp: generate the data checksum".

Sorry for the delay!

This is now applied in our tree:

- 982a61af0143: "squashed" in "mptcp: generate the data checksum"
- Results: e61f1205c3fe..df4fcde34fab

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210603T145651
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210603T145651

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: generate the data checksum"

[Mat Martineau]

On Tue, 18 May 2021, Geliang Tang wrote:

> Move this line "__wsum csum = ~csum_unfold(mpext->csum);" from "mptcp:
> validate the data checksum" to "mptcp: generate the data checksum".
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/protocol.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 38ce8d50e665..58253bd09f93 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1311,7 +1311,7 @@ static bool mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk)
> static void mptcp_update_data_checksum(struct sk_buff *skb, int added)
> {
> 	struct mptcp_ext *mpext = mptcp_get_ext(skb);
> -	__wsum csum = csum_unfold(mpext->csum);
> +	__wsum csum = ~csum_unfold(mpext->csum);
> 	int offset = skb->len - added;
>
> 	mpext->csum = csum_fold(csum_block_add(csum, skb_checksum(skb, offset, added, 0), offset));
> -- 
> 2.31.1

Looks good, thanks Geliang.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel

[MPTCP][PATCH mptcp-next] Squash to "mptcp: generate the data checksum"

[Geliang Tang]

Move this line "__wsum csum = ~csum_unfold(mpext->csum);" from "mptcp:
validate the data checksum" to "mptcp: generate the data checksum".

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/protocol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 38ce8d50e665..58253bd09f93 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1311,7 +1311,7 @@ static bool mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk)
 static void mptcp_update_data_checksum(struct sk_buff *skb, int added)
 {
 	struct mptcp_ext *mpext = mptcp_get_ext(skb);
-	__wsum csum = csum_unfold(mpext->csum);
+	__wsum csum = ~csum_unfold(mpext->csum);
 	int offset = skb->len - added;
 
 	mpext->csum = csum_fold(csum_block_add(csum, skb_checksum(skb, offset, added, 0), offset));
-- 
2.31.1