[MPTCP] Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: use WRITE_ONCE/READ_ONCE for the pernet *_max"

[MPTCP] Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: use WRITE_ONCE/READ_ONCE for the pernet *_max"

[Matthieu Baerts]

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

Hi Geliang, Mat,

On 01/02/2021 23:21, Mat Martineau wrote:
> On Sun, 31 Jan 2021, Geliang Tang wrote:
> 
>> As Jakub suggested, drop all the READ_ONCEs in the original patch.

Thank you for the patch and the review!

>> And the commit message needs to be updated too:
>>
>> -
>>
>> Subject: mptcp: use WRITE_ONCE for the pernet *_max
>>
>> This patch uses WRITE_ONCE() for all the pernet add_addr_signal_max,
>> add_addr_accept_max, local_addr_max and subflows_max fields in struct
>> pm_nl_pernet to avoid concurrency issues.
>>
>> -
>>
>> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
>> ---
>> net/mptcp/pm_netlink.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Yes, Jakub was correct to point out that READ_ONCE() isn't needed when 
> all of the writes are done with the lock held.
> 
> I'll go ahead and apply this locally and resubmit v2 now. Matthieu, I 
> guess you should apply as well so the topgit tree updates correctly when 
> the patches are applied on net-next.

Done:

- 04e13b990f05: "squashed" in "mptcp: use WRITE_ONCE/READ_ONCE for the 
pernet *_max"
- e82ca37a5376: tg:msg: reflect modif from prev commit
- Results: 4c0c22889ac1..b7b5f6b202bf

Tests + export are in progress!

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

[MPTCP] Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: use WRITE_ONCE/READ_ONCE for the pernet *_max"

[Mat Martineau]

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

On Sun, 31 Jan 2021, Geliang Tang wrote:

> As Jakub suggested, drop all the READ_ONCEs in the original patch.
>
> And the commit message needs to be updated too:
>
> -
>
> Subject: mptcp: use WRITE_ONCE for the pernet *_max
>
> This patch uses WRITE_ONCE() for all the pernet add_addr_signal_max,
> add_addr_accept_max, local_addr_max and subflows_max fields in struct
> pm_nl_pernet to avoid concurrency issues.
>
> -
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> net/mptcp/pm_netlink.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)

Yes, Jakub was correct to point out that READ_ONCE() isn't needed when all 
of the writes are done with the lock held.

I'll go ahead and apply this locally and resubmit v2 now. Matthieu, I 
guess you should apply as well so the topgit tree updates correctly when 
the patches are applied on net-next.

Mat


>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 7d6081d9a1db..c429bd82313e 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -616,11 +616,11 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
> 		pernet->next_id = entry->addr.id;
>
> 	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL) {
> -		addr_max = READ_ONCE(pernet->add_addr_signal_max);
> +		addr_max = pernet->add_addr_signal_max;
> 		WRITE_ONCE(pernet->add_addr_signal_max, addr_max + 1);
> 	}
> 	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
> -		addr_max = READ_ONCE(pernet->local_addr_max);
> +		addr_max = pernet->local_addr_max;
> 		WRITE_ONCE(pernet->local_addr_max, addr_max + 1);
> 	}
>
> @@ -932,11 +932,11 @@ static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info)
> 		return -EINVAL;
> 	}
> 	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL) {
> -		addr_max = READ_ONCE(pernet->add_addr_signal_max);
> +		addr_max = pernet->add_addr_signal_max;
> 		WRITE_ONCE(pernet->add_addr_signal_max, addr_max - 1);
> 	}
> 	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
> -		addr_max = READ_ONCE(pernet->local_addr_max);
> +		addr_max = pernet->local_addr_max;
> 		WRITE_ONCE(pernet->local_addr_max, addr_max - 1);
> 	}
>
> @@ -1140,12 +1140,12 @@ mptcp_nl_cmd_set_limits(struct sk_buff *skb, struct genl_info *info)
> 	int ret;
>
> 	spin_lock_bh(&pernet->lock);
> -	rcv_addrs = READ_ONCE(pernet->add_addr_accept_max);
> +	rcv_addrs = pernet->add_addr_accept_max;
> 	ret = parse_limit(info, MPTCP_PM_ATTR_RCV_ADD_ADDRS, &rcv_addrs);
> 	if (ret)
> 		goto unlock;
>
> -	subflows = READ_ONCE(pernet->subflows_max);
> +	subflows = pernet->subflows_max;
> 	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOWS, &subflows);
> 	if (ret)
> 		goto unlock;
> -- 
> 2.29.2

--
Mat Martineau
Intel