[MPTCP] Re: [PATCH] Squash-to: "mptcp: add the incoming RM_ADDR support"

[MPTCP] Re: [PATCH] Squash-to: "mptcp: add the incoming RM_ADDR support"

[Matthieu Baerts]

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

Hi Geliang,

On 15/09/2020 10:04, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for applying this patch.
> 
> Matthieu Baerts <matthieu.baerts(a)tessares.net> 于2020年9月11日周五 上午3:03写道：
>>
>> Hi Geliang, Paolo,
>>
>> On 10/09/2020 18:28, Paolo Abeni wrote:
>>> On Thu, 2020-09-03 at 18:38 +0800, Geliang Tang wrote:
>>>> This squash-to patch fixed three issues in "mptcp: add the incoming RM_ADDR
>>>> support":
>>>>
>>>> 1. We will use another function mptcp_pm_nl_rm_subflow_received to remove a
>>>> local subflow, so the mptcp_pm_nl_rm_addr_received function only need to
>>>> deal with remote address removing. Thus we only need to check
>>>> subflow->remote_id in mptcp_pm_nl_rm_addr_received.
>>>>
>>>> 2. Update PM counters in the right place.
>>>>
>>>> 3. Drop the remot_id fixing code here, since another new patch will deal with
>>>> subflow's remote_id and local_id fixing.
>>
>> Small note: I removed this part from the patch because I previously
>> applied this new patch you mentioned before in the list of commits.
> 
> That's fine, but there are duplicate code here. subflow->remote_id
> will be set twice:
> 
> 1078         subflow->remote_id = remote->id;
>   ... ...
> 1096         subflow->remote_id = remote_id;
> 
> I think we need to remove this duplicate code someday.

Oh yes OK, I didn't see the lines were different. I think I had 
conflicts for some other reasons and I thought the lines were the same.

Now fixed:

- 8d1ec4c8327c: mptcp: drop useless assignment

Tests + export will be started soon.

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

[MPTCP] Re: [PATCH] Squash-to: "mptcp: add the incoming RM_ADDR support"

[Geliang Tang]

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

Hi Matt,

Thanks for applying this patch.

Matthieu Baerts <matthieu.baerts(a)tessares.net> 于2020年9月11日周五 上午3:03写道：
>
> Hi Geliang, Paolo,
>
> On 10/09/2020 18:28, Paolo Abeni wrote:
> > On Thu, 2020-09-03 at 18:38 +0800, Geliang Tang wrote:
> >> This squash-to patch fixed three issues in "mptcp: add the incoming RM_ADDR
> >> support":
> >>
> >> 1. We will use another function mptcp_pm_nl_rm_subflow_received to remove a
> >> local subflow, so the mptcp_pm_nl_rm_addr_received function only need to
> >> deal with remote address removing. Thus we only need to check
> >> subflow->remote_id in mptcp_pm_nl_rm_addr_received.
> >>
> >> 2. Update PM counters in the right place.
> >>
> >> 3. Drop the remot_id fixing code here, since another new patch will deal with
> >> subflow's remote_id and local_id fixing.
>
> Small note: I removed this part from the patch because I previously
> applied this new patch you mentioned before in the list of commits.

That's fine, but there are duplicate code here. subflow->remote_id
will be set twice:

1078         subflow->remote_id = remote->id;
 ... ...
1096         subflow->remote_id = remote_id;

I think we need to remove this duplicate code someday.

-Geliang


>
> >>
> >> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> >
> > LGTM, thanks!
>
> Thank you for the patch and the review!
>
> Just applied:
>
> - bef779739129: "squashed" (with conflicts) in "mptcp: add the incoming
> RM_ADDR support"
> - b826ff1c8ae3..377cd31c5d51: result
>
> Tests + export are in progress!
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

[MPTCP] Re: [PATCH] Squash-to: "mptcp: add the incoming RM_ADDR support"

[Matthieu Baerts]

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

Hi Geliang, Paolo,

On 10/09/2020 18:28, Paolo Abeni wrote:
> On Thu, 2020-09-03 at 18:38 +0800, Geliang Tang wrote:
>> This squash-to patch fixed three issues in "mptcp: add the incoming RM_ADDR
>> support":
>>
>> 1. We will use another function mptcp_pm_nl_rm_subflow_received to remove a
>> local subflow, so the mptcp_pm_nl_rm_addr_received function only need to
>> deal with remote address removing. Thus we only need to check
>> subflow->remote_id in mptcp_pm_nl_rm_addr_received.
>>
>> 2. Update PM counters in the right place.
>>
>> 3. Drop the remot_id fixing code here, since another new patch will deal with
>> subflow's remote_id and local_id fixing.

