| Message ID | 20220920125243.2880-4-dmytro@shytyi.net (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Paolo Abeni |
| Headers | show |
| Series | mptcp: Fast Open Mechanism | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 63 lines checked |
| matttbe/build | fail | Build error with: -Werror |
| matttbe/KVM_Validation__normal | warning | Unstable: 2 failed test(s): selftest_mptcp_join selftest_mptfo |
| matttbe/KVM_Validation__debug | warning | Unstable: 1 failed test(s): selftest_mptfo |
On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote: > In the following patches we will reuse modified tcp_sendmsg_fastopen(). > We call it from mptcp_sendmsg(). > > Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net> > --- > include/net/tcp.h | 3 +++ > net/ipv4/tcp.c | 18 +++++++++++++----- > net/mptcp/protocol.c | 11 +++++++++-- > 3 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 735e957f7f4b..a7d49e42470a 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -1754,6 +1754,9 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb, > struct request_sock *req, > struct tcp_fastopen_cookie *foc, > const struct dst_entry *dst); > +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, > + int *copied, size_t size, > + struct ubuf_info *uarg); > void tcp_fastopen_init_key_once(struct net *net); > bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss, > struct tcp_fastopen_cookie *cookie); > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 9251c99d3cfd..d10a3cdae220 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -280,6 +280,9 @@ > #include <asm/ioctls.h> > #include <net/busy_poll.h> > > +#include <net/mptcp.h> > +#include "../mptcp/protocol.h" > + > /* Track pending CMSGs. */ > enum { > TCP_CMSG_INQ = 1, > @@ -1162,9 +1165,9 @@ void tcp_free_fastopen_req(struct tcp_sock *tp) > } > } > > -static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, > - int *copied, size_t size, > - struct ubuf_info *uarg) > +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, > + int *copied, size_t size, > + struct ubuf_info *uarg) > { > struct tcp_sock *tp = tcp_sk(sk); > struct inet_sock *inet = inet_sk(sk); > @@ -1197,8 +1200,13 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, > } > } > flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0; > - err = __inet_stream_connect(sk->sk_socket, uaddr, > - msg->msg_namelen, flags, 1); > + if (!sk_is_mptcp(sk)) > + err = __inet_stream_connect(sk->sk_socket, uaddr, > + msg->msg_namelen, flags, 1); > + else > + err = mptcp_stream_connect(sk->sk_socket, uaddr, > + msg->msg_namelen, msg->msg_flags); I guess the goal of the above change is let mptcp_stream_connect() update the msk socket status, is that correct? However there are a few problems with lock: you must acquite the subflow socket lock before calling tcp_sendmsg_fastopen() - or will see subflow state corruption - but that in turn will cause a deadlock as mptcp_stream_connect() acquires the msk socket lock and then the subflow socket lock. I think it's better leave the tcp_sendmsg_fastopen() body unchanged... > + > /* fastopen_req could already be freed in __inet_stream_connect > * if the connection times out or gets rst > */ > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 470045793181..8cf307e4e59c 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -1673,9 +1673,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) > long timeo; > > /* we don't support FASTOPEN yet */ > - if (msg->msg_flags & MSG_FASTOPEN) > - return -EOPNOTSUPP; > + if (msg->msg_flags & MSG_FASTOPEN) { > + struct socket *ssock = __mptcp_nmpc_socket(msk); ... acquire the subflow socket lock here... > > + if (ssock) { > + int copied_syn_fastopen = 0; > + > + ret = tcp_sendmsg_fastopen(ssock->sk, msg, &copied_syn_fastopen, len, NULL); > + copied += copied_syn_fastopen; > + } ... and additionally handle the sock state update here. Possibly you can encapsulate all the fastopen code in a new function - say __mptcp_sendmsg_fastopen(), as it will be called under the msk socket lock. Side note: you should enter the fastopen branch even when inet_sk(ssock->sk)->defer_connect is set Cheers, Paolo
Hi Paolo, Dmytro, On 20/09/2022 16:36, Paolo Abeni wrote: > On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote: >> In the following patches we will reuse modified tcp_sendmsg_fastopen(). >> We call it from mptcp_sendmsg(). >> >> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net> >> --- >> include/net/tcp.h | 3 +++ >> net/ipv4/tcp.c | 18 +++++++++++++----- >> net/mptcp/protocol.c | 11 +++++++++-- >> 3 files changed, 25 insertions(+), 7 deletions(-) >> >> diff --git a/include/net/tcp.h b/include/net/tcp.h >> index 735e957f7f4b..a7d49e42470a 100644 >> --- a/include/net/tcp.h >> +++ b/include/net/tcp.h >> @@ -1754,6 +1754,9 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb, >> struct request_sock *req, >> struct tcp_fastopen_cookie *foc, >> const struct dst_entry *dst); >> +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >> + int *copied, size_t size, >> + struct ubuf_info *uarg); >> void tcp_fastopen_init_key_once(struct net *net); >> bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss, >> struct tcp_fastopen_cookie *cookie); >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >> index 9251c99d3cfd..d10a3cdae220 100644 >> --- a/net/ipv4/tcp.c >> +++ b/net/ipv4/tcp.c >> @@ -280,6 +280,9 @@ >> #include <asm/ioctls.h> >> #include <net/busy_poll.h> >> >> +#include <net/mptcp.h> >> +#include "../mptcp/protocol.h" >> + >> /* Track pending CMSGs. */ >> enum { >> TCP_CMSG_INQ = 1, >> @@ -1162,9 +1165,9 @@ void tcp_free_fastopen_req(struct tcp_sock *tp) >> } >> } >> >> -static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >> - int *copied, size_t size, >> - struct ubuf_info *uarg) >> +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >> + int *copied, size_t size, >> + struct ubuf_info *uarg) >> { >> struct tcp_sock *tp = tcp_sk(sk); >> struct inet_sock *inet = inet_sk(sk); >> @@ -1197,8 +1200,13 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >> } >> } >> flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0; >> - err = __inet_stream_connect(sk->sk_socket, uaddr, >> - msg->msg_namelen, flags, 1); >> + if (!sk_is_mptcp(sk)) >> + err = __inet_stream_connect(sk->sk_socket, uaddr, >> + msg->msg_namelen, flags, 1); >> + else >> + err = mptcp_stream_connect(sk->sk_socket, uaddr, >> + msg->msg_namelen, msg->msg_flags); > > > I guess the goal of the above change is let mptcp_stream_connect() > update the msk socket status, is that correct? > > However there are a few problems with lock: you must acquite the > subflow socket lock before calling tcp_sendmsg_fastopen() - or will see > subflow state corruption - but that in turn will cause a deadlock as > mptcp_stream_connect() acquires the msk socket lock and then the > subflow socket lock. > > I think it's better leave the tcp_sendmsg_fastopen() body unchanged... > >> + >> /* fastopen_req could already be freed in __inet_stream_connect >> * if the connection times out or gets rst >> */ >> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >> index 470045793181..8cf307e4e59c 100644 >> --- a/net/mptcp/protocol.c >> +++ b/net/mptcp/protocol.c >> @@ -1673,9 +1673,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) >> long timeo; >> >> /* we don't support FASTOPEN yet */ >> - if (msg->msg_flags & MSG_FASTOPEN) >> - return -EOPNOTSUPP; >> + if (msg->msg_flags & MSG_FASTOPEN) { >> + struct socket *ssock = __mptcp_nmpc_socket(msk); > > ... acquire the subflow socket lock here... > >> >> + if (ssock) { >> + int copied_syn_fastopen = 0; >> + >> + ret = tcp_sendmsg_fastopen(ssock->sk, msg, &copied_syn_fastopen, len, NULL); >> + copied += copied_syn_fastopen; >> + } > > ... and additionally handle the sock state update here. Possibly you > can encapsulate all the fastopen code in a new function - say > __mptcp_sendmsg_fastopen(), as it will be called under the msk socket > lock. > > > Side note: you should enter the fastopen branch even when > inet_sk(ssock->sk)->defer_connect is set I think we should better discuss about that at the next meeting because all items you are asking here is what Benjamin did in [1]: https://patchwork.kernel.org/project/mptcp/patch/20220908133829.3410092-4-benjamin.hesmans@tessares.net/ The work from Dmytro helped Benjamin to start looking at that and propose another approach. Before the meeting, we can look at creating a series focussed on the sending part taking patches from both Benjamin and Dmytro using Benjamin's approach if that's OK. Of course both Dmytro and Benjamin will be credited to have worked on that. Cheers, Matt
Hi Matthieu, My previous patches had locks in this function (mptcp_sendmsg()) also and code was generated much earlier. If you add locks in the mptcp _sendmsg() in current context it will lead to deadlock. I temporarly avoided adding locks as I need to carefully think about this. Best, Dmytro. On 9/20/2022 5:02 PM, Matthieu Baerts wrote: > Hi Paolo, Dmytro, > > On 20/09/2022 16:36, Paolo Abeni wrote: >> On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote: >>> In the following patches we will reuse modified tcp_sendmsg_fastopen(). >>> We call it from mptcp_sendmsg(). >>> >>> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net> >>> --- >>> include/net/tcp.h | 3 +++ >>> net/ipv4/tcp.c | 18 +++++++++++++----- >>> net/mptcp/protocol.c | 11 +++++++++-- >>> 3 files changed, 25 insertions(+), 7 deletions(-) >>> >>> diff --git a/include/net/tcp.h b/include/net/tcp.h >>> index 735e957f7f4b..a7d49e42470a 100644 >>> --- a/include/net/tcp.h >>> +++ b/include/net/tcp.h >>> @@ -1754,6 +1754,9 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb, >>> struct request_sock *req, >>> struct tcp_fastopen_cookie *foc, >>> const struct dst_entry *dst); >>> +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >>> + int *copied, size_t size, >>> + struct ubuf_info *uarg); >>> void tcp_fastopen_init_key_once(struct net *net); >>> bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss, >>> struct tcp_fastopen_cookie *cookie); >>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >>> index 9251c99d3cfd..d10a3cdae220 100644 >>> --- a/net/ipv4/tcp.c >>> +++ b/net/ipv4/tcp.c >>> @@ -280,6 +280,9 @@ >>> #include <asm/ioctls.h> >>> #include <net/busy_poll.h> >>> >>> +#include <net/mptcp.h> >>> +#include "../mptcp/protocol.h" >>> + >>> /* Track pending CMSGs. */ >>> enum { >>> TCP_CMSG_INQ = 1, >>> @@ -1162,9 +1165,9 @@ void tcp_free_fastopen_req(struct tcp_sock *tp) >>> } >>> } >>> >>> -static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >>> - int *copied, size_t size, >>> - struct ubuf_info *uarg) >>> +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >>> + int *copied, size_t size, >>> + struct ubuf_info *uarg) >>> { >>> struct tcp_sock *tp = tcp_sk(sk); >>> struct inet_sock *inet = inet_sk(sk); >>> @@ -1197,8 +1200,13 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >>> } >>> } >>> flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0; >>> - err = __inet_stream_connect(sk->sk_socket, uaddr, >>> - msg->msg_namelen, flags, 1); >>> + if (!sk_is_mptcp(sk)) >>> + err = __inet_stream_connect(sk->sk_socket, uaddr, >>> + msg->msg_namelen, flags, 1); >>> + else >>> + err = mptcp_stream_connect(sk->sk_socket, uaddr, >>> + msg->msg_namelen, msg->msg_flags); >> >> I guess the goal of the above change is let mptcp_stream_connect() >> update the msk socket status, is that correct? >> >> However there are a few problems with lock: you must acquite the >> subflow socket lock before calling tcp_sendmsg_fastopen() - or will see >> subflow state corruption - but that in turn will cause a deadlock as >> mptcp_stream_connect() acquires the msk socket lock and then the >> subflow socket lock. >> >> I think it's better leave the tcp_sendmsg_fastopen() body unchanged... >> >>> + >>> /* fastopen_req could already be freed in __inet_stream_connect >>> * if the connection times out or gets rst >>> */ >>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >>> index 470045793181..8cf307e4e59c 100644 >>> --- a/net/mptcp/protocol.c >>> +++ b/net/mptcp/protocol.c >>> @@ -1673,9 +1673,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) >>> long timeo; >>> >>> /* we don't support FASTOPEN yet */ >>> - if (msg->msg_flags & MSG_FASTOPEN) >>> - return -EOPNOTSUPP; >>> + if (msg->msg_flags & MSG_FASTOPEN) { >>> + struct socket *ssock = __mptcp_nmpc_socket(msk); >> ... acquire the subflow socket lock here... >> >>> >>> + if (ssock) { >>> + int copied_syn_fastopen = 0; >>> + >>> + ret = tcp_sendmsg_fastopen(ssock->sk, msg, &copied_syn_fastopen, len, NULL); >>> + copied += copied_syn_fastopen; >>> + } >> ... and additionally handle the sock state update here. Possibly you >> can encapsulate all the fastopen code in a new function - say >> __mptcp_sendmsg_fastopen(), as it will be called under the msk socket >> lock. >> >> >> Side note: you should enter the fastopen branch even when >> inet_sk(ssock->sk)->defer_connect is set > I think we should better discuss about that at the next meeting because > all items you are asking here is what Benjamin did in [1]: > > https://patchwork.kernel.org/project/mptcp/patch/20220908133829.3410092-4-benjamin.hesmans@tessares.net/ > > The work from Dmytro helped Benjamin to start looking at that and > propose another approach. Before the meeting, we can look at creating a > series focussed on the sending part taking patches from both Benjamin > and Dmytro using Benjamin's approach if that's OK. Of course both Dmytro > and Benjamin will be credited to have worked on that. > > Cheers, > Matt
On Tue, 2022-09-20 at 17:02 +0200, Matthieu Baerts wrote: > Hi Paolo, Dmytro, > > On 20/09/2022 16:36, Paolo Abeni wrote: > > On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote: > > > In the following patches we will reuse modified tcp_sendmsg_fastopen(). > > > We call it from mptcp_sendmsg(). > > > > > > Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net> > > > --- > > > include/net/tcp.h | 3 +++ > > > net/ipv4/tcp.c | 18 +++++++++++++----- > > > net/mptcp/protocol.c | 11 +++++++++-- > > > 3 files changed, 25 insertions(+), 7 deletions(-) > > > > > > diff --git a/include/net/tcp.h b/include/net/tcp.h > > > index 735e957f7f4b..a7d49e42470a 100644 > > > --- a/include/net/tcp.h > > > +++ b/include/net/tcp.h > > > @@ -1754,6 +1754,9 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb, > > > struct request_sock *req, > > > struct tcp_fastopen_cookie *foc, > > > const struct dst_entry *dst); > > > +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, > > > + int *copied, size_t size, > > > + struct ubuf_info *uarg); > > > void tcp_fastopen_init_key_once(struct net *net); > > > bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss, > > > struct tcp_fastopen_cookie *cookie); > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > > > index 9251c99d3cfd..d10a3cdae220 100644 > > > --- a/net/ipv4/tcp.c > > > +++ b/net/ipv4/tcp.c > > > @@ -280,6 +280,9 @@ > > > #include <asm/ioctls.h> > > > #include <net/busy_poll.h> > > > > > > +#include <net/mptcp.h> > > > +#include "../mptcp/protocol.h" > > > + > > > /* Track pending CMSGs. */ > > > enum { > > > TCP_CMSG_INQ = 1, > > > @@ -1162,9 +1165,9 @@ void tcp_free_fastopen_req(struct tcp_sock *tp) > > > } > > > } > > > > > > -static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, > > > - int *copied, size_t size, > > > - struct ubuf_info *uarg) > > > +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, > > > + int *copied, size_t size, > > > + struct ubuf_info *uarg) > > > { > > > struct tcp_sock *tp = tcp_sk(sk); > > > struct inet_sock *inet = inet_sk(sk); > > > @@ -1197,8 +1200,13 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, > > > } > > > } > > > flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0; > > > - err = __inet_stream_connect(sk->sk_socket, uaddr, > > > - msg->msg_namelen, flags, 1); > > > + if (!sk_is_mptcp(sk)) > > > + err = __inet_stream_connect(sk->sk_socket, uaddr, > > > + msg->msg_namelen, flags, 1); > > > + else > > > + err = mptcp_stream_connect(sk->sk_socket, uaddr, > > > + msg->msg_namelen, msg->msg_flags); > > > > > > I guess the goal of the above change is let mptcp_stream_connect() > > update the msk socket status, is that correct? > > > > However there are a few problems with lock: you must acquite the > > subflow socket lock before calling tcp_sendmsg_fastopen() - or will see > > subflow state corruption - but that in turn will cause a deadlock as > > mptcp_stream_connect() acquires the msk socket lock and then the > > subflow socket lock. > > > > I think it's better leave the tcp_sendmsg_fastopen() body unchanged... > > > > > + > > > /* fastopen_req could already be freed in __inet_stream_connect > > > * if the connection times out or gets rst > > > */ > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > > index 470045793181..8cf307e4e59c 100644 > > > --- a/net/mptcp/protocol.c > > > +++ b/net/mptcp/protocol.c > > > @@ -1673,9 +1673,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) > > > long timeo; > > > > > > /* we don't support FASTOPEN yet */ > > > - if (msg->msg_flags & MSG_FASTOPEN) > > > - return -EOPNOTSUPP; > > > + if (msg->msg_flags & MSG_FASTOPEN) { > > > + struct socket *ssock = __mptcp_nmpc_socket(msk); > > > > ... acquire the subflow socket lock here... > > > > > > > > + if (ssock) { > > > + int copied_syn_fastopen = 0; > > > + > > > + ret = tcp_sendmsg_fastopen(ssock->sk, msg, &copied_syn_fastopen, len, NULL); > > > + copied += copied_syn_fastopen; > > > + } > > > > ... and additionally handle the sock state update here. Possibly you > > can encapsulate all the fastopen code in a new function - say > > __mptcp_sendmsg_fastopen(), as it will be called under the msk socket > > lock. > > > > > > Side note: you should enter the fastopen branch even when > > inet_sk(ssock->sk)->defer_connect is set > > I think we should better discuss about that at the next meeting because > all items you are asking here is what Benjamin did in [1]: > > https://patchwork.kernel.org/project/mptcp/patch/20220908133829.3410092-4-benjamin.hesmans@tessares.net/ Almost. The above lacks the msk socket state update. Cheers, Paolo
On 9/20/2022 4:36 PM, Paolo Abeni wrote: > On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote: >> In the following patches we will reuse modified tcp_sendmsg_fastopen(). >> We call it from mptcp_sendmsg(). >> >> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net> >> --- >> include/net/tcp.h | 3 +++ >> net/ipv4/tcp.c | 18 +++++++++++++----- >> net/mptcp/protocol.c | 11 +++++++++-- >> 3 files changed, 25 insertions(+), 7 deletions(-) >> >> diff --git a/include/net/tcp.h b/include/net/tcp.h >> index 735e957f7f4b..a7d49e42470a 100644 >> --- a/include/net/tcp.h >> +++ b/include/net/tcp.h >> @@ -1754,6 +1754,9 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb, >> struct request_sock *req, >> struct tcp_fastopen_cookie *foc, >> const struct dst_entry *dst); >> +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >> + int *copied, size_t size, >> + struct ubuf_info *uarg); >> void tcp_fastopen_init_key_once(struct net *net); >> bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss, >> struct tcp_fastopen_cookie *cookie); >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >> index 9251c99d3cfd..d10a3cdae220 100644 >> --- a/net/ipv4/tcp.c >> +++ b/net/ipv4/tcp.c >> @@ -280,6 +280,9 @@ >> #include <asm/ioctls.h> >> #include <net/busy_poll.h> >> >> +#include <net/mptcp.h> >> +#include "../mptcp/protocol.h" >> + >> /* Track pending CMSGs. */ >> enum { >> TCP_CMSG_INQ = 1, >> @@ -1162,9 +1165,9 @@ void tcp_free_fastopen_req(struct tcp_sock *tp) >> } >> } >> >> -static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >> - int *copied, size_t size, >> - struct ubuf_info *uarg) >> +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >> + int *copied, size_t size, >> + struct ubuf_info *uarg) >> { >> struct tcp_sock *tp = tcp_sk(sk); >> struct inet_sock *inet = inet_sk(sk); >> @@ -1197,8 +1200,13 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, >> } >> } >> flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0; >> - err = __inet_stream_connect(sk->sk_socket, uaddr, >> - msg->msg_namelen, flags, 1); >> + if (!sk_is_mptcp(sk)) >> + err = __inet_stream_connect(sk->sk_socket, uaddr, >> + msg->msg_namelen, flags, 1); >> + else >> + err = mptcp_stream_connect(sk->sk_socket, uaddr, >> + msg->msg_namelen, msg->msg_flags); > > I guess the goal of the above change is let mptcp_stream_connect() > update the msk socket status, is that correct? > > However there are a few problems with lock: you must acquite the > subflow socket lock before calling tcp_sendmsg_fastopen() - or will see > subflow state corruption - but that in turn will cause a deadlock as > mptcp_stream_connect() acquires the msk socket lock and then the > subflow socket lock. > > I think it's better leave the tcp_sendmsg_fastopen() body unchanged... In v9 I still modify the tcp_sendmsg_fastopen() a little bit with adding some locks. >> + >> /* fastopen_req could already be freed in __inet_stream_connect >> * if the connection times out or gets rst >> */ >> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >> index 470045793181..8cf307e4e59c 100644 >> --- a/net/mptcp/protocol.c >> +++ b/net/mptcp/protocol.c >> @@ -1673,9 +1673,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) >> long timeo; >> >> /* we don't support FASTOPEN yet */ >> - if (msg->msg_flags & MSG_FASTOPEN) >> - return -EOPNOTSUPP; >> + if (msg->msg_flags & MSG_FASTOPEN) { >> + struct socket *ssock = __mptcp_nmpc_socket(msk); > ... acquire the subflow socket lock here... Some locks are acquired in v9. >> >> + if (ssock) { >> + int copied_syn_fastopen = 0; >> + >> + ret = tcp_sendmsg_fastopen(ssock->sk, msg, &copied_syn_fastopen, len, NULL); >> + copied += copied_syn_fastopen; >> + } > ... and additionally handle the sock state update here. Possibly you > can encapsulate all the fastopen code in a new function - say > __mptcp_sendmsg_fastopen(), as it will be called under the msk socket > lock. > > > Side note: you should enter the fastopen branch even when > inet_sk(ssock->sk)->defer_connect is set I tried to add this (defer_connect) in v9, but didn't check if it works. > Cheers, > > Paolo Best, Dmytro.
diff --git a/include/net/tcp.h b/include/net/tcp.h index 735e957f7f4b..a7d49e42470a 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -1754,6 +1754,9 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb, struct request_sock *req, struct tcp_fastopen_cookie *foc, const struct dst_entry *dst); +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, + int *copied, size_t size, + struct ubuf_info *uarg); void tcp_fastopen_init_key_once(struct net *net); bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss, struct tcp_fastopen_cookie *cookie); diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 9251c99d3cfd..d10a3cdae220 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -280,6 +280,9 @@ #include <asm/ioctls.h> #include <net/busy_poll.h> +#include <net/mptcp.h> +#include "../mptcp/protocol.h" + /* Track pending CMSGs. */ enum { TCP_CMSG_INQ = 1, @@ -1162,9 +1165,9 @@ void tcp_free_fastopen_req(struct tcp_sock *tp) } } -static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, - int *copied, size_t size, - struct ubuf_info *uarg) +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, + int *copied, size_t size, + struct ubuf_info *uarg) { struct tcp_sock *tp = tcp_sk(sk); struct inet_sock *inet = inet_sk(sk); @@ -1197,8 +1200,13 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, } } flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0; - err = __inet_stream_connect(sk->sk_socket, uaddr, - msg->msg_namelen, flags, 1); + if (!sk_is_mptcp(sk)) + err = __inet_stream_connect(sk->sk_socket, uaddr, + msg->msg_namelen, flags, 1); + else + err = mptcp_stream_connect(sk->sk_socket, uaddr, + msg->msg_namelen, msg->msg_flags); + /* fastopen_req could already be freed in __inet_stream_connect * if the connection times out or gets rst */ diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 470045793181..8cf307e4e59c 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1673,9 +1673,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) long timeo; /* we don't support FASTOPEN yet */ - if (msg->msg_flags & MSG_FASTOPEN) - return -EOPNOTSUPP; + if (msg->msg_flags & MSG_FASTOPEN) { + struct socket *ssock = __mptcp_nmpc_socket(msk); + if (ssock) { + int copied_syn_fastopen = 0; + + ret = tcp_sendmsg_fastopen(ssock->sk, msg, &copied_syn_fastopen, len, NULL); + copied += copied_syn_fastopen; + } + } /* silently ignore everything else */ msg->msg_flags &= MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL;
In the following patches we will reuse modified tcp_sendmsg_fastopen(). We call it from mptcp_sendmsg(). Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net> --- include/net/tcp.h | 3 +++ net/ipv4/tcp.c | 18 +++++++++++++----- net/mptcp/protocol.c | 11 +++++++++-- 3 files changed, 25 insertions(+), 7 deletions(-)