* [MPTCP] Re: [MPTCP][PATCH v6 mptcp-next 2/9] mptcp: set the listening socket's subflow
@ 2020-11-25 9:24 Paolo Abeni
0 siblings, 0 replies; 5+ messages in thread
From: Paolo Abeni @ 2020-11-25 9:24 UTC (permalink / raw
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2831 bytes --]
On Wed, 2020-11-25 at 11:01 +0800, Geliang Tang wrote:
> This patch set the subflow context of the listening socket.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> net/mptcp/pm_netlink.c | 5 +++++
> net/mptcp/protocol.h | 1 +
> net/mptcp/subflow.c | 37 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 43 insertions(+)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 0d9a3c77fdf1..39a91fcac403 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -330,6 +330,11 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> if (mptcp_pm_alloc_anno_list(msk, local)) {
> msk->pm.add_addr_signaled++;
> mptcp_pm_announce_addr(msk, &local->addr, false, local->addr.port);
> + if (local->addr.port) {
> + spin_unlock_bh(&msk->pm.lock);
> + mptcp_pm_set_listen_socket_subflow(sk, local->lsk);
Why it's needed to this at announce time? Why not at lsk creation time?
Probably more related to the previous patch, why lsk is TCP socket
instead of an MPTCP one? (I feel the latter would be a more natural
choice, but it's just a feeling).
> + spin_lock_bh(&msk->pm.lock);
> + }
> mptcp_pm_nl_add_addr_send_ack(msk);
> }
> } else {
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index f202e22058b5..38001b7b0bf6 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -469,6 +469,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
> int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock);
> void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
> struct sockaddr_storage *addr);
> +int mptcp_pm_set_listen_socket_subflow(struct sock *sk, struct socket *sf);
>
> static inline void mptcp_subflow_tcp_fallback(struct sock *sk,
> struct mptcp_subflow_context *ctx)
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index d4c136f1e754..6053a88c176f 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1208,6 +1208,43 @@ int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock)
> return 0;
> }
>
> +int mptcp_pm_set_listen_socket_subflow(struct sock *sk, struct socket *sf)
> +{
> + struct mptcp_subflow_context *subflow;
> + struct net *net = sock_net(sk);
> + int err;
> +
> + if (!sf)
> + return -EINVAL;
> +
> + lock_sock(sf->sk);
> +
> + mptcp_attach_cgroup(sk, sf->sk);
This will likely not work if there are multiple MPTCP sockets using
this endpoint.
Possibly the cgroup attaching should be done at subflow_syn_recv_sock()
time, for the child socket, with some custom code - because it must
override the existing cgroup inherited from the listener.
/P
^ permalink raw reply [flat|nested] 5+ messages in thread
* [MPTCP] Re: [MPTCP][PATCH v6 mptcp-next 2/9] mptcp: set the listening socket's subflow
@ 2020-11-25 10:20 Geliang Tang
0 siblings, 0 replies; 5+ messages in thread
From: Geliang Tang @ 2020-11-25 10:20 UTC (permalink / raw
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 3817 bytes --]
Hi Paolo,
Paolo Abeni <pabeni(a)redhat.com> 于2020年11月25日周三 下午5:25写道：
>
> On Wed, 2020-11-25 at 11:01 +0800, Geliang Tang wrote:
> > This patch set the subflow context of the listening socket.
> >
> > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > ---
> > net/mptcp/pm_netlink.c | 5 +++++
> > net/mptcp/protocol.h | 1 +
> > net/mptcp/subflow.c | 37 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 43 insertions(+)
> >
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 0d9a3c77fdf1..39a91fcac403 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -330,6 +330,11 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> > if (mptcp_pm_alloc_anno_list(msk, local)) {
> > msk->pm.add_addr_signaled++;
> > mptcp_pm_announce_addr(msk, &local->addr, false, local->addr.port);
> > + if (local->addr.port) {
> > + spin_unlock_bh(&msk->pm.lock);
> > + mptcp_pm_set_listen_socket_subflow(sk, local->lsk);
>
> Why it's needed to this at announce time? Why not at lsk creation time?
> Probably more related to the previous patch, why lsk is TCP socket
> instead of an MPTCP one? (I feel the latter would be a more natural
> choice, but it's just a feeling).
I think the listening socket lsk is a subsocket. It should be joined to
the msk as a subflow. lsk is created in PM netlink as a TCP socket, since
this moment, the msk is not created.
At announce time, the msk is fully established, here I set up lsk's subflow
context, and add it into the msk as a subflow.
If lsk is an MPTCP socket, how can it to be joined to the msk?
-Geliang
>
> > + spin_lock_bh(&msk->pm.lock);
> > + }
> > mptcp_pm_nl_add_addr_send_ack(msk);
> > }
> > } else {
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index f202e22058b5..38001b7b0bf6 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -469,6 +469,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
> > int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock);
> > void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
> > struct sockaddr_storage *addr);
> > +int mptcp_pm_set_listen_socket_subflow(struct sock *sk, struct socket *sf);
> >
> > static inline void mptcp_subflow_tcp_fallback(struct sock *sk,
> > struct mptcp_subflow_context *ctx)
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index d4c136f1e754..6053a88c176f 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -1208,6 +1208,43 @@ int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock)
> > return 0;
> > }
> >
> > +int mptcp_pm_set_listen_socket_subflow(struct sock *sk, struct socket *sf)
> > +{
> > + struct mptcp_subflow_context *subflow;
> > + struct net *net = sock_net(sk);
> > + int err;
> > +
> > + if (!sf)
> > + return -EINVAL;
> > +
> > + lock_sock(sf->sk);
> > +
> > + mptcp_attach_cgroup(sk, sf->sk);
>
> This will likely not work if there are multiple MPTCP sockets using
> this endpoint.
>
> Possibly the cgroup attaching should be done at subflow_syn_recv_sock()
> time, for the child socket, with some custom code - because it must
> override the existing cgroup inherited from the listener.
>
> /P
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [MPTCP] Re: [MPTCP][PATCH v6 mptcp-next 2/9] mptcp: set the listening socket's subflow
@ 2020-11-25 12:11 Paolo Abeni
0 siblings, 0 replies; 5+ messages in thread
From: Paolo Abeni @ 2020-11-25 12:11 UTC (permalink / raw
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]
On Wed, 2020-11-25 at 18:20 +0800, Geliang Tang wrote:
> Hi Paolo,
>
> Paolo Abeni <pabeni(a)redhat.com> 于2020年11月25日周三 下午5:25写道：
> > On Wed, 2020-11-25 at 11:01 +0800, Geliang Tang wrote:
> > > This patch set the subflow context of the listening socket.
> > >
> > > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > > ---
> > > net/mptcp/pm_netlink.c | 5 +++++
> > > net/mptcp/protocol.h | 1 +
> > > net/mptcp/subflow.c | 37 +++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 43 insertions(+)
> > >
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index 0d9a3c77fdf1..39a91fcac403 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> > > @@ -330,6 +330,11 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> > > if (mptcp_pm_alloc_anno_list(msk, local)) {
> > > msk->pm.add_addr_signaled++;
> > > mptcp_pm_announce_addr(msk, &local->addr, false, local->addr.port);
> > > + if (local->addr.port) {
> > > + spin_unlock_bh(&msk->pm.lock);
> > > + mptcp_pm_set_listen_socket_subflow(sk, local->lsk);
> >
> > Why it's needed to this at announce time? Why not at lsk creation time?
> > Probably more related to the previous patch, why lsk is TCP socket
> > instead of an MPTCP one? (I feel the latter would be a more natural
> > choice, but it's just a feeling).
>
> I think the listening socket lsk is a subsocket. It should be joined to
> the msk as a subflow. lsk is created in PM netlink as a TCP socket, since
> this moment, the msk is not created.
Ah that is the main point! I think quite the opposite: the listening
socket *should* be unrelated to any user-created msk, because it's sort
of "shared" among all the msk announcing the additional port.
The subflow created by the listening sockets at accept time instead
should join an existing msk, and that is already done
by subflow_syn_recv_sock().
>
> At announce time, the msk is fully established, here I set up lsk's subflow
> context, and add it into the msk as a subflow.
What if another msk annunces the same endpoint?
> If lsk is an MPTCP socket, how can it to be joined to the msk?
As said, I think the listener subflow should _not_ join any existing
user-created msk.
Not sure if the above is clear enough, please let me know.
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* [MPTCP] Re: [MPTCP][PATCH v6 mptcp-next 2/9] mptcp: set the listening socket's subflow
@ 2020-12-04 1:40 Mat Martineau
0 siblings, 0 replies; 5+ messages in thread
From: Mat Martineau @ 2020-12-04 1:40 UTC (permalink / raw
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]
On Wed, 25 Nov 2020, Paolo Abeni wrote:
> On Wed, 2020-11-25 at 11:01 +0800, Geliang Tang wrote:
>> This patch set the subflow context of the listening socket.
>>
>> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
>> ---
>> net/mptcp/pm_netlink.c | 5 +++++
>> net/mptcp/protocol.h | 1 +
>> net/mptcp/subflow.c | 37 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 43 insertions(+)
>>
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index d4c136f1e754..6053a88c176f 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -1208,6 +1208,43 @@ int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock)
>> return 0;
>> }
>>
>> +int mptcp_pm_set_listen_socket_subflow(struct sock *sk, struct socket *sf)
>> +{
>> + struct mptcp_subflow_context *subflow;
>> + struct net *net = sock_net(sk);
>> + int err;
>> +
>> + if (!sf)
>> + return -EINVAL;
>> +
>> + lock_sock(sf->sk);
>> +
>> + mptcp_attach_cgroup(sk, sf->sk);
>
> This will likely not work if there are multiple MPTCP sockets using
> this endpoint.
>
> Possibly the cgroup attaching should be done at subflow_syn_recv_sock()
> time, for the child socket, with some custom code - because it must
> override the existing cgroup inherited from the listener.
>
Geliang -
I didn't see the cgroup issue addressed in v7, was it not necessary or is
there more to figure out to get the child socket attached to the right
cgroup?
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [MPTCP] Re: [MPTCP][PATCH v6 mptcp-next 2/9] mptcp: set the listening socket's subflow
@ 2020-12-09 9:11 Geliang Tang
0 siblings, 0 replies; 5+ messages in thread
From: Geliang Tang @ 2020-12-09 9:11 UTC (permalink / raw
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1928 bytes --]
Hi Mat,
Mat Martineau <mathew.j.martineau(a)linux.intel.com> 于2020年12月4日周五 上午9:40写道：
>
> On Wed, 25 Nov 2020, Paolo Abeni wrote:
>
> > On Wed, 2020-11-25 at 11:01 +0800, Geliang Tang wrote:
> >> This patch set the subflow context of the listening socket.
> >>
> >> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> >> ---
> >> net/mptcp/pm_netlink.c | 5 +++++
> >> net/mptcp/protocol.h | 1 +
> >> net/mptcp/subflow.c | 37 +++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 43 insertions(+)
> >>
> >> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> >> index d4c136f1e754..6053a88c176f 100644
> >> --- a/net/mptcp/subflow.c
> >> +++ b/net/mptcp/subflow.c
> >> @@ -1208,6 +1208,43 @@ int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock)
> >> return 0;
> >> }
> >>
> >> +int mptcp_pm_set_listen_socket_subflow(struct sock *sk, struct socket *sf)
> >> +{
> >> + struct mptcp_subflow_context *subflow;
> >> + struct net *net = sock_net(sk);
> >> + int err;
> >> +
> >> + if (!sf)
> >> + return -EINVAL;
> >> +
> >> + lock_sock(sf->sk);
> >> +
> >> + mptcp_attach_cgroup(sk, sf->sk);
> >
> > This will likely not work if there are multiple MPTCP sockets using
> > this endpoint.
> >
> > Possibly the cgroup attaching should be done at subflow_syn_recv_sock()
> > time, for the child socket, with some custom code - because it must
> > override the existing cgroup inherited from the listener.
> >
>
> Geliang -
>
> I didn't see the cgroup issue addressed in v7, was it not necessary or is
> there more to figure out to get the child socket attached to the right
> cgroup?
This patch is dropped in v7 since a MPTCP socket is used in v7, instead of
the TCP socket in v6. So no need to deal with the cgroup attaching now.
-Geliang
>
> --
> Mat Martineau
> Intel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-09 9:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-25 12:11 [MPTCP] Re: [MPTCP][PATCH v6 mptcp-next 2/9] mptcp: set the listening socket's subflow Paolo Abeni
-- strict thread matches above, loose matches on Subject: below --
2020-12-09 9:11 Geliang Tang
2020-12-04 1:40 Mat Martineau
2020-11-25 10:20 Geliang Tang
2020-11-25 9:24 Paolo Abeni
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.