Small note: I removed this part from the patch because I previously 
applied this new patch you mentioned before in the list of commits.

>>
>> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> 
> LGTM, thanks!

Thank you for the patch and the review!

Just applied:

- bef779739129: "squashed" (with conflicts) in "mptcp: add the incoming 
RM_ADDR support"
- b826ff1c8ae3..377cd31c5d51: result

Tests + export are in progress!

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

[MPTCP] Re: [PATCH] Squash-to: "mptcp: add the incoming RM_ADDR support"

[Paolo Abeni]

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

On Thu, 2020-09-03 at 18:38 +0800, Geliang Tang wrote:
> This squash-to patch fixed three issues in "mptcp: add the incoming RM_ADDR
> support":
> 
> 1. We will use another function mptcp_pm_nl_rm_subflow_received to remove a
> local subflow, so the mptcp_pm_nl_rm_addr_received function only need to
> deal with remote address removing. Thus we only need to check
> subflow->remote_id in mptcp_pm_nl_rm_addr_received.
> 
> 2. Update PM counters in the right place.
> 
> 3. Drop the remot_id fixing code here, since another new patch will deal with
> subflow's remote_id and local_id fixing.
> 
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
>  net/mptcp/pm_netlink.c | 16 ++++++++--------
>  net/mptcp/subflow.c    |  1 -
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 544adfe66454..b1ce74bc589d 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -173,7 +173,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>  {
>  	struct sock *sk = (struct sock *)msk;
>  	struct mptcp_pm_addr_entry *local;
> -	struct mptcp_addr_info remote = { 0 };
> +	struct mptcp_addr_info remote;
>  	struct pm_nl_pernet *pernet;
>  
>  	pernet = net_generic(sock_net((struct sock *)msk), pm_nl_pernet_id);
> @@ -266,7 +266,7 @@ void mptcp_pm_nl_rm_addr_received(struct mptcp_sock *msk)
>  	struct mptcp_subflow_context *subflow, *tmp;
>  	struct sock *sk = (struct sock *)msk;
>  
> -	pr_debug("rm_id %d", msk->pm.rm_id);
> +	pr_debug("address rm_id %d", msk->pm.rm_id);
>  
>  	if (!msk->pm.rm_id)
>  		return;
> @@ -274,23 +274,23 @@ void mptcp_pm_nl_rm_addr_received(struct mptcp_sock *msk)
>  	if (list_empty(&msk->conn_list))
>  		return;
>  
> -	msk->pm.add_addr_accepted--;
> -	msk->pm.subflows--;
> -	WRITE_ONCE(msk->pm.accept_addr, true);
> -
>  	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
>  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>  		int how = RCV_SHUTDOWN | SEND_SHUTDOWN;
>  		long timeout = 0;
>  
> -		if (msk->pm.rm_id != subflow->remote_id &&
> -		    msk->pm.rm_id != subflow->local_id)
> +		if (msk->pm.rm_id != subflow->remote_id)
>  			continue;
>  
>  		spin_unlock_bh(&msk->pm.lock);
>  		mptcp_subflow_shutdown(sk, ssk, how);
>  		__mptcp_close_ssk(sk, ssk, subflow, timeout);
>  		spin_lock_bh(&msk->pm.lock);
> +
> +		msk->pm.add_addr_accepted--;
> +		msk->pm.subflows--;
> +		WRITE_ONCE(msk->pm.accept_addr, true);
> +
>  		break;
>  	}
>  }
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 8464170e8dc1..535a3f9f8cfc 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1094,7 +1094,6 @@ int __mptcp_subflow_connect(struct sock *sk, int ifindex,
>  	subflow->remote_key = msk->remote_key;
>  	subflow->local_key = msk->local_key;
>  	subflow->token = msk->token;
> -	subflow->remote_id = remote->id;
>  	mptcp_info2sockaddr(loc, &addr);
>  
>  	addrlen = sizeof(struct sockaddr_in);

LGTM, thanks!

Paolo