1. Patchew
  2. MPTCP
  3. [PATCH mptcp-net v6 00/11] mptcp: pm: more fixes around the ID 0
  4. View patch

[PATCH mptcp-net v6 05/11] mptcp: pm: send ACK on an active subflow

Matthieu Baerts (NGI0) posted 11 patches 4 months ago
There is a newer version of this series
[PATCH mptcp-net v6 05/11] mptcp: pm: send ACK on an active subflow
Posted by Matthieu Baerts (NGI0) 4 months ago 
Taking the first one on the list doesn't work in some cases, e.g. if the
initial subflow is being removed. Pick another one instead of not
sending anything.

Fixes: 84dfe3677a6f ("mptcp: send out dedicated ADD_ADDR packet")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
  - We could also check if the subflow is not "staled", but probably
    best to do that for net-next only.
---
 net/mptcp/pm_netlink.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 4000de54c99c..e25cda3909fa 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -768,9 +768,12 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
 	    !mptcp_pm_should_rm_signal(msk))
 		return;
 
-	subflow = list_first_entry_or_null(&msk->conn_list, typeof(*subflow), node);
-	if (subflow)
-		mptcp_pm_send_ack(msk, subflow, false, false);
+	mptcp_for_each_subflow(msk, subflow) {
+		if (__mptcp_subflow_active(subflow)) {
+			mptcp_pm_send_ack(msk, subflow, false, false);
+			break;
+		}
+	}
 }
 
 int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,

-- 
2.45.2
Re: [PATCH mptcp-net v6 05/11] mptcp: pm: send ACK on an active subflow
Posted by Mat Martineau 3 months, 4 weeks ago 
On Fri, 2 Aug 2024, Matthieu Baerts (NGI0) wrote:

> Taking the first one on the list doesn't work in some cases, e.g. if the
> initial subflow is being removed. Pick another one instead of not
> sending anything.
>
> Fixes: 84dfe3677a6f ("mptcp: send out dedicated ADD_ADDR packet")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>  - We could also check if the subflow is not "staled", but probably
>    best to do that for net-next only.
> ---
> net/mptcp/pm_netlink.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 4000de54c99c..e25cda3909fa 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -768,9 +768,12 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
> 	    !mptcp_pm_should_rm_signal(msk))
> 		return;
>
> -	subflow = list_first_entry_or_null(&msk->conn_list, typeof(*subflow), node);
> -	if (subflow)
> -		mptcp_pm_send_ack(msk, subflow, false, false);
> +	mptcp_for_each_subflow(msk, subflow) {
> +		if (__mptcp_subflow_active(subflow)) {
> +			mptcp_pm_send_ack(msk, subflow, false, false);
> +			break;
> +		}
> +	}

Hi Matthieu -

If all subflows are stale, it should still try to send on one (falling 
back to the original first entry seems as good as anything else?)

- Mat

> }
>
> int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
>
> -- 
> 2.45.2
>
>
Re: [PATCH mptcp-net v6 05/11] mptcp: pm: send ACK on an active subflow
Posted by Matthieu Baerts 3 months, 4 weeks ago 
Hi Mat,

Thank you for the review!

On 07/08/2024 00:29, Mat Martineau wrote:
> On Fri, 2 Aug 2024, Matthieu Baerts (NGI0) wrote:
> 
>> Taking the first one on the list doesn't work in some cases, e.g. if the
>> initial subflow is being removed. Pick another one instead of not
>> sending anything.
>>
>> Fixes: 84dfe3677a6f ("mptcp: send out dedicated ADD_ADDR packet")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Notes:
>>  - We could also check if the subflow is not "staled", but probably
>>    best to do that for net-next only.
>> ---
>> net/mptcp/pm_netlink.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index 4000de54c99c..e25cda3909fa 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -768,9 +768,12 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock
>> *msk)
>>         !mptcp_pm_should_rm_signal(msk))
>>         return;
>>
>> -    subflow = list_first_entry_or_null(&msk->conn_list,
>> typeof(*subflow), node);
>> -    if (subflow)
>> -        mptcp_pm_send_ack(msk, subflow, false, false);
>> +    mptcp_for_each_subflow(msk, subflow) {
>> +        if (__mptcp_subflow_active(subflow)) {
>> +            mptcp_pm_send_ack(msk, subflow, false, false);
>> +            break;
>> +        }
>> +    }
> 
> Hi Matthieu -
> 
> If all subflows are stale, it should still try to send on one (falling
> back to the original first entry seems as good as anything else?)

