[MPTCP] Re: [RFC PATCH net 1/2] mptcp: Consistently use READ_ONCE/WRITE_ONCE with msk->ack_seq

[MPTCP] Re: [RFC PATCH net 1/2] mptcp: Consistently use READ_ONCE/WRITE_ONCE with msk->ack_seq

[Matthieu Baerts]

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

Hi Mat,

On 30/09/2020 12:19, Matthieu Baerts wrote:
> Hi Mat,
> 
> On 26/09/2020 02:24, Mat Martineau wrote:
>> The msk->ack_seq value is sometimes read without the msk lock held, so
>> make proper use of READ_ONCE and WRITE_ONCE.
>>
>> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
>> ---
>>
>> This does create some merge conflicts with net-next - what's the best
>> way to handle that?
> 
> Should we send to netdev ML about how we resolve the conflicts?

(...)

> Patch 2:
> - subflow.c: we don't use the returned value of 
> mptcp_update_rcv_data_fin() in net-next

I just noticed your commit ef59b1953c26 ("mptcp: Wake up MPTCP worker 
when DATA_FIN found on a TCP FIN packet") is not on net-next yet.

So we do use the returned value if I put your two new patches on top of 
ef59b1953c26!

The "export" branch is going to be updated soon.

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

[MPTCP] Re: [RFC PATCH net 1/2] mptcp: Consistently use READ_ONCE/WRITE_ONCE with msk->ack_seq

[Matthieu Baerts]

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

Hi Mat,

On 30/09/2020 19:33, Mat Martineau wrote:
> On Wed, 30 Sep 2020, Matthieu Baerts wrote:
> 
>> Hi Mat,
>>
>> On 30/09/2020 12:31, Matthieu Baerts wrote:
>>> Hi Mat,
>>>
>>> On 30/09/2020 12:19, Matthieu Baerts wrote:
>>>> Hi Mat,
>>>>
>>>> On 26/09/2020 02:24, Mat Martineau wrote:
>>>>> The msk->ack_seq value is sometimes read without the msk lock held, so
>>>>> make proper use of READ_ONCE and WRITE_ONCE.
>>>>>
>>>>> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
>>>>> ---
>>>>>
>>>>> This does create some merge conflicts with net-next - what's the best
>>>>> way to handle that?
>>>>
>>>> Should we send to netdev ML about how we resolve the conflicts?
>>>
>>> (...)
>>>
>>>> Patch 2:
>>>> - subflow.c: we don't use the returned value of 
>>>> mptcp_update_rcv_data_fin() in net-next
>>>
>>> I just noticed your commit ef59b1953c26 ("mptcp: Wake up MPTCP worker 
>>> when DATA_FIN found on a TCP FIN packet") is not on net-next yet.
>>>
>>> So we do use the returned value if I put your two new patches on top 
>>> of ef59b1953c26!
>>>
>>> The "export" branch is going to be updated soon.
>>
>> I also just pushed a new branch: merge_net_net-next_20200930
>>
>> You can have a look at bac51c27a3c9 ("Merge remote-tracking branch 
>> 'net' into net-next") for the merge resolution. We have the 
>> explanation with 'git show' but I don't know if we can have the same 
>> view on GitHub.
>>
>> This link is not helpful :)
>>
>> https://github.com/multipath-tcp/mptcp_net-next/commit/merge_net_net-next_20200930 
>>
>>
> 
> Hi Matthieu -
> 
> I mentioned my recommended merge resolution in the cover letter for the 
> latest -net changes:
> 
> """
> This does introduce two merge conflicts with net-next, but both have
> straightforward resolution. Patch 1 modifies a line that got removed in
> net-next so the modification can be dropped when merging. Patch 2 will
> require a trivial conflict resolution for a modified function
> declaration.
> """

Indeed, my bad, I missed that!

> Rather than update __mptcp_move_skb() as part of the merge commit, I 
> think it would be better to just drop the change during the merge and 
> send a separate patch to net-next to add the WRITE_ONCE() to 
> __mptcp_move_skb().

Indeed, certainly easier for the maintainer. We will have to remember 
fixing this but I should keep track of this particular modification in 
the "export" branch anyway.

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

[MPTCP] Re: [RFC PATCH net 1/2] mptcp: Consistently use READ_ONCE/WRITE_ONCE with msk->ack_seq

[Mat Martineau]

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

On Wed, 30 Sep 2020, Matthieu Baerts wrote:

> Hi Mat,
>
> On 30/09/2020 12:31, Matthieu Baerts wrote:
>> Hi Mat,
>> 
>> On 30/09/2020 12:19, Matthieu Baerts wrote:
>>> Hi Mat,
>>> 
>>> On 26/09/2020 02:24, Mat Martineau wrote:
>>>> The msk->ack_seq value is sometimes read without the msk lock held, so
>>>> make proper use of READ_ONCE and WRITE_ONCE.
>>>> 
>>>> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
>>>> ---
>>>> 
>>>> This does create some merge conflicts with net-next - what's the best
>>>> way to handle that?
>>> 
>>> Should we send to netdev ML about how we resolve the conflicts?
>> 
>> (...)
>> 
>>> Patch 2:
>>> - subflow.c: we don't use the returned value of 
>>> mptcp_update_rcv_data_fin() in net-next
>> 
>> I just noticed your commit ef59b1953c26 ("mptcp: Wake up MPTCP worker when 
>> DATA_FIN found on a TCP FIN packet") is not on net-next yet.
>> 
>> So we do use the returned value if I put your two new patches on top of 
>> ef59b1953c26!
>> 
>> The "export" branch is going to be updated soon.
>
> I also just pushed a new branch: merge_net_net-next_20200930
>
> You can have a look at bac51c27a3c9 ("Merge remote-tracking branch 'net' into 
> net-next") for the merge resolution. We have the explanation with 'git show' 
> but I don't know if we can have the same view on GitHub.
>
> This link is not helpful :)
>
> https://github.com/multipath-tcp/mptcp_net-next/commit/merge_net_net-next_20200930
>

Hi Matthieu -

I mentioned my recommended merge resolution in the cover letter for the 
latest -net changes:

"""
This does introduce two merge conflicts with net-next, but both have
straightforward resolution. Patch 1 modifies a line that got removed in
net-next so the modification can be dropped when merging. Patch 2 will
require a trivial conflict resolution for a modified function
declaration.
"""

Rather than update __mptcp_move_skb() as part of the merge commit, I think 
it would be better to just drop the change during the merge and send a 
separate patch to net-next to add the WRITE_ONCE() to __mptcp_move_skb().

--
Mat Martineau
Intel

[MPTCP] Re: [RFC PATCH net 1/2] mptcp: Consistently use READ_ONCE/WRITE_ONCE with msk->ack_seq

[Matthieu Baerts]

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

Hi Mat,

On 30/09/2020 12:31, Matthieu Baerts wrote:
> Hi Mat,
> 
> On 30/09/2020 12:19, Matthieu Baerts wrote:
>> Hi Mat,
>>
>> On 26/09/2020 02:24, Mat Martineau wrote:
>>> The msk->ack_seq value is sometimes read without the msk lock held, so
>>> make proper use of READ_ONCE and WRITE_ONCE.
>>>
>>> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
>>> ---
>>>
>>> This does create some merge conflicts with net-next - what's the best
>>> way to handle that?
>>
>> Should we send to netdev ML about how we resolve the conflicts?
> 
> (...)
> 
>> Patch 2:
>> - subflow.c: we don't use the returned value of 
>> mptcp_update_rcv_data_fin() in net-next
> 
> I just noticed your commit ef59b1953c26 ("mptcp: Wake up MPTCP worker 
> when DATA_FIN found on a TCP FIN packet") is not on net-next yet.
> 
> So we do use the returned value if I put your two new patches on top of 
> ef59b1953c26!
> 
> The "export" branch is going to be updated soon.

I also just pushed a new branch: merge_net_net-next_20200930

You can have a look at bac51c27a3c9 ("Merge remote-tracking branch 'net' 
into net-next") for the merge resolution. We have the explanation with 
'git show' but I don't know if we can have the same view on GitHub.

This link is not helpful :)

https://github.com/multipath-tcp/mptcp_net-next/commit/merge_net_net-next_20200930

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

[MPTCP] Re: [RFC PATCH net 1/2] mptcp: Consistently use READ_ONCE/WRITE_ONCE with msk->ack_seq

[Matthieu Baerts]

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

Hi Mat,

On 26/09/2020 02:24, Mat Martineau wrote:
> The msk->ack_seq value is sometimes read without the msk lock held, so
> make proper use of READ_ONCE and WRITE_ONCE.
> 
> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> ---
> 
> This does create some merge conflicts with net-next - what's the best
> way to handle that?

Should we send to netdev ML about how we resolve the conflicts?

I am adding the two patches you sent upstream to the "export" branch and 
resolved the conflicts.

- d80ba0d7cf7f: mptcp: Consistently use READ_ONCE/WRITE_ONCE with 
msk->ack_seq
- 69bd9c6e1c45: mptcp: Handle incoming 32-bit DATA_FIN values

About the conflicts I had:


Patch 1:
- protocol.c: using the version from net-next and modifying 
__mptcp_move_skb() where we have:

   msk->ack_seq += copy_len;


Patch 2:
- subflow.c: we don't use the returned value of 
mptcp_update_rcv_data_fin() in net-next
- protocol.h: we take the new version of mptcp_update_rcv_data_fin() and 
we keep mptcp_destroy_common() unchanged.

I don't know if we need to provide a Git repo or a diff.
Note that on my side, I don't have the result of a merge but a 
modification of the patches for net-next. I don't think that's what they 
want.

Note that we no longer read the output of mptcp_update_rcv_data_fin().


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