[PATCH mptcp-next 0/2] mptcp: a couple of cleanups

[PATCH mptcp-next 0/2] mptcp: a couple of cleanups

[Paolo Abeni]

The first patch was already shared as RFC, this iteration
addresses a couple of bugs there.

The second patch removes a bunch of conditionals and atomic
operations in the fast path, but the performance impact is
actually below noise level. Still I think it's worthy, as the
unneeded atomic operations are confusing.

Paolo Abeni (2):
  mptcp: cleanup MPJ subflow list handling
  mptcp: avoid atomic bit manipulation when possible

 net/mptcp/pm_netlink.c |   3 -
 net/mptcp/protocol.c   | 149 ++++++++++++++++++-----------------------
 net/mptcp/protocol.h   |  31 ++++-----
 net/mptcp/sockopt.c    |  24 ++-----
 net/mptcp/subflow.c    |   9 ++-
 5 files changed, 89 insertions(+), 127 deletions(-)

-- 
2.33.1

Re: [PATCH mptcp-next 0/2] mptcp: a couple of cleanups

[Mat Martineau]

On Mon, 6 Dec 2021, Paolo Abeni wrote:

> The first patch was already shared as RFC, this iteration
> addresses a couple of bugs there.
>
> The second patch removes a bunch of conditionals and atomic
> operations in the fast path, but the performance impact is
> actually below noise level. Still I think it's worthy, as the
> unneeded atomic operations are confusing.
>
> Paolo Abeni (2):
>  mptcp: cleanup MPJ subflow list handling
>  mptcp: avoid atomic bit manipulation when possible
>
> net/mptcp/pm_netlink.c |   3 -
> net/mptcp/protocol.c   | 149 ++++++++++++++++++-----------------------
> net/mptcp/protocol.h   |  31 ++++-----
> net/mptcp/sockopt.c    |  24 ++-----
> net/mptcp/subflow.c    |   9 ++-
> 5 files changed, 89 insertions(+), 127 deletions(-)

Looks good to me:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Matthieu, note that this patch set depends on the "mptcp: improve subflow 
creation on errors" patches.

--
Mat Martineau
Intel

[PATCH mptcp-next 0/2] mptcp: a couple of cleanups

[Paolo Abeni]

After the recent fixes we have both a good changes and some
needs to simplify subflow_syn_recv_sock().

A couple of patches in that direction, also addressing a long
standing feature issue.

Paolo Abeni (2):
  mptcp: avoid unneeded address copy
  mptcp: simplify subflow_syn_recv_sock()

 net/mptcp/subflow.c | 40 ++++++++++------------------------------
 1 file changed, 10 insertions(+), 30 deletions(-)

-- 
2.39.2

[PATCH mptcp-next 1/2] mptcp: avoid unneeded address copy

[Paolo Abeni]

In the syn_recv fallback path, the msk is unused. We can skip
setting the socket address.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/subflow.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 1ca8d30e9276..f0758b23c6b2 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -821,8 +821,6 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 				goto dispose_child;
 			}
 
-			if (new_msk)
-				mptcp_copy_inaddrs(new_msk, child);
 			mptcp_subflow_drop_ctx(child);
 			goto out;
 		}
-- 
2.39.2

[PATCH mptcp-next 2/2] mptcp: simplify subflow_syn_recv_sock()

[Paolo Abeni]

Postpone the msk cloning to the child process creation
so that we can avoid a bunch of conditionals.

Close: https://github.com/multipath-tcp/mptcp_net-next/issues/61
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/subflow.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index f0758b23c6b2..d79926cb9152 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -696,14 +696,6 @@ static bool subflow_hmac_valid(const struct request_sock *req,
 	return !crypto_memneq(hmac, mp_opt->hmac, MPTCPOPT_HMAC_LEN);
 }
 
