Initiator: MSG_FASTOPEN in sendto triggers the mptcp_sendsmg_fastopen.
It requests a MPTFO cookie.
Suggestion @palbeni(JAN 17): 'split the patch in several small one'.
Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
---
net/mptcp/fastopen.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
net/mptcp/protocol.c | 4 ++--
net/mptcp/protocol.h | 6 ++++++
3 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index 0c9ef6f5d528..9974508e0f4c 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -3,3 +3,53 @@
*/
#include "protocol.h"
+
+int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
+ size_t len, struct mptcp_sock *msk,
+ size_t *copied)
+{
+ const struct iphdr *iph;
+ struct ubuf_info *uarg;
+ struct sockaddr *uaddr;
+ struct sk_buff *skb;
+ struct tcp_sock *tp;
+ struct socket *ssk;
+ int ret;
+
+ ssk = __mptcp_nmpc_socket(msk);
+ if (unlikely(!ssk))
+ goto out_EFAULT;
+ skb = tcp_stream_alloc_skb(ssk->sk, 0, ssk->sk->sk_allocation, true);
+ if (unlikely(!skb))
+ goto out_EFAULT;
+ iph = ip_hdr(skb);
+ if (unlikely(!iph))
+ goto out_EFAULT;
+ uarg = msg_zerocopy_realloc(sk, len, skb_zcopy(skb));
+ if (unlikely(!uarg))
+ goto out_EFAULT;
+ uaddr = msg->msg_name;
+
+ tp = tcp_sk(ssk->sk);
+ if (unlikely(!tp))
+ goto out_EFAULT;
+ if (!tp->fastopen_req)
+ tp->fastopen_req = kzalloc(sizeof(*tp->fastopen_req),
+ ssk->sk->sk_allocation);
+
+ if (unlikely(!tp->fastopen_req))
+ goto out_EFAULT;
+ tp->fastopen_req->data = msg;
+ tp->fastopen_req->size = len;
+ tp->fastopen_req->uarg = uarg;
+
+ /* requests a cookie */
+ ret = mptcp_stream_connect(sk->sk_socket, uaddr,
+ msg->msg_namelen, msg->msg_flags);
+ if (!ret)
+ *copied = len;
+ return ret;
+out_EFAULT:
+ ret = -EFAULT;
+ return ret;
+}
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0e5db0a634d3..af99a03021c9 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1672,9 +1672,9 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
int ret = 0;
long timeo;
- /* we don't support FASTOPEN yet */
+ /* we don't fully support FASTOPEN yet */
if (msg->msg_flags & MSG_FASTOPEN)
- return -EOPNOTSUPP;
+ ret = mptcp_sendmsg_fastopen(sk, msg, len, msk, &copied);
/* silently ignore everything else */
msg->msg_flags &= MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 1e21293bceaa..21f9bf6d2f7e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -837,6 +837,12 @@ void mptcp_event_addr_removed(const struct mptcp_sock *msk, u8 id);
bool mptcp_userspace_pm_active(const struct mptcp_sock *msk);
int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr, int addr_len, int flags);
+// Fast Open Mechanism functions begin
+int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
+ size_t len, struct mptcp_sock *msk,
+ size_t *copied);
+// Fast Open Mechanism functions end
+
static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk)
{
return READ_ONCE(msk->pm.addr_signal) &
--
2.25.1
On Sun, 2022-09-18 at 00:28 +0200, Dmytro Shytyi wrote:
> Initiator: MSG_FASTOPEN in sendto triggers the mptcp_sendsmg_fastopen.
> It requests a MPTFO cookie.
> Suggestion @palbeni(JAN 17): 'split the patch in several small one'.
Minor nit: the above line is not needed here. You can add this
"changelog" related info after the '---' separator, so that they will
not land in git.
>
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> ---
> net/mptcp/fastopen.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
> net/mptcp/protocol.c | 4 ++--
> net/mptcp/protocol.h | 6 ++++++
> 3 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
> index 0c9ef6f5d528..9974508e0f4c 100644
> --- a/net/mptcp/fastopen.c
> +++ b/net/mptcp/fastopen.c
> @@ -3,3 +3,53 @@
> */
>
> #include "protocol.h"
> +
> +int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
> + size_t len, struct mptcp_sock *msk,
No need to pass both 'sk' and 'msk', they are the same ptr casted to
different types. Just pass 'sk' and then:
struct mptcp_sock *msk = mptcp_sk(sk);
> + size_t *copied)
> +{
> + const struct iphdr *iph;
> + struct ubuf_info *uarg;
> + struct sockaddr *uaddr;
> + struct sk_buff *skb;
> + struct tcp_sock *tp;
> + struct socket *ssk;
The above variable name is misleading. 'ssk' should be a 'struct sock',
you should use 'ssock' for subflow 'struct socket'.
I think it's better to use a 'struct sock', you could do:
ssk = msk->first;
if (unlikely(ssk))
return -EINVAL;
> + int ret;
> +
> + ssk = __mptcp_nmpc_socket(msk);
> + if (unlikely(!ssk))
> + goto out_EFAULT;
See the above.
> + skb = tcp_stream_alloc_skb(ssk->sk, 0, ssk->sk->sk_allocation, true);
> + if (unlikely(!skb))
> + goto out_EFAULT;
> + iph = ip_hdr(skb);
> + if (unlikely(!iph))
> + goto out_EFAULT;
Use only lower case for macro names. Also EFAULT is probably not a good
return value. EINVAL should fit better. More importantly, it looks like
this check is not needed ?!?
> + uarg = msg_zerocopy_realloc(sk, len, skb_zcopy(skb));
We don't support zerocopy yet, so the above is not needed.
> + if (unlikely(!uarg))
> + goto out_EFAULT;
> + uaddr = msg->msg_name;
> +
> + tp = tcp_sk(ssk->sk);
simply:
tp = tcp_sk(ssk);
> + if (unlikely(!tp))
> + goto out_EFAULT;
Check not needed
> + if (!tp->fastopen_req)
> + tp->fastopen_req = kzalloc(sizeof(*tp->fastopen_req),
> + ssk->sk->sk_allocation);
> +
> + if (unlikely(!tp->fastopen_req))
> + goto out_EFAULT;
> + tp->fastopen_req->data = msg;
> + tp->fastopen_req->size = len;
> + tp->fastopen_req->uarg = uarg;
> +
> + /* requests a cookie */
> + ret = mptcp_stream_connect(sk->sk_socket, uaddr,
> + msg->msg_namelen, msg->msg_flags);
> + if (!ret)
> + *copied = len;
> + return ret;
> +out_EFAULT:
> + ret = -EFAULT;
> + return ret;
> +}
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0e5db0a634d3..af99a03021c9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1672,9 +1672,9 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> int ret = 0;
> long timeo;
>
> - /* we don't support FASTOPEN yet */
> + /* we don't fully support FASTOPEN yet */
> if (msg->msg_flags & MSG_FASTOPEN)
> - return -EOPNOTSUPP;
> + ret = mptcp_sendmsg_fastopen(sk, msg, len, msk, &copied);
likely:
return mptcp_sendmsg_fastopen(sk, msg, len, msk, &copied);
???
>
> /* silently ignore everything else */
> msg->msg_flags &= MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 1e21293bceaa..21f9bf6d2f7e 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -837,6 +837,12 @@ void mptcp_event_addr_removed(const struct
> mptcp_sock *msk, u8 id);
> bool mptcp_userspace_pm_active(const struct mptcp_sock *msk);
> int mptcp_stream_connect(struct socket *sock, struct sockaddr
> *uaddr, int addr_len, int flags);
>
> +// Fast Open Mechanism functions begin
> +int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
> + size_t len, struct mptcp_sock *msk,
> + size_t *copied);
> +// Fast Open Mechanism functions end
Plase, do not include the above comments, they are not needed and '//'
should not be used.
Thanks,
Paolo
On Mon, 2022-09-19 at 12:35 +0200, Paolo Abeni wrote:
> On Sun, 2022-09-18 at 00:28 +0200, Dmytro Shytyi wrote:
> > Initiator: MSG_FASTOPEN in sendto triggers the mptcp_sendsmg_fastopen.
> > It requests a MPTFO cookie.
> > Suggestion @palbeni(JAN 17): 'split the patch in several small one'.
>
> Minor nit: the above line is not needed here. You can add this
> "changelog" related info after the '---' separator, so that they will
> not land in git.
> >
> > Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> > ---
> > net/mptcp/fastopen.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
> > net/mptcp/protocol.c | 4 ++--
> > net/mptcp/protocol.h | 6 ++++++
> > 3 files changed, 58 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
> > index 0c9ef6f5d528..9974508e0f4c 100644
> > --- a/net/mptcp/fastopen.c
> > +++ b/net/mptcp/fastopen.c
> > @@ -3,3 +3,53 @@
> > */
> >
> > #include "protocol.h"
> > +
> > +int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
> > + size_t len, struct mptcp_sock *msk,
>
> No need to pass both 'sk' and 'msk', they are the same ptr casted to
> different types. Just pass 'sk' and then:
>
> struct mptcp_sock *msk = mptcp_sk(sk);
>
> > + size_t *copied)
> > +{
> > + const struct iphdr *iph;
> > + struct ubuf_info *uarg;
> > + struct sockaddr *uaddr;
> > + struct sk_buff *skb;
> > + struct tcp_sock *tp;
> > + struct socket *ssk;
>
> The above variable name is misleading. 'ssk' should be a 'struct sock',
> you should use 'ssock' for subflow 'struct socket'.
>
> I think it's better to use a 'struct sock', you could do:
>
> ssk = msk->first;
> if (unlikely(ssk))
> return -EINVAL;
>
> > + int ret;
> > +
> > + ssk = __mptcp_nmpc_socket(msk);
> > + if (unlikely(!ssk))
> > + goto out_EFAULT;
>
> See the above.
>
> > + skb = tcp_stream_alloc_skb(ssk->sk, 0, ssk->sk->sk_allocation, true);
> > + if (unlikely(!skb))
> > + goto out_EFAULT;
> > + iph = ip_hdr(skb);
> > + if (unlikely(!iph))
> > + goto out_EFAULT;
>
> Use only lower case for macro names. Also EFAULT is probably not a good
> return value. EINVAL should fit better. More importantly, it looks like
> this check is not needed ?!?
Addendum: you probably need to add/duplicate the full checks
implemented in tcp_sendmsg_fastopen():
https://elixir.bootlin.com/linux/v6.0-rc5/source/net/ipv4/tcp.c#L1174
lines 1174-1180
Cheers,
Paolo
Hi Paolo, Dmytro, On 19/09/2022 12:44, Paolo Abeni wrote: > On Mon, 2022-09-19 at 12:35 +0200, Paolo Abeni wrote: >> On Sun, 2022-09-18 at 00:28 +0200, Dmytro Shytyi wrote: (...) >>> + skb = tcp_stream_alloc_skb(ssk->sk, 0, ssk->sk->sk_allocation, true); >>> + if (unlikely(!skb)) >>> + goto out_EFAULT; >>> + iph = ip_hdr(skb); >>> + if (unlikely(!iph)) >>> + goto out_EFAULT; >> >> Use only lower case for macro names. Also EFAULT is probably not a good >> return value. EINVAL should fit better. More importantly, it looks like >> this check is not needed ?!? > > Addendum: you probably need to add/duplicate the full checks > implemented in tcp_sendmsg_fastopen(): > > https://elixir.bootlin.com/linux/v6.0-rc5/source/net/ipv4/tcp.c#L1174 > > lines 1174-1180 We discussed a bit about that at the last meeting and I think it might make more sense to re-use tcp_sendmsg_fastopen() like Benjamin did in his seris, see patch 2/10 and 3/10 from [1]. I think there were just some confusions here: it is not forbidden to modify TCP code. Of course we don't want modifications for MPTCP affecting TCP perf, especially the fast path but here, it makes sense to export some functions to be re-used in MPTCP instead of duplicating a big bunch of code. It is not needed in our case here, it is more a generic comment about MPTCP dev but I think we can also say that if needed, it might make sense to refactor some code from TCP to export a part of it, no? Cheers, Matt [1] https://lore.kernel.org/mptcp/20220908133829.3410092-1-benjamin.hesmans@tessares.net/T/ -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
Hello Paolo. Matthieu, Please, check my last commit please v8 . It *fully reuses the existing code.* As from the beginning my misunderstanging from the received feedback was that it is better to not impact the existing TCP code and I was forced to create hooks and reimplement some parts. As from now I see clear acceptance from majority of modification of the existing code (execpt of fastpath), I prepared a patch v8 that is fully reuse the exsiting code at maximum :) On 9/19/2022 1:22 PM, Matthieu Baerts wrote: > Hi Paolo, Dmytro, > > On 19/09/2022 12:44, Paolo Abeni wrote: >> On Mon, 2022-09-19 at 12:35 +0200, Paolo Abeni wrote: >>> On Sun, 2022-09-18 at 00:28 +0200, Dmytro Shytyi wrote: > (...) > >>>> + skb = tcp_stream_alloc_skb(ssk->sk, 0, ssk->sk->sk_allocation, true); >>>> + if (unlikely(!skb)) >>>> + goto out_EFAULT; >>>> + iph = ip_hdr(skb); >>>> + if (unlikely(!iph)) >>>> + goto out_EFAULT; >>> Use only lower case for macro names. Also EFAULT is probably not a good >>> return value. EINVAL should fit better. More importantly, it looks like >>> this check is not needed ?!? >> Addendum: you probably need to add/duplicate the full checks >> implemented in tcp_sendmsg_fastopen(): >> >> https://elixir.bootlin.com/linux/v6.0-rc5/source/net/ipv4/tcp.c#L1174 >> >> lines 1174-1180 > We discussed a bit about that at the last meeting and I think it might > make more sense to re-use tcp_sendmsg_fastopen() like Benjamin did in > his seris, see patch 2/10 and 3/10 from [1]. > > I think there were just some confusions here: it is not forbidden to > modify TCP code. Of course we don't want modifications for MPTCP > affecting TCP perf, especially the fast path but here, it makes sense to > export some functions to be re-used in MPTCP instead of duplicating a > big bunch of code. > > It is not needed in our case here, it is more a generic comment about > MPTCP dev but I think we can also say that if needed, it might make > sense to refactor some code from TCP to export a part of it, no? > > Cheers, > Matt > > [1] > https://lore.kernel.org/mptcp/20220908133829.3410092-1-benjamin.hesmans@tessares.net/T/
© 2016 - 2026 Red Hat, Inc.