Rewrite the mptcp socket accept op, partially open-codying
inet_accept(), instead of indirectly calling it.

This way we can avoid acquiring the new socket lock twice
and we can avoid a couple of indirect calls.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f742d558a1b8..cf161deb64d8 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3748,6 +3748,7 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 {
 	struct mptcp_sock *msk = mptcp_sk(sock->sk);
 	struct socket *ssock;
+	struct sock *newsk;
 	int err;
 
 	pr_debug("msk=%p", msk);
@@ -3759,16 +3760,26 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 	if (!ssock)
 		return -EINVAL;
 
-	err = ssock->ops->accept(sock, newsock, flags, kern);
-	if (err == 0 && !mptcp_is_tcpsk(newsock->sk)) {
-		struct mptcp_sock *msk = mptcp_sk(newsock->sk);
+	newsk = mptcp_accept(sock->sk, flags, &err, kern);
+	if (!newsk)
+		return err;
+
+	lock_sock(newsk);
+
+	sock_rps_record_flow(newsk);
+	WARN_ON(!((1 << newsk->sk_state) &
+		  (TCPF_ESTABLISHED | TCPF_SYN_RECV |
+		  TCPF_CLOSE_WAIT | TCPF_CLOSE)));
+
+	sock_graft(newsk, newsock);
+
+	newsock->state = SS_CONNECTED;
+	if (!mptcp_is_tcpsk(newsock->sk)) {
+		struct mptcp_sock *msk = mptcp_sk(newsk);
 		struct mptcp_subflow_context *subflow;
-		struct sock *newsk = newsock->sk;
 
 		set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);
 
-		lock_sock(newsk);
-
 		/* set ssk->sk_socket of accept()ed flows to mptcp socket.
 		 * This is needed so NOSPACE flag can be set from tcp stack.
 		 */
@@ -3778,10 +3789,10 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 			if (!ssk->sk_socket)
 				mptcp_sock_graft(ssk, newsock);
 		}
-		release_sock(newsk);
 	}
+	release_sock(newsk);
 
-	return err;
+	return 0;
 }
 
 static __poll_t mptcp_check_writeable(struct mptcp_sock *msk)
-- 
2.39.0

Hi Paolo,

On 17/01/2023 08:36, Paolo Abeni wrote:
> Rewrite the mptcp socket accept op, partially open-codying
> inet_accept(), instead of indirectly calling it.
>
> This way we can avoid acquiring the new socket lock twice
> and we can avoid a couple of indirect calls.

Maybe a stupid idea but could we eventually split inet_accept() to have
a __inet_accept() version with what has to be done while holding the
lock and reusable in MPTCP?

From MPTCP, we would not need to duplicate the code but call
__inet_accept() (no more indirection) and be ready for Zero Copy stuff
or other modifications of __inet_accept() later if needed.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

On Wed, 2023-01-25 at 18:47 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 17/01/2023 08:36, Paolo Abeni wrote:
> > Rewrite the mptcp socket accept op, partially open-codying
> > inet_accept(), instead of indirectly calling it.
> > 
> > This way we can avoid acquiring the new socket lock twice
> > and we can avoid a couple of indirect calls.
> 
> Maybe a stupid idea but could we eventually split inet_accept() to have
> a __inet_accept() version with what has to be done while holding the
> lock and reusable in MPTCP?

I thought about the above but in the end that would be a very small
helper:


        sock_rps_record_flow(sk2);
        WARN_ON(!((1 << sk2->sk_state) &
                  (TCPF_ESTABLISHED | TCPF_SYN_RECV |
                  TCPF_CLOSE_WAIT | TCPF_CLOSE)));

        if (test_bit(SOCK_SUPPORT_ZC, &sock->flags))
                set_bit(SOCK_SUPPORT_ZC, &newsock->flags);
        sock_graft(sk2, newsock);

        newsock->state = SS_CONNECTED;

with the larger part  - the WARN_ON - being likely obsoleted. 

I can do that, if you prefer.

Thanks,

Paolo

Hi Paolo,

