[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 2/3] mptcp: check the removing initial subflow

[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 2/3] mptcp: check the removing initial subflow

[Mat Martineau]

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

On Tue, 2 Mar 2021, Geliang Tang wrote:

> If the removing address id is 0, the initial subflow needs to be removed.
> Since the initial subflow's both local_id and remote_id are all zeros,
> this patch checks them to make sure it's now removing the right subflow.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> net/mptcp/pm_netlink.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index eaac9824f444..439c9ee57566 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -629,6 +629,11 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
> 			if (rm_list.ids[i] != id)
> 				continue;
>
> +			if (!rm_list.ids[i]) {
> +				if (subflow->local_id || subflow->remote_id)
> +					continue;
> +			}
> +

In my reply to v2, I commented that I did not see the initial subflow 
closed. But that does not mean that RM_ADDR 0 should only affect the 
initial subflow. There may be other subflows involving that address id 
that need to be closed.

Without this commit, do some self tests fail only because all of the 
subflows got closed?  <--- (I am interested in the answer to this and 
having a discussion before making more code changes)

If peer A sends RM_ADDR 0 and all of the subflows are using local_id 0 on 
peer A's side, then it seems like it is expected that all of the subflows will 
close. We would want to test RM_ADDR 0 when there are subflows that will 
stay open because they do not use ID 0 on the side that is sending 
RM_ADDR. Is this what is going on with the tests or is there a different 
issue that is solved by the added code in this patch?

Thanks,

Mat


> 			pr_debug(" -> %s rm_list_ids[%d]=%u local_id=%u remote_id=%u",
> 				 type == RM_ADDR ? "address" : "subflow", i, rm_list.ids[i],
> 				 subflow->local_id, subflow->remote_id);
> -- 
> 2.29.2
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org
>

--
Mat Martineau
Intel

[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 2/3] mptcp: check the removing initial subflow

[Mat Martineau]

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

On Wed, 3 Mar 2021, Geliang Tang wrote:

> Hi Mat,
>
> Mat Martineau <mathew.j.martineau(a)linux.intel.com> 于2021年3月3日周三 上午9:44写道：
>>
>> On Tue, 2 Mar 2021, Geliang Tang wrote:
>>
>>> If the removing address id is 0, the initial subflow needs to be removed.
>>> Since the initial subflow's both local_id and remote_id are all zeros,
>>> this patch checks them to make sure it's now removing the right subflow.
>>>
>>> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
>>> ---
>>> net/mptcp/pm_netlink.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index eaac9824f444..439c9ee57566 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -629,6 +629,11 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
>>>                       if (rm_list.ids[i] != id)
>>>                               continue;
>>>
>>> +                     if (!rm_list.ids[i]) {
>>> +                             if (subflow->local_id || subflow->remote_id)
>>> +                                     continue;
>>> +                     }
>>> +
>>
>> In my reply to v2, I commented that I did not see the initial subflow
>> closed. But that does not mean that RM_ADDR 0 should only affect the
>> initial subflow. There may be other subflows involving that address id
>> that need to be closed.
>>
>> Without this commit, do some self tests fail only because all of the
>> subflows got closed?  <--- (I am interested in the answer to this and
>> having a discussion before making more code changes)
>
> When I ran the test in v2, some self tests got the poll timeout failures
> since all of the subflows got closed. When I ran the test in v3, without
> this commit, no self test fail.
>
> The original removing code can deal with id 0 address well, only the
> "break" in mptcp_pm_nl_rm_addr_or_subflow needs to be dropped. Since this
> "break" prevents the iteration to the next subflow involving that address.
> I fixed this in v4.

Ok, thanks for this information!


Mat

>
>>
>> If peer A sends RM_ADDR 0 and all of the subflows are using local_id 0 on
>> peer A's side, then it seems like it is expected that all of the subflows will
>> close. We would want to test RM_ADDR 0 when there are subflows that will
>> stay open because they do not use ID 0 on the side that is sending
>> RM_ADDR. Is this what is going on with the tests or is there a different
>> issue that is solved by the added code in this patch?
>
> I thought that removing id 0 was just removing the initial subflow, so I
> added this patch. As mentioned above, all the subflows involving the id 0
> address need to be removed. So this patch need to be dropped.
>
> Thanks.
>
> -Geliang
>
>>
>> Thanks,
>>
>> Mat
>>
>>
>>>                       pr_debug(" -> %s rm_list_ids[%d]=%u local_id=%u remote_id=%u",
>>>                                type == RM_ADDR ? "address" : "subflow", i, rm_list.ids[i],
>>>                                subflow->local_id, subflow->remote_id);
>>> --
>>> 2.29.2
>>> _______________________________________________
>>> mptcp mailing list -- mptcp(a)lists.01.org
>>> To unsubscribe send an email to mptcp-leave(a)lists.01.org
>>>
>>
>> --
>> Mat Martineau
>> Intel
>

--
Mat Martineau
Intel

[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 2/3] mptcp: check the removing initial subflow

[Geliang Tang]

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

Hi Mat,

Mat Martineau <mathew.j.martineau(a)linux.intel.com> 于2021年3月3日周三 上午9:44写道：
>
> On Tue, 2 Mar 2021, Geliang Tang wrote:
>
> > If the removing address id is 0, the initial subflow needs to be removed.
> > Since the initial subflow's both local_id and remote_id are all zeros,
> > this patch checks them to make sure it's now removing the right subflow.
> >
> > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > ---
> > net/mptcp/pm_netlink.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index eaac9824f444..439c9ee57566 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -629,6 +629,11 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
> >                       if (rm_list.ids[i] != id)
> >                               continue;
> >
> > +                     if (!rm_list.ids[i]) {
> > +                             if (subflow->local_id || subflow->remote_id)
> > +                                     continue;
> > +                     }
> > +
>
> In my reply to v2, I commented that I did not see the initial subflow
> closed. But that does not mean that RM_ADDR 0 should only affect the
> initial subflow. There may be other subflows involving that address id
> that need to be closed.
>
> Without this commit, do some self tests fail only because all of the
> subflows got closed?  <--- (I am interested in the answer to this and
> having a discussion before making more code changes)

When I ran the test in v2, some self tests got the poll timeout failures
since all of the subflows got closed. When I ran the test in v3, without
this commit, no self test fail.

The original removing code can deal with id 0 address well, only the
"break" in mptcp_pm_nl_rm_addr_or_subflow needs to be dropped. Since this
"break" prevents the iteration to the next subflow involving that address.
I fixed this in v4.

>
> If peer A sends RM_ADDR 0 and all of the subflows are using local_id 0 on
> peer A's side, then it seems like it is expected that all of the subflows will
> close. We would want to test RM_ADDR 0 when there are subflows that will
> stay open because they do not use ID 0 on the side that is sending
> RM_ADDR. Is this what is going on with the tests or is there a different
> issue that is solved by the added code in this patch?

I thought that removing id 0 was just removing the initial subflow, so I
added this patch. As mentioned above, all the subflows involving the id 0
address need to be removed. So this patch need to be dropped.

Thanks.

-Geliang

>
> Thanks,
>
> Mat
>
>
> >                       pr_debug(" -> %s rm_list_ids[%d]=%u local_id=%u remote_id=%u",
> >                                type == RM_ADDR ? "address" : "subflow", i, rm_list.ids[i],
> >                                subflow->local_id, subflow->remote_id);
> > --
> > 2.29.2
> > _______________________________________________
> > mptcp mailing list -- mptcp(a)lists.01.org
> > To unsubscribe send an email to mptcp-leave(a)lists.01.org
> >
>
> --
> Mat Martineau
> Intel