Re: [MPTCP] [PATCH RFC 6/6] mptcp: use __tcp_poll instead of tcp_poll

Re: [MPTCP] [PATCH RFC 6/6] mptcp: use __tcp_poll instead of tcp_poll

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> >  		tcp_sock = mptcp_subflow_tcp_socket(subflow);
> > -		ret |= tcp_poll(file, tcp_sock, wait);
> > +		ret |= __tcp_poll(tcp_sock->sk);
> >  	}
> >  	release_sock(sk);
> 
> This allows mptcp_poll working even without a 'struct socket'. we still
> relies on such struct presence for:

Thats what I though as well, but tcp_poll assumes sk->sk_socket points
to the userspace-facing socket structure.

> kernel_accept
> kernel_sock_shutdown
> kernel_setsockopt
> kernel_getsockopt
> kernel_bind
> kernel_connect
> 
> I'm wondering if we should try to get rid completely of any depenency
> on 'struct socket'? That will fix the current issue with MP_JOIN, will
> reduce the memory usage and likely will improve accept time.

It will require careful audit of the stack, afaics Tcp assumes that for
anythig incoming tcp sk might have been detached from usersapce already
(i.e., application called close()).  But for anything coming from
syscalls that doesn't seem to be the case.

I started to hack up an MP_JOIN patch that uses a per-sk workqueue
to defer incoming joins so that a socket can be allocated for it.

I don't like it, but it seems easier to go with that route.

Re: [MPTCP] [PATCH RFC 6/6] mptcp: use __tcp_poll instead of tcp_poll

[Paolo Abeni]

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

On Mon, 2019-09-09 at 17:53 +0200, Florian Westphal wrote:
> tcp_poll places current task on a wait queue.  This can cause us to block
> forever -- some subflows might be backup/failover and never receive any
> data.
> 
> Use __tcp_poll instead to avoid blocking current task again after parent
> mptcp socket has been woken up from the subflow layer.
> 
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/protocol.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2a91fff1f385..533fe2c50d3a 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1182,7 +1182,7 @@ static __poll_t mptcp_poll(struct file *file, struct socket *sock,
>  		struct socket *tcp_sock;
>  
>  		tcp_sock = mptcp_subflow_tcp_socket(subflow);
> -		ret |= tcp_poll(file, tcp_sock, wait);
> +		ret |= __tcp_poll(tcp_sock->sk);
>  	}
>  	release_sock(sk);

This allows mptcp_poll working even without a 'struct socket'. we still
relies on such struct presence for:

kernel_accept
kernel_sock_shutdown
kernel_setsockopt
kernel_getsockopt
kernel_bind
kernel_connect

I'm wondering if we should try to get rid completely of any depenency
on 'struct socket'? That will fix the current issue with MP_JOIN, will
reduce the memory usage and likely will improve accept time.

A quick look makes me think that the most problematic call to be
addressed without using any 'struct socket' are kernel_bind() and
kernel_connect(), perhaps we can use a transient 'struct socket' just
for subflow_connect() ? WDYT?

Thanks,

Paolo

[MPTCP] [PATCH RFC 6/6] mptcp: use __tcp_poll instead of tcp_poll

[Florian Westphal]

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

tcp_poll places current task on a wait queue.  This can cause us to block
forever -- some subflows might be backup/failover and never receive any
data.

Use __tcp_poll instead to avoid blocking current task again after parent
mptcp socket has been woken up from the subflow layer.

Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
 net/mptcp/protocol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2a91fff1f385..533fe2c50d3a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1182,7 +1182,7 @@ static __poll_t mptcp_poll(struct file *file, struct socket *sock,
 		struct socket *tcp_sock;
 
 		tcp_sock = mptcp_subflow_tcp_socket(subflow);
-		ret |= tcp_poll(file, tcp_sock, wait);
+		ret |= __tcp_poll(tcp_sock->sk);
 	}
 	release_sock(sk);
 
-- 
2.21.0