[MPTCP] Re: [RFC 7/7] make accept not allocate kernel socket struct

[MPTCP] Re: [RFC 7/7] make accept not allocate kernel socket struct

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> > No, its also for outgoing connections, we use in kernel sockets for
> > those.
> 
> Uhm...  I see. Everything created via mptcp_subflow_create_socket(),
> right? I though of always using msk's socket also/mainly to reduce the
> memory usage, but server side we should have very few
> mptcp_subflow_create_socket() instances, so I guess it's no necessary
> optimize them.

FWIW it might make sense to not have those in kernel sockets either
in the future, but this might need a bit more changes to how outgoing
connections work in mptcp-next.

I would have a look at it at some later date, I don't think its
a high priority.

> > > > @@ -1443,6 +1470,20 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
> > > >  		return -EINVAL;
> > > >  
> > > >  	err = ssock->ops->accept(sock, newsock, flags, kern);
> > > > +	if (err == 0 && (newsock->sk->sk_prot == &mptcp_prot ||
> > > > +			 is_mptcp_v6(newsock))) {
> > > 
> > > Is not clear to me why/how we can hit the condition '!(newsock->sk-
> > > > sk_prot == &mptcp_prot || is_mptcp_v6(newsock))' ... Can you please
> > > explain?
> > 
> > We will hit it when we get connection from non-mptcp peer, i.e. the
> > !mp_capable part of mptcp_accept().
> > 
> > In that case sk_prot is &tcp_prot.
> > 
> > It might make sense to change this to
> >     err == 0 && newsock->sk->sk_prot != &tcp_prot
> > 
> > perhaps that would clarify this a bit.  WDYT?
> 
> Yep, I like this latter code more!

Ok, I've flagged the patches 'RFC' in patchwork already.
I will make this change and will resubmit that part later.

Thanks,
Florian

[MPTCP] Re: [RFC 7/7] make accept not allocate kernel socket struct

[Paolo Abeni]

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

On Mon, 2019-11-25 at 13:37 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Mon, 2019-11-25 at 03:15 +0100, Florian Westphal wrote:
> > > @@ -924,10 +924,17 @@ static void mptcp_close(struct sock *sk, long timeout)
> > >  
> > >  	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
> > >  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > > +		struct socket *sock = READ_ONCE(ssk->sk_socket);
> > >  
> > >  		pr_debug("conn_list->subflow=%p", subflow);
> > >  		list_del(&subflow->node);
> > > -		sock_release(ssk->sk_socket);
> > > +
> > > +		if (sock && sock != sk->sk_socket) {
> > > +			sock_release(sock);
> > 
> > Double checking I read the above correctly: the condition 'sock && sock
> > != sk->sk_socket' only for the first subflow of a client socket, right?
> > If so, can we use the msk socket even for that one?
> 
> No, its also for outgoing connections, we use in kernel sockets for
> those.

Uhm...  I see. Everything created via mptcp_subflow_create_socket(),
right? I though of always using msk's socket also/mainly to reduce the
memory usage, but server side we should have very few
mptcp_subflow_create_socket() instances, so I guess it's no necessary
optimize them.

> > > @@ -1443,6 +1470,20 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
> > >  		return -EINVAL;
> > >  
> > >  	err = ssock->ops->accept(sock, newsock, flags, kern);
> > > +	if (err == 0 && (newsock->sk->sk_prot == &mptcp_prot ||
> > > +			 is_mptcp_v6(newsock))) {
> > 
> > Is not clear to me why/how we can hit the condition '!(newsock->sk-
> > > sk_prot == &mptcp_prot || is_mptcp_v6(newsock))' ... Can you please
> > explain?
> 
> We will hit it when we get connection from non-mptcp peer, i.e. the
> !mp_capable part of mptcp_accept().
> 
> In that case sk_prot is &tcp_prot.
> 
> It might make sense to change this to
>     err == 0 && newsock->sk->sk_prot != &tcp_prot
> 
> perhaps that would clarify this a bit.  WDYT?

Yep, I like this latter code more!

Thank you,

Paolo

[MPTCP] Re: [RFC 7/7] make accept not allocate kernel socket struct

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Mon, 2019-11-25 at 03:15 +0100, Florian Westphal wrote:
> > @@ -924,10 +924,17 @@ static void mptcp_close(struct sock *sk, long timeout)
> >  
> >  	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
> >  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > +		struct socket *sock = READ_ONCE(ssk->sk_socket);
> >  
> >  		pr_debug("conn_list->subflow=%p", subflow);
> >  		list_del(&subflow->node);
> > -		sock_release(ssk->sk_socket);
> > +
> > +		if (sock && sock != sk->sk_socket) {
> > +			sock_release(sock);
> 
> Double checking I read the above correctly: the condition 'sock && sock
> != sk->sk_socket' only for the first subflow of a client socket, right?
> If so, can we use the msk socket even for that one?

No, its also for outgoing connections, we use in kernel sockets for
those.

> > @@ -1443,6 +1470,20 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
> >  		return -EINVAL;
> >  
> >  	err = ssock->ops->accept(sock, newsock, flags, kern);
> > +	if (err == 0 && (newsock->sk->sk_prot == &mptcp_prot ||
> > +			 is_mptcp_v6(newsock))) {
> 
> Is not clear to me why/how we can hit the condition '!(newsock->sk-
> >sk_prot == &mptcp_prot || is_mptcp_v6(newsock))' ... Can you please
> explain?

We will hit it when we get connection from non-mptcp peer, i.e. the
!mp_capable part of mptcp_accept().

In that case sk_prot is &tcp_prot.

It might make sense to change this to
    err == 0 && newsock->sk->sk_prot != &tcp_prot

perhaps that would clarify this a bit.  WDYT?

[MPTCP] Re: [RFC 7/7] make accept not allocate kernel socket struct

[Paolo Abeni]

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

On Mon, 2019-11-25 at 03:15 +0100, Florian Westphal wrote:
> @@ -924,10 +924,17 @@ static void mptcp_close(struct sock *sk, long timeout)
>  
>  	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
>  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +		struct socket *sock = READ_ONCE(ssk->sk_socket);
>  
>  		pr_debug("conn_list->subflow=%p", subflow);
>  		list_del(&subflow->node);
> -		sock_release(ssk->sk_socket);
> +
> +		if (sock && sock != sk->sk_socket) {
> +			sock_release(sock);

Double checking I read the above correctly: the condition 'sock && sock
!= sk->sk_socket' only for the first subflow of a client socket, right?
If so, can we use the msk socket even for that one?

> @@ -1443,6 +1470,20 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
>  		return -EINVAL;
>  
>  	err = ssock->ops->accept(sock, newsock, flags, kern);
> +	if (err == 0 && (newsock->sk->sk_prot == &mptcp_prot ||
> +			 is_mptcp_v6(newsock))) {

Is not clear to me why/how we can hit the condition '!(newsock->sk-
>sk_prot == &mptcp_prot || is_mptcp_v6(newsock))' ... Can you please
explain?

Thanks!

Paolo