* [PATCH mptcp-next] mptcp: factor out mptcp_connect()
@ 2022-10-03 15:59 Paolo Abeni
2022-10-03 16:51 ` Dmytro Shytyi
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Paolo Abeni @ 2022-10-03 15:59 UTC (permalink / raw)
To: mptcp; +Cc: Dmytro Shytyi
The current MPTCP connect implementation duplicates a bit of inet
code and does not use nor provide a struct proto->connect callback,
which in turn will not fit the upcoming fastopen implementation.
Refactor such implementation to use the common helper, moving the
MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
indirect call to the subflow connect callback.
Note that the fastopen call-path invokes mptcp_connect() while already
holding the subflow socket lock. Explicitly keep track of such path
via a new MPTCP-level flag and handle the locking accordingly.
Additionally, track the connect flags in a new msk field to allow
propagating them to the subflow inet_stream_connect call.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 128 +++++++++++++++++++++----------------------
net/mptcp/protocol.h | 4 +-
2 files changed, 65 insertions(+), 67 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 8feb684408f7..6245248d0182 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1689,7 +1689,10 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
lock_sock(ssk);
+ msk->connect_flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
+ msk->is_sendmsg = 1;
ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
+ msk->is_sendmsg = 0;
copied += copied_syn;
if (ret == -EINPROGRESS && copied_syn > 0) {
/* reflect the new state on the MPTCP socket */
@@ -3506,10 +3509,65 @@ static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
return put_user(answ, (int __user *)arg);
}
+static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
+ struct mptcp_subflow_context *subflow)
+{
+ subflow->request_mptcp = 0;
+ __mptcp_do_fallback(msk);
+}
+
+static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
+{
+ struct mptcp_subflow_context *subflow;
+ struct mptcp_sock *msk = mptcp_sk(sk);
+ struct socket *ssock;
+ int err = -EINVAL;
+
+ ssock = __mptcp_nmpc_socket(msk);
+ if (!ssock)
+ return -EINVAL;
+
+ mptcp_token_destroy(msk);
+ inet_sk_state_store(sk, TCP_SYN_SENT);
+ subflow = mptcp_subflow_ctx(ssock->sk);
+#ifdef CONFIG_TCP_MD5SIG
+ /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
+ * TCP option space.
+ */
+ if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
+ mptcp_subflow_early_fallback(msk, subflow);
+#endif
+ if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
+ MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
+ mptcp_subflow_early_fallback(msk, subflow);
+ }
+ if (likely(!__mptcp_check_fallback(msk)))
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
+
+ /* if reaching here via the fastopen/sendmsg path, the caller already
+ * acquired the subflow socket lock, too.
+ */
+ if (msk->is_sendmsg)
+ err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
+ else
+ err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
+ inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
+
+ /* on successful connect, the msk state will be moved to established by
+ * subflow_finish_connect()
+ */
+ if (!err || err == -EINPROGRESS)
+ mptcp_copy_inaddrs(sk, ssock->sk);
+ else
+ inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
+ return err;
+}
+
static struct proto mptcp_prot = {
.name = "MPTCP",
.owner = THIS_MODULE,
.init = mptcp_init_sock,
+ .connect = mptcp_connect,
.disconnect = mptcp_disconnect,
.close = mptcp_close,
.accept = mptcp_accept,
@@ -3561,78 +3619,16 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
return err;
}
-static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
- struct mptcp_subflow_context *subflow)
-{
- subflow->request_mptcp = 0;
- __mptcp_do_fallback(msk);
-}
-
static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
int addr_len, int flags)
{
- struct mptcp_sock *msk = mptcp_sk(sock->sk);
- struct mptcp_subflow_context *subflow;
- struct socket *ssock;
- int err = -EINVAL;
+ int ret;
lock_sock(sock->sk);
- if (uaddr) {
- if (addr_len < sizeof(uaddr->sa_family))
- goto unlock;
-
- if (uaddr->sa_family == AF_UNSPEC) {
- err = mptcp_disconnect(sock->sk, flags);
- sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
- goto unlock;
- }
- }
-
- if (sock->state != SS_UNCONNECTED && msk->subflow) {
- /* pending connection or invalid state, let existing subflow
- * cope with that
- */
- ssock = msk->subflow;
- goto do_connect;
- }
-
- ssock = __mptcp_nmpc_socket(msk);
- if (!ssock)
- goto unlock;
-
- mptcp_token_destroy(msk);
- inet_sk_state_store(sock->sk, TCP_SYN_SENT);
- subflow = mptcp_subflow_ctx(ssock->sk);
-#ifdef CONFIG_TCP_MD5SIG
- /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
- * TCP option space.
- */
- if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
- mptcp_subflow_early_fallback(msk, subflow);
-#endif
- if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
- MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
- mptcp_subflow_early_fallback(msk, subflow);
- }
- if (likely(!__mptcp_check_fallback(msk)))
- MPTCP_INC_STATS(sock_net(sock->sk), MPTCP_MIB_MPCAPABLEACTIVE);
-
-do_connect:
- err = ssock->ops->connect(ssock, uaddr, addr_len, flags);
- inet_sk(sock->sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
- sock->state = ssock->state;
-
- /* on successful connect, the msk state will be moved to established by
- * subflow_finish_connect()
- */
- if (!err || err == -EINPROGRESS)
- mptcp_copy_inaddrs(sock->sk, ssock->sk);
- else
- inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
-
-unlock:
+ mptcp_sk(sock->sk)->connect_flags = flags;
+ ret = __inet_stream_connect(sock, uaddr, addr_len, flags, 0);
release_sock(sock->sk);
- return err;
+ return ret;
}
static int mptcp_listen(struct socket *sock, int backlog)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 93c535440a5c..18f866b1afda 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -285,7 +285,9 @@ struct mptcp_sock {
u8 mpc_endpoint_id;
u8 recvmsg_inq:1,
cork:1,
- nodelay:1;
+ nodelay:1,
+ is_sendmsg:1;
+ int connect_flags;
struct work_struct work;
struct sk_buff *ooo_last_skb;
struct rb_root out_of_order_queue;
--
2.37.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-next] mptcp: factor out mptcp_connect()
2022-10-03 15:59 [PATCH mptcp-next] mptcp: factor out mptcp_connect() Paolo Abeni
@ 2022-10-03 16:51 ` Dmytro Shytyi
2022-10-03 22:30 ` Mat Martineau
2022-10-06 14:54 ` mptcp: factor out mptcp_connect(): Tests Results MPTCP CI
2 siblings, 0 replies; 6+ messages in thread
From: Dmytro Shytyi @ 2022-10-03 16:51 UTC (permalink / raw)
To: Paolo Abeni, mptcp
Thank you, Paolo!
I will rebase Fast Open Mechanism patches, after these patches will be
merged to the tree.
Best,
Dmytro.
On 10/3/2022 5:59 PM, Paolo Abeni wrote:
> not fit the upcoming fastopen implementation.
>
> Refactor such implementation to use the common helper, moving the
> MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
> indirect call to the subflow connect callback.
>
> Note that the fastopen call-path invokes mptcp_connect() while already
> holding the subflow socket lock. Explicitly keep track of such path
> via a new MPTCP-level flag and handle the locking accordingly.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-next] mptcp: factor out mptcp_connect()
2022-10-03 15:59 [PATCH mptcp-next] mptcp: factor out mptcp_connect() Paolo Abeni
2022-10-03 16:51 ` Dmytro Shytyi
@ 2022-10-03 22:30 ` Mat Martineau
2022-10-04 14:36 ` Paolo Abeni
2022-10-06 14:54 ` mptcp: factor out mptcp_connect(): Tests Results MPTCP CI
2 siblings, 1 reply; 6+ messages in thread
From: Mat Martineau @ 2022-10-03 22:30 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp, Dmytro Shytyi
On Mon, 3 Oct 2022, Paolo Abeni wrote:
> The current MPTCP connect implementation duplicates a bit of inet
> code and does not use nor provide a struct proto->connect callback,
> which in turn will not fit the upcoming fastopen implementation.
>
> Refactor such implementation to use the common helper, moving the
> MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
> indirect call to the subflow connect callback.
>
> Note that the fastopen call-path invokes mptcp_connect() while already
> holding the subflow socket lock. Explicitly keep track of such path
> via a new MPTCP-level flag and handle the locking accordingly.
>
> Additionally, track the connect flags in a new msk field to allow
> propagating them to the subflow inet_stream_connect call.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 128 +++++++++++++++++++++----------------------
> net/mptcp/protocol.h | 4 +-
> 2 files changed, 65 insertions(+), 67 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 8feb684408f7..6245248d0182 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1689,7 +1689,10 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>
> lock_sock(ssk);
>
> + msk->connect_flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
> + msk->is_sendmsg = 1;
> ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
> + msk->is_sendmsg = 0;
I'm not excited about passing the flags this way (new msk members), but it
seems better than changing the parameters of (struct proto_ops)->connect
(and every connect function it's used with). So, I'm ok with the above
change.
> copied += copied_syn;
> if (ret == -EINPROGRESS && copied_syn > 0) {
> /* reflect the new state on the MPTCP socket */
> @@ -3506,10 +3509,65 @@ static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
> return put_user(answ, (int __user *)arg);
> }
>
> +static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
> + struct mptcp_subflow_context *subflow)
> +{
> + subflow->request_mptcp = 0;
> + __mptcp_do_fallback(msk);
> +}
> +
> +static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
> +{
> + struct mptcp_subflow_context *subflow;
> + struct mptcp_sock *msk = mptcp_sk(sk);
> + struct socket *ssock;
> + int err = -EINVAL;
> +
> + ssock = __mptcp_nmpc_socket(msk);
> + if (!ssock)
> + return -EINVAL;
> +
> + mptcp_token_destroy(msk);
> + inet_sk_state_store(sk, TCP_SYN_SENT);
> + subflow = mptcp_subflow_ctx(ssock->sk);
> +#ifdef CONFIG_TCP_MD5SIG
> + /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> + * TCP option space.
> + */
> + if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
> + mptcp_subflow_early_fallback(msk, subflow);
> +#endif
> + if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
> + MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
> + mptcp_subflow_early_fallback(msk, subflow);
> + }
> + if (likely(!__mptcp_check_fallback(msk)))
> + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
> +
> + /* if reaching here via the fastopen/sendmsg path, the caller already
> + * acquired the subflow socket lock, too.
> + */
> + if (msk->is_sendmsg)
> + err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
> + else
> + err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
> + inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
> +
> + /* on successful connect, the msk state will be moved to established by
> + * subflow_finish_connect()
> + */
> + if (!err || err == -EINPROGRESS)
> + mptcp_copy_inaddrs(sk, ssock->sk);
> + else
> + inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
> + return err;
> +}
> +
> static struct proto mptcp_prot = {
> .name = "MPTCP",
> .owner = THIS_MODULE,
> .init = mptcp_init_sock,
> + .connect = mptcp_connect,
> .disconnect = mptcp_disconnect,
> .close = mptcp_close,
> .accept = mptcp_accept,
> @@ -3561,78 +3619,16 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> return err;
> }
>
> -static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
> - struct mptcp_subflow_context *subflow)
> -{
> - subflow->request_mptcp = 0;
> - __mptcp_do_fallback(msk);
> -}
> -
> static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> int addr_len, int flags)
> {
> - struct mptcp_sock *msk = mptcp_sk(sock->sk);
> - struct mptcp_subflow_context *subflow;
> - struct socket *ssock;
> - int err = -EINVAL;
> + int ret;
>
> lock_sock(sock->sk);
> - if (uaddr) {
> - if (addr_len < sizeof(uaddr->sa_family))
> - goto unlock;
> -
> - if (uaddr->sa_family == AF_UNSPEC) {
> - err = mptcp_disconnect(sock->sk, flags);
> - sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
> - goto unlock;
> - }
> - }
> -
> - if (sock->state != SS_UNCONNECTED && msk->subflow) {
> - /* pending connection or invalid state, let existing subflow
> - * cope with that
> - */
> - ssock = msk->subflow;
> - goto do_connect;
> - }
I don't see a similar chunk of code added back in elsewhere in this patch.
After looking at the commit message for 41be81a8d3d0 ("mptcp: fix
unblocking connect()"), I'm not sure the nonblocking connect is still
handled? I see where __inet_stream_connect() will not call mptcp_connect()
with SS_UNCONNECTED, but it's not obvious why it's no longer important to
handle this scenario any more. Could you clarify the reasoning for this in
the commit message if the nonblocking connect is handled some other way
now?
- Mat
> -
> - ssock = __mptcp_nmpc_socket(msk);
> - if (!ssock)
> - goto unlock;
> -
> - mptcp_token_destroy(msk);
> - inet_sk_state_store(sock->sk, TCP_SYN_SENT);
> - subflow = mptcp_subflow_ctx(ssock->sk);
> -#ifdef CONFIG_TCP_MD5SIG
> - /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> - * TCP option space.
> - */
> - if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
> - mptcp_subflow_early_fallback(msk, subflow);
> -#endif
> - if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
> - MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
> - mptcp_subflow_early_fallback(msk, subflow);
> - }
> - if (likely(!__mptcp_check_fallback(msk)))
> - MPTCP_INC_STATS(sock_net(sock->sk), MPTCP_MIB_MPCAPABLEACTIVE);
> -
> -do_connect:
> - err = ssock->ops->connect(ssock, uaddr, addr_len, flags);
> - inet_sk(sock->sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
> - sock->state = ssock->state;
> -
> - /* on successful connect, the msk state will be moved to established by
> - * subflow_finish_connect()
> - */
> - if (!err || err == -EINPROGRESS)
> - mptcp_copy_inaddrs(sock->sk, ssock->sk);
> - else
> - inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
> -
> -unlock:
> + mptcp_sk(sock->sk)->connect_flags = flags;
> + ret = __inet_stream_connect(sock, uaddr, addr_len, flags, 0);
> release_sock(sock->sk);
> - return err;
> + return ret;
> }
>
> static int mptcp_listen(struct socket *sock, int backlog)
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 93c535440a5c..18f866b1afda 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -285,7 +285,9 @@ struct mptcp_sock {
> u8 mpc_endpoint_id;
> u8 recvmsg_inq:1,
> cork:1,
> - nodelay:1;
> + nodelay:1,
> + is_sendmsg:1;
> + int connect_flags;
> struct work_struct work;
> struct sk_buff *ooo_last_skb;
> struct rb_root out_of_order_queue;
> --
> 2.37.3
>
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-next] mptcp: factor out mptcp_connect()
2022-10-03 22:30 ` Mat Martineau
@ 2022-10-04 14:36 ` Paolo Abeni
2022-10-04 16:03 ` Mat Martineau
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2022-10-04 14:36 UTC (permalink / raw)
To: Mat Martineau; +Cc: mptcp, Dmytro Shytyi
On Mon, 2022-10-03 at 15:30 -0700, Mat Martineau wrote:
> On Mon, 3 Oct 2022, Paolo Abeni wrote:
>
> > The current MPTCP connect implementation duplicates a bit of inet
> > code and does not use nor provide a struct proto->connect callback,
> > which in turn will not fit the upcoming fastopen implementation.
> >
> > Refactor such implementation to use the common helper, moving the
> > MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
> > indirect call to the subflow connect callback.
> >
> > Note that the fastopen call-path invokes mptcp_connect() while already
> > holding the subflow socket lock. Explicitly keep track of such path
> > via a new MPTCP-level flag and handle the locking accordingly.
> >
> > Additionally, track the connect flags in a new msk field to allow
> > propagating them to the subflow inet_stream_connect call.
> >
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/protocol.c | 128 +++++++++++++++++++++----------------------
> > net/mptcp/protocol.h | 4 +-
> > 2 files changed, 65 insertions(+), 67 deletions(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 8feb684408f7..6245248d0182 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1689,7 +1689,10 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >
> > lock_sock(ssk);
> >
> > + msk->connect_flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
> > + msk->is_sendmsg = 1;
> > ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
> > + msk->is_sendmsg = 0;
>
> I'm not excited about passing the flags this way (new msk members), but it
> seems better than changing the parameters of (struct proto_ops)->connect
> (and every connect function it's used with). So, I'm ok with the above
> change.
I agree it's not very nice. Perhpas it can be made less ugly moving the
msk->is_sendmsg = 0;
in mptcp_stream_connect()?
> > copied += copied_syn;
> > if (ret == -EINPROGRESS && copied_syn > 0) {
> > /* reflect the new state on the MPTCP socket */
> > @@ -3506,10 +3509,65 @@ static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
> > return put_user(answ, (int __user *)arg);
> > }
> >
> > +static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
> > + struct mptcp_subflow_context *subflow)
> > +{
> > + subflow->request_mptcp = 0;
> > + __mptcp_do_fallback(msk);
> > +}
> > +
> > +static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
> > +{
> > + struct mptcp_subflow_context *subflow;
> > + struct mptcp_sock *msk = mptcp_sk(sk);
> > + struct socket *ssock;
> > + int err = -EINVAL;
> > +
> > + ssock = __mptcp_nmpc_socket(msk);
> > + if (!ssock)
> > + return -EINVAL;
> > +
> > + mptcp_token_destroy(msk);
> > + inet_sk_state_store(sk, TCP_SYN_SENT);
> > + subflow = mptcp_subflow_ctx(ssock->sk);
> > +#ifdef CONFIG_TCP_MD5SIG
> > + /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> > + * TCP option space.
> > + */
> > + if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
> > + mptcp_subflow_early_fallback(msk, subflow);
> > +#endif
> > + if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
> > + MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
> > + mptcp_subflow_early_fallback(msk, subflow);
> > + }
> > + if (likely(!__mptcp_check_fallback(msk)))
> > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
> > +
> > + /* if reaching here via the fastopen/sendmsg path, the caller already
> > + * acquired the subflow socket lock, too.
> > + */
> > + if (msk->is_sendmsg)
> > + err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
> > + else
> > + err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
> > + inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
> > +
> > + /* on successful connect, the msk state will be moved to established by
> > + * subflow_finish_connect()
> > + */
> > + if (!err || err == -EINPROGRESS)
> > + mptcp_copy_inaddrs(sk, ssock->sk);
> > + else
> > + inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
> > + return err;
> > +}
> > +
> > static struct proto mptcp_prot = {
> > .name = "MPTCP",
> > .owner = THIS_MODULE,
> > .init = mptcp_init_sock,
> > + .connect = mptcp_connect,
> > .disconnect = mptcp_disconnect,
> > .close = mptcp_close,
> > .accept = mptcp_accept,
> > @@ -3561,78 +3619,16 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> > return err;
> > }
> >
> > -static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
> > - struct mptcp_subflow_context *subflow)
> > -{
> > - subflow->request_mptcp = 0;
> > - __mptcp_do_fallback(msk);
> > -}
> > -
> > static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> > int addr_len, int flags)
> > {
> > - struct mptcp_sock *msk = mptcp_sk(sock->sk);
> > - struct mptcp_subflow_context *subflow;
> > - struct socket *ssock;
> > - int err = -EINVAL;
> > + int ret;
> >
> > lock_sock(sock->sk);
> > - if (uaddr) {
> > - if (addr_len < sizeof(uaddr->sa_family))
> > - goto unlock;
> > -
> > - if (uaddr->sa_family == AF_UNSPEC) {
> > - err = mptcp_disconnect(sock->sk, flags);
> > - sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
> > - goto unlock;
> > - }
> > - }
> > -
> > - if (sock->state != SS_UNCONNECTED && msk->subflow) {
> > - /* pending connection or invalid state, let existing subflow
> > - * cope with that
> > - */
> > - ssock = msk->subflow;
> > - goto do_connect;
> > - }
>
> I don't see a similar chunk of code added back in elsewhere in this patch.
>
> After looking at the commit message for 41be81a8d3d0 ("mptcp: fix
> unblocking connect()"), I'm not sure the nonblocking connect is still
> handled? I see where __inet_stream_connect() will not call mptcp_connect()
> with SS_UNCONNECTED, but it's not obvious why it's no longer important to
> handle this scenario any more. Could you clarify the reasoning for this in
> the commit message if the nonblocking connect is handled some other way
> now?
Thanks for noticing this, I underlooked this chunk of code. It looks
unblocking connect still works, as the relevant pktdrill tests pass,
and I'm adding a new one adding more coverage of the relevant code
path.
The main point is that before this commit we called (indirectly) into
inet_stream_connect() to update the 'struct socket' state on both the
subflow and the msk. That did _not_ cause a call to the subflow
tcp_v{4,6}_connect() callback.
Now the msk' struct socket is update directly by
mptcp_stream_connect/inet_stream_connect. We possibly miss/lack the
state update on the ssk struct socket, but it looks harmless.
Anyhow I'll send a v2 with more comments and the change mentioned
above.
Thank,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-next] mptcp: factor out mptcp_connect()
2022-10-04 14:36 ` Paolo Abeni
@ 2022-10-04 16:03 ` Mat Martineau
0 siblings, 0 replies; 6+ messages in thread
From: Mat Martineau @ 2022-10-04 16:03 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp, Dmytro Shytyi
On Tue, 4 Oct 2022, Paolo Abeni wrote:
> On Mon, 2022-10-03 at 15:30 -0700, Mat Martineau wrote:
>> On Mon, 3 Oct 2022, Paolo Abeni wrote:
>>
>>> The current MPTCP connect implementation duplicates a bit of inet
>>> code and does not use nor provide a struct proto->connect callback,
>>> which in turn will not fit the upcoming fastopen implementation.
>>>
>>> Refactor such implementation to use the common helper, moving the
>>> MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
>>> indirect call to the subflow connect callback.
>>>
>>> Note that the fastopen call-path invokes mptcp_connect() while already
>>> holding the subflow socket lock. Explicitly keep track of such path
>>> via a new MPTCP-level flag and handle the locking accordingly.
>>>
>>> Additionally, track the connect flags in a new msk field to allow
>>> propagating them to the subflow inet_stream_connect call.
>>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> net/mptcp/protocol.c | 128 +++++++++++++++++++++----------------------
>>> net/mptcp/protocol.h | 4 +-
>>> 2 files changed, 65 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 8feb684408f7..6245248d0182 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -1689,7 +1689,10 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>>>
>>> lock_sock(ssk);
>>>
>>> + msk->connect_flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
>>> + msk->is_sendmsg = 1;
>>> ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
>>> + msk->is_sendmsg = 0;
>>
>> I'm not excited about passing the flags this way (new msk members), but it
>> seems better than changing the parameters of (struct proto_ops)->connect
>> (and every connect function it's used with). So, I'm ok with the above
>> change.
>
> I agree it's not very nice. Perhpas it can be made less ugly moving the
>
> msk->is_sendmsg = 0;
>
> in mptcp_stream_connect()?
>
I hadn't thought to look at that - but after some consideration, I think
it fits here (in mptcp_sendmsg()) better, since fastopen is the "unusual"
case.
>>> copied += copied_syn;
>>> if (ret == -EINPROGRESS && copied_syn > 0) {
>>> /* reflect the new state on the MPTCP socket */
>>> @@ -3506,10 +3509,65 @@ static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
>>> return put_user(answ, (int __user *)arg);
>>> }
>>>
>>> +static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
>>> + struct mptcp_subflow_context *subflow)
>>> +{
>>> + subflow->request_mptcp = 0;
>>> + __mptcp_do_fallback(msk);
>>> +}
>>> +
>>> +static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>>> +{
>>> + struct mptcp_subflow_context *subflow;
>>> + struct mptcp_sock *msk = mptcp_sk(sk);
>>> + struct socket *ssock;
>>> + int err = -EINVAL;
>>> +
>>> + ssock = __mptcp_nmpc_socket(msk);
>>> + if (!ssock)
>>> + return -EINVAL;
>>> +
>>> + mptcp_token_destroy(msk);
>>> + inet_sk_state_store(sk, TCP_SYN_SENT);
>>> + subflow = mptcp_subflow_ctx(ssock->sk);
>>> +#ifdef CONFIG_TCP_MD5SIG
>>> + /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
>>> + * TCP option space.
>>> + */
>>> + if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
>>> + mptcp_subflow_early_fallback(msk, subflow);
>>> +#endif
>>> + if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
>>> + MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
>>> + mptcp_subflow_early_fallback(msk, subflow);
>>> + }
>>> + if (likely(!__mptcp_check_fallback(msk)))
>>> + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
>>> +
>>> + /* if reaching here via the fastopen/sendmsg path, the caller already
>>> + * acquired the subflow socket lock, too.
>>> + */
>>> + if (msk->is_sendmsg)
>>> + err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
>>> + else
>>> + err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
>>> + inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
>>> +
>>> + /* on successful connect, the msk state will be moved to established by
>>> + * subflow_finish_connect()
>>> + */
>>> + if (!err || err == -EINPROGRESS)
>>> + mptcp_copy_inaddrs(sk, ssock->sk);
>>> + else
>>> + inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
>>> + return err;
>>> +}
>>> +
>>> static struct proto mptcp_prot = {
>>> .name = "MPTCP",
>>> .owner = THIS_MODULE,
>>> .init = mptcp_init_sock,
>>> + .connect = mptcp_connect,
>>> .disconnect = mptcp_disconnect,
>>> .close = mptcp_close,
>>> .accept = mptcp_accept,
>>> @@ -3561,78 +3619,16 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>>> return err;
>>> }
>>>
>>> -static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
>>> - struct mptcp_subflow_context *subflow)
>>> -{
>>> - subflow->request_mptcp = 0;
>>> - __mptcp_do_fallback(msk);
>>> -}
>>> -
>>> static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
>>> int addr_len, int flags)
>>> {
>>> - struct mptcp_sock *msk = mptcp_sk(sock->sk);
>>> - struct mptcp_subflow_context *subflow;
>>> - struct socket *ssock;
>>> - int err = -EINVAL;
>>> + int ret;
>>>
>>> lock_sock(sock->sk);
>>> - if (uaddr) {
>>> - if (addr_len < sizeof(uaddr->sa_family))
>>> - goto unlock;
>>> -
>>> - if (uaddr->sa_family == AF_UNSPEC) {
>>> - err = mptcp_disconnect(sock->sk, flags);
>>> - sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
>>> - goto unlock;
>>> - }
>>> - }
>>> -
>>> - if (sock->state != SS_UNCONNECTED && msk->subflow) {
>>> - /* pending connection or invalid state, let existing subflow
>>> - * cope with that
>>> - */
>>> - ssock = msk->subflow;
>>> - goto do_connect;
>>> - }
>>
>> I don't see a similar chunk of code added back in elsewhere in this patch.
>>
>> After looking at the commit message for 41be81a8d3d0 ("mptcp: fix
>> unblocking connect()"), I'm not sure the nonblocking connect is still
>> handled? I see where __inet_stream_connect() will not call mptcp_connect()
>> with SS_UNCONNECTED, but it's not obvious why it's no longer important to
>> handle this scenario any more. Could you clarify the reasoning for this in
>> the commit message if the nonblocking connect is handled some other way
>> now?
>
> Thanks for noticing this, I underlooked this chunk of code. It looks
> unblocking connect still works, as the relevant pktdrill tests pass,
> and I'm adding a new one adding more coverage of the relevant code
> path.
>
> The main point is that before this commit we called (indirectly) into
> inet_stream_connect() to update the 'struct socket' state on both the
> subflow and the msk. That did _not_ cause a call to the subflow
> tcp_v{4,6}_connect() callback.
>
> Now the msk' struct socket is update directly by
> mptcp_stream_connect/inet_stream_connect. We possibly miss/lack the
> state update on the ssk struct socket, but it looks harmless.
>
That explanation helps, thank you!
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mptcp: factor out mptcp_connect(): Tests Results
2022-10-03 15:59 [PATCH mptcp-next] mptcp: factor out mptcp_connect() Paolo Abeni
2022-10-03 16:51 ` Dmytro Shytyi
2022-10-03 22:30 ` Mat Martineau
@ 2022-10-06 14:54 ` MPTCP CI
2 siblings, 0 replies; 6+ messages in thread
From: MPTCP CI @ 2022-10-06 14:54 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal:
- Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
- Task: https://cirrus-ci.com/task/5080762767638528
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5080762767638528/summary/summary.txt
- KVM Validation: debug:
- Success! ✅:
- Task: https://cirrus-ci.com/task/6206662674481152
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6206662674481152/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/56fd8c59c652
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-debug
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-06 14:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-03 15:59 [PATCH mptcp-next] mptcp: factor out mptcp_connect() Paolo Abeni
2022-10-03 16:51 ` Dmytro Shytyi
2022-10-03 22:30 ` Mat Martineau
2022-10-04 14:36 ` Paolo Abeni
2022-10-04 16:03 ` Mat Martineau
2022-10-06 14:54 ` mptcp: factor out mptcp_connect(): Tests Results MPTCP CI
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.