[MPTCP] Re: [PATCH] Squash-to: "net: mptcp: improve fallback to TCP"

[MPTCP] Re: [PATCH] Squash-to: "net: mptcp: improve fallback to TCP"

[Davide Caratti]

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

On Sat, 2020-06-27 at 09:50 +0200, Matthieu Baerts wrote:
> Tests are still OK (except the issue with Packetdrill)

hello,

FYI: the issue with packetdrill has been narrowed down to this commit:

commit e9c15badbb7b20ccdbadf5da14e0a68fbad51015
Author: Mel Gorman <mgorman(a)techsingularity.net>
Date:   Mon Jun 15 13:13:58 2020 +0100

    fs: Do not check if there is a fsnotify watcher on pseudo inodes

with this commit, all tests starting with lines like the following one,

0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
0.1 fcntl(3, F_GETFL) = 0x2 (flags O_RDWR)

are going to fail, regardless of the the value of 'protocol' in the call
to socket(family, type, protocol). The call to fcntl() will return O_RDWR
| O_NOATIME (that is 0x4000002, insted of just O_RDWR, hence the failure
below:

# cat prova.pkt
0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 fcntl(3, F_GETFL) = 0x2 (flags O_RDWR)
# ./packetdrill -vvv ./prova.pkt
socket syscall: 1593425866.359845
fcntl syscall: 1593425866.359899
./prova.pkt:2: runtime error in fcntl call: Expected result 2 but got 67108866 with errno 2 (No such file or directory)

(where 67108866 -> 0x4000002 -> O_RDWR | O_NOATIME).

the problem is common to all tests (both TCP and MPTCP). If we want to fix
this, we should not expect fcntl() to return a scalar value, but we should
accept all return values matching a bitmask.

Do you think it's worth a question to the official packetdrill mailing
list?

any feedback appreciated, thank you in advance!
-- 
davide

[MPTCP] Re: [PATCH] Squash-to: "net: mptcp: improve fallback to TCP"

[Matthieu Baerts]

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

Hi Paolo, Mat,

On 26/06/2020 18:17, Mat Martineau wrote:
> 
> On Fri, 26 Jun 2020, Paolo Abeni wrote:
> 
>> This fixes blocking accept() never waking-up.
>>
>> When blocking, mptcp_stream_accept() calls into:
>> ssock->ops->acceptk -> ssock->sk->sk_prot->accept ->
>> inet_csk_accept() -> inet_csk_wait_for_connect(),
>>
>> which in turns waits on ssock->sk->sk_wq, while
>> all signaling how happens on sock->sk->sk_wq.
>>
>> Since we ssock->sk->sk_wq is never used otherwise,
>> just copy sock->sk->sk_wq into it at first subflow
>> creatin time.
> 
> This fixes the problem for me - thanks Paolo!

Thank you for the patch and the review!

- 9d9ba398bf90: "squashed" in "net: mptcp: improve fallback to TCP"
- e3f1af462a5e: conflict in 
t/mptcp-create-first-subflow-at-msk-creation-time
- 0367023402da..e1929209d8a3: result

Tests are still OK (except the issue with Packetdrill)

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

[MPTCP] Re: [PATCH] Squash-to: "net: mptcp: improve fallback to TCP"

[Mat Martineau]

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


On Fri, 26 Jun 2020, Paolo Abeni wrote:

> This fixes blocking accept() never waking-up.
>
> When blocking, mptcp_stream_accept() calls into:
> ssock->ops->acceptk -> ssock->sk->sk_prot->accept ->
> inet_csk_accept() -> inet_csk_wait_for_connect(),
>
> which in turns waits on ssock->sk->sk_wq, while
> all signaling how happens on sock->sk->sk_wq.
>
> Since we ssock->sk->sk_wq is never used otherwise,
> just copy sock->sk->sk_wq into it at first subflow
> creatin time.
>
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/protocol.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 4d6d35e99d0f..5ae8952189d2 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -128,6 +128,10 @@ static struct socket *__mptcp_socket_create(struct mptcp_sock *msk, int state)
> 	list_add(&subflow->node, &msk->conn_list);
> 	subflow->request_mptcp = 1;
>
> +	/* accept() will wait on first subflow sk_wq, and we always wakes up
> +	 * via msk->sk_socket */
> +	RCU_INIT_POINTER(msk->first->sk_wq, &sk->sk_socket->wq);
> +
> set_state:
> 	if (state != MPTCP_SAME_STATE)
> 		inet_sk_state_store(sk, state);
> -- 
> 2.26.2

This fixes the problem for me - thanks Paolo!

--
Mat Martineau
Intel