-static void mptcp_force_close(struct sock *sk)
-{
-	/* the msk is not yet exposed to user-space, and refcount is 2 */
-	inet_sk_state_store(sk, TCP_CLOSE);
-	sk_common_release(sk);
-	sock_put(sk);
-}
-
 static void subflow_ulp_fallback(struct sock *sk,
 				 struct mptcp_subflow_context *old_ctx)
 {
@@ -755,7 +747,6 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	struct mptcp_subflow_request_sock *subflow_req;
 	struct mptcp_options_received mp_opt;
 	bool fallback, fallback_is_fatal;
-	struct sock *new_msk = NULL;
 	struct mptcp_sock *owner;
 	struct sock *child;
 
@@ -784,14 +775,9 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		 * options.
 		 */
 		mptcp_get_options(skb, &mp_opt);
-		if (!(mp_opt.suboptions & OPTIONS_MPTCP_MPC)) {
+		if (!(mp_opt.suboptions & OPTIONS_MPTCP_MPC))
 			fallback = true;
-			goto create_child;
-		}
 
-		new_msk = mptcp_sk_clone(listener->conn, &mp_opt, req);
-		if (!new_msk)
-			fallback = true;
 	} else if (subflow_req->mp_join) {
 		mptcp_get_options(skb, &mp_opt);
 		if (!(mp_opt.suboptions & OPTIONS_MPTCP_MPJ) ||
@@ -820,21 +806,23 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 				subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
 				goto dispose_child;
 			}
-
-			mptcp_subflow_drop_ctx(child);
-			goto out;
+			goto fallback;
 		}
 
 		/* ssk inherits options of listener sk */
 		ctx->setsockopt_seq = listener->setsockopt_seq;
 
 		if (ctx->mp_capable) {
-			owner = mptcp_sk(new_msk);
+			ctx->conn = mptcp_sk_clone(listener->conn, &mp_opt, req);
+			if (!ctx->conn)
+				goto fallback;
+
+			owner = mptcp_sk(ctx->conn);
 
 			/* this can't race with mptcp_close(), as the msk is
 			 * not yet exposted to user-space
 			 */
-			inet_sk_state_store((void *)new_msk, TCP_ESTABLISHED);
+			inet_sk_state_store(ctx->conn, TCP_ESTABLISHED);
 
 			/* record the newly created socket as the first msk
 			 * subflow, but don't link it yet into conn_list
@@ -844,11 +832,9 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			/* new mpc subflow takes ownership of the newly
 			 * created mptcp socket
 			 */
-			mptcp_sk(new_msk)->setsockopt_seq = ctx->setsockopt_seq;
+			owner->setsockopt_seq = ctx->setsockopt_seq;
 			mptcp_pm_new_connection(owner, child, 1);
 			mptcp_token_accept(subflow_req, owner);
-			ctx->conn = new_msk;
-			new_msk = NULL;
 
 			/* set msk addresses early to ensure mptcp_pm_get_local_id()
 			 * uses the correct data
@@ -901,14 +887,10 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		 * soon, but context must be explicitly deleted or will be
 		 * leaked
 		 */
+fallback:
 		mptcp_subflow_drop_ctx(child);
 	}
 
-out:
-	/* dispose of the left over mptcp master, if any */
-	if (unlikely(new_msk))
-		mptcp_force_close(new_msk);
-
 	/* check for expected invariant - should never trigger, just help
 	 * catching eariler subtle bugs
 	 */
-- 
2.39.2

Re: mptcp: simplify subflow_syn_recv_sock(): Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6711639714562048
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6711639714562048/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4565393017143296
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4565393017143296/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5194737863360512
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5194737863360512/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5691292923985920
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5691292923985920/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/8dce9f2e2a70


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-next 1/2] mptcp: avoid unneeded address copy

[Matthieu Baerts]

Hi Paolo,

Thank you for this patch!

On 06/03/2023 19:30, Paolo Abeni wrote:
> In the syn_recv fallback path, the msk is unused. We can skip
> setting the socket address.

Should this go to -net with a Fixes tag?

Or not needed, not to have to delay the next patch?

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

Re: [PATCH mptcp-next 0/2] mptcp: a couple of cleanups

[Matthieu Baerts]

Hi Paolo,

On 06/03/2023 19:30, Paolo Abeni wrote:
> After the recent fixes we have both a good changes and some
> needs to simplify subflow_syn_recv_sock().
> 
> A couple of patches in that direction, also addressing a long
> standing feature issue.

Thank you for these patches!

I just have one question about the first patch but for the rest, it
looks good to me!

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

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

Re: mptcp: simplify subflow_syn_recv_sock(): Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5946997224505344
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5946997224505344/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5384047271084032
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5384047271084032/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4680359829307392
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4680359829307392/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6509947177926656
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6509947177926656/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b9f71868d34f


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-next 0/2] mptcp: a couple of cleanups

[Matthieu Baerts]

Hi Paolo,

On 06/03/2023 19:30, Paolo Abeni wrote:
> After the recent fixes we have both a good changes and some
> needs to simplify subflow_syn_recv_sock().
> 
> A couple of patches in that direction, also addressing a long
> standing feature issue.

Thank you for the patches!

Now in our tree (feat. for net-next):

New patches for t/upstream:
- a8d0495b5733: mptcp: avoid unneeded address copy
- 7526f37142a5: mptcp: simplify subflow_syn_recv_sock()
- Results: c26d7d0e4343..6f879388dfd8 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230307T173437

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