Yes, good point, probably something I should do in patch 11/11.

But here, 'stale' is not checked. Probably the __mptcp_subflow_active()
helper doesn't have a good name. It is only checking the state (fully
established, and not closed):


> static inline bool __tcp_can_send(const struct sock *ssk)
> {
> 	/* only send if our side has not closed yet */
> 	return ((1 << inet_sk_state_load(ssk)) & (TCPF_ESTABLISHED | TCPF_CLOSE_WAIT));
> }
> 
> static inline bool __mptcp_subflow_active(struct mptcp_subflow_context *subflow)
> {
> 	/* can't send if JOIN hasn't completed yet (i.e. is usable for mptcp) */
> 	if (subflow->request_join && !subflow->fully_established)
> 		return false;
> 
> 	return __tcp_can_send(mptcp_subflow_tcp_sock(subflow));
> }


If __mptcp_subflow_active() returns 'false', we cannot send the
ADD_ADDR. So I think we are good like that, no?

We can drop the patch 11/11 for the moment.

> 
> - Mat
> 
>> }
>>
>> int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
>>
>> -- 
>> 2.45.2
>>
>>

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.
Re: [PATCH mptcp-net v6 05/11] mptcp: pm: send ACK on an active subflow
Posted by Mat Martineau 3 months, 4 weeks ago 
On Wed, 7 Aug 2024, Matthieu Baerts wrote:

> Hi Mat,
>
> Thank you for the review!
>
> On 07/08/2024 00:29, Mat Martineau wrote:
>> On Fri, 2 Aug 2024, Matthieu Baerts (NGI0) wrote:
>>
>>> Taking the first one on the list doesn't work in some cases, e.g. if the
>>> initial subflow is being removed. Pick another one instead of not
>>> sending anything.
>>>
>>> Fixes: 84dfe3677a6f ("mptcp: send out dedicated ADD_ADDR packet")
>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>> ---
>>> Notes:
>>>  - We could also check if the subflow is not "staled", but probably
>>>    best to do that for net-next only.
>>> ---
>>> net/mptcp/pm_netlink.c | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index 4000de54c99c..e25cda3909fa 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -768,9 +768,12 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock
>>> *msk)
>>>         !mptcp_pm_should_rm_signal(msk))
>>>         return;
>>>
>>> -    subflow = list_first_entry_or_null(&msk->conn_list,
>>> typeof(*subflow), node);
>>> -    if (subflow)
>>> -        mptcp_pm_send_ack(msk, subflow, false, false);
>>> +    mptcp_for_each_subflow(msk, subflow) {
>>> +        if (__mptcp_subflow_active(subflow)) {
>>> +            mptcp_pm_send_ack(msk, subflow, false, false);
>>> +            break;
>>> +        }
>>> +    }
>>
>> Hi Matthieu -
>>
>> If all subflows are stale, it should still try to send on one (falling
>> back to the original first entry seems as good as anything else?)
>
> Yes, good point, probably something I should do in patch 11/11.
>
> But here, 'stale' is not checked. Probably the __mptcp_subflow_active()
> helper doesn't have a good name. It is only checking the state (fully
> established, and not closed):
>

I think I looked at mptcp_subflow_active() and saw the stale check there - 
sorry about that. This check should be fine as it is.

- Mat

>
>> static inline bool __tcp_can_send(const struct sock *ssk)
>> {
>> 	/* only send if our side has not closed yet */
>> 	return ((1 << inet_sk_state_load(ssk)) & (TCPF_ESTABLISHED | TCPF_CLOSE_WAIT));
>> }
>>
>> static inline bool __mptcp_subflow_active(struct mptcp_subflow_context *subflow)
>> {
>> 	/* can't send if JOIN hasn't completed yet (i.e. is usable for mptcp) */
>> 	if (subflow->request_join && !subflow->fully_established)
>> 		return false;
>>
>> 	return __tcp_can_send(mptcp_subflow_tcp_sock(subflow));
>> }
>
>
> If __mptcp_subflow_active() returns 'false', we cannot send the
> ADD_ADDR. So I think we are good like that, no?
>
> We can drop the patch 11/11 for the moment.
>
>>
>> - Mat
>>
>>> }
>>>
>>> int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
>>>
>>> --
??>> 2.45.2
>>>
>>>
>
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.
>
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.