On 26/01/2023 18:53, Paolo Abeni wrote:
> On Wed, 2023-01-25 at 18:47 +0100, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 17/01/2023 08:36, Paolo Abeni wrote:
>>> Rewrite the mptcp socket accept op, partially open-codying
>>> inet_accept(), instead of indirectly calling it.
>>>
>>> This way we can avoid acquiring the new socket lock twice
>>> and we can avoid a couple of indirect calls.
>>
>> Maybe a stupid idea but could we eventually split inet_accept() to have
>> a __inet_accept() version with what has to be done while holding the
>> lock and reusable in MPTCP?
> 
> I thought about the above but in the end that would be a very small
> helper:
> 
> 
>         sock_rps_record_flow(sk2);
>         WARN_ON(!((1 << sk2->sk_state) &
>                   (TCPF_ESTABLISHED | TCPF_SYN_RECV |
>                   TCPF_CLOSE_WAIT | TCPF_CLOSE)));
> 
>         if (test_bit(SOCK_SUPPORT_ZC, &sock->flags))
>                 set_bit(SOCK_SUPPORT_ZC, &newsock->flags);
>         sock_graft(sk2, newsock);
> 
>         newsock->state = SS_CONNECTED;
> 
> with the larger part  - the WARN_ON - being likely obsoleted. 
> 
> I can do that, if you prefer.

I would prefer but please tell me if you think we should not.

I'm maybe a bit too paranoiac because we had some nasty bugs in the past
with mptcp.org due to modifications on TCP side. (But I know the
situation is different here: MPTCP is upstream, people might now think
about it when changing the core and a great net maintainer is super
aware of MPTCP :-D )

If the chunk is "duplicated", it would be nice to at least add a comment
in the code :)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

On Thu, 2023-01-26 at 19:27 +0100, Matthieu Baerts wrote:
> On 26/01/2023 18:53, Paolo Abeni wrote:
> > On Wed, 2023-01-25 at 18:47 +0100, Matthieu Baerts wrote:
> > > On 17/01/2023 08:36, Paolo Abeni wrote:
> > > > Rewrite the mptcp socket accept op, partially open-codying
> > > > inet_accept(), instead of indirectly calling it.
> > > > 
> > > > This way we can avoid acquiring the new socket lock twice
> > > > and we can avoid a couple of indirect calls.
> > > 
> > > Maybe a stupid idea but could we eventually split inet_accept() to have
> > > a __inet_accept() version with what has to be done while holding the
> > > lock and reusable in MPTCP?
> > 
> > I thought about the above but in the end that would be a very small
> > helper:
> > 
> > 
> >         sock_rps_record_flow(sk2);
> >         WARN_ON(!((1 << sk2->sk_state) &
> >                   (TCPF_ESTABLISHED | TCPF_SYN_RECV |
> >                   TCPF_CLOSE_WAIT | TCPF_CLOSE)));
> > 
> >         if (test_bit(SOCK_SUPPORT_ZC, &sock->flags))
> >                 set_bit(SOCK_SUPPORT_ZC, &newsock->flags);
> >         sock_graft(sk2, newsock);
> > 
> >         newsock->state = SS_CONNECTED;
> > 
> > with the larger part  - the WARN_ON - being likely obsoleted. 
> > 
> > I can do that, if you prefer.
> 
> I would prefer but please tell me if you think we should not.
> 
> I'm maybe a bit too paranoiac because we had some nasty bugs in the past
> with mptcp.org due to modifications on TCP side. (But I know the
> situation is different here: MPTCP is upstream, people might now think
> about it when changing the core and a great net maintainer is super
> aware of MPTCP :-D )
> 
> If the chunk is "duplicated", it would be nice to at least add a comment
> in the code :)

Ok I can send out a v3 with the bunch of additional comments discussed
in the other patches and the new helper (will add one more patch).

Cheers,

Paolo

Hi Paolo,

On 17/01/2023 08:36, Paolo Abeni wrote:
> Rewrite the mptcp socket accept op, partially open-codying
> inet_accept(), instead of indirectly calling it.
>
> This way we can avoid acquiring the new socket lock twice
> and we can avoid a couple of indirect calls.

Maybe a stupid idea but could we eventually split inet_accept() to have
a __inet_accept() version with what has to be done while holding the
lock and reusable in MPTCP?

From MPTCP, we would not need to duplicate the code but call
__inet_accept() (no more indirection) and be ready for Zero Copy stuff
or other modifications of __inet_accept() later if needed.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net