mptcp: sockopt: use new helper for TCP_DEFER_ACCEPT

[PATCH mptcp-next] mptcp: sockopt: use new helper for TCP_DEFER_ACCEPT

Posted by Matthieu Baerts 3 years, 4 months ago

mptcp_setsockopt_sol_tcp_defer() was doing the same thing as
mptcp_setsockopt_first_sf_only() except for the returned code in case of
error.

Ignoring the error is needed to mimic how TCP_DEFER_ACCEPT is handled
when used with "plain" TCP sockets.

The specific function for TCP_DEFER_ACCEPT can be replaced by the new
mptcp_setsockopt_first_sf_only() helper and errors can be ignored to
stay compatible with TCP. A bit of cleanup.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Suggested-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/sockopt.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 1857281a0dd5..f85e9bbfe86f 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -758,18 +758,6 @@ static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
 	return -EOPNOTSUPP;
 }
 
-static int mptcp_setsockopt_sol_tcp_defer(struct mptcp_sock *msk, sockptr_t optval,
-					  unsigned int optlen)
-{
-	struct socket *listener;
-
-	listener = __mptcp_nmpc_socket(msk);
-	if (!listener)
-		return 0; /* TCP_DEFER_ACCEPT does not fail */
-
-	return tcp_setsockopt(listener->sk, SOL_TCP, TCP_DEFER_ACCEPT, optval, optlen);
-}
-
 static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int optname,
 					  sockptr_t optval, unsigned int optlen)
 {
@@ -810,7 +798,9 @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
 	case TCP_NODELAY:
 		return mptcp_setsockopt_sol_tcp_nodelay(msk, optval, optlen);
 	case TCP_DEFER_ACCEPT:
-		return mptcp_setsockopt_sol_tcp_defer(msk, optval, optlen);
+		/* See tcp.c: TCP_DEFER_ACCEPT does not fail */
+		mptcp_setsockopt_first_sf_only(msk, SOL_TCP, optname, optval, optlen);
+		return 0;
 	case TCP_FASTOPEN_CONNECT:
 	case TCP_FASTOPEN_NO_COOKIE:
 		return mptcp_setsockopt_first_sf_only(msk, SOL_TCP, optname,

base-commit: 6711fbb5e6c8541f84685542127455c18145d559
-- 
2.37.2

Re: [PATCH mptcp-next] mptcp: sockopt: use new helper for TCP_DEFER_ACCEPT

Posted by Matthieu Baerts 3 years, 4 months ago

Hi Paolo, Mat,

On 28/09/2022 11:18, Matthieu Baerts wrote:
> mptcp_setsockopt_sol_tcp_defer() was doing the same thing as
> mptcp_setsockopt_first_sf_only() except for the returned code in case of
> error.
> 
> Ignoring the error is needed to mimic how TCP_DEFER_ACCEPT is handled
> when used with "plain" TCP sockets.
> 
> The specific function for TCP_DEFER_ACCEPT can be replaced by the new
> mptcp_setsockopt_first_sf_only() helper and errors can be ignored to
> stay compatible with TCP. A bit of cleanup.

Thank you both for your review!

I just applied this patch in our tree (feat. for net-next) without
Paolo's Suggested-by tag.

New patches for t/upstream:
- db52578cbc69: mptcp: sockopt: use new helper for TCP_DEFER_ACCEPT
- Results: 6ae98b87e288..a34025ee429c (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220930T081708

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

Re: [PATCH mptcp-next] mptcp: sockopt: use new helper for TCP_DEFER_ACCEPT

Posted by Mat Martineau 3 years, 4 months ago

On Wed, 28 Sep 2022, Matthieu Baerts wrote:

> mptcp_setsockopt_sol_tcp_defer() was doing the same thing as
> mptcp_setsockopt_first_sf_only() except for the returned code in case of
> error.
>
> Ignoring the error is needed to mimic how TCP_DEFER_ACCEPT is handled
> when used with "plain" TCP sockets.
>
> The specific function for TCP_DEFER_ACCEPT can be replaced by the new
> mptcp_setsockopt_first_sf_only() helper and errors can be ignored to
> stay compatible with TCP. A bit of cleanup.
>
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Suggested-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Looks good to me, thanks!

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

> ---
> net/mptcp/sockopt.c | 16 +++-------------
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 1857281a0dd5..f85e9bbfe86f 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -758,18 +758,6 @@ static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
> 	return -EOPNOTSUPP;
> }
>
> -static int mptcp_setsockopt_sol_tcp_defer(struct mptcp_sock *msk, sockptr_t optval,
> -					  unsigned int optlen)
> -{
> -	struct socket *listener;
> -
> -	listener = __mptcp_nmpc_socket(msk);
> -	if (!listener)
> -		return 0; /* TCP_DEFER_ACCEPT does not fail */
> -
> -	return tcp_setsockopt(listener->sk, SOL_TCP, TCP_DEFER_ACCEPT, optval, optlen);
> -}
> -
> static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int optname,
> 					  sockptr_t optval, unsigned int optlen)
> {
> @@ -810,7 +798,9 @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
> 	case TCP_NODELAY:
> 		return mptcp_setsockopt_sol_tcp_nodelay(msk, optval, optlen);
> 	case TCP_DEFER_ACCEPT:
> -		return mptcp_setsockopt_sol_tcp_defer(msk, optval, optlen);
> +		/* See tcp.c: TCP_DEFER_ACCEPT does not fail */
> +		mptcp_setsockopt_first_sf_only(msk, SOL_TCP, optname, optval, optlen);
> +		return 0;
> 	case TCP_FASTOPEN_CONNECT:
> 	case TCP_FASTOPEN_NO_COOKIE:
> 		return mptcp_setsockopt_first_sf_only(msk, SOL_TCP, optname,
>
> base-commit: 6711fbb5e6c8541f84685542127455c18145d559
> -- 
> 2.37.2
>
>

--
Mat Martineau
Intel

Re: mptcp: sockopt: use new helper for TCP_DEFER_ACCEPT: Tests Results

Posted by MPTCP CI 3 years, 4 months ago

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4607251246743552
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4607251246743552/summary/summary.txt

- KVM Validation: debug:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5733151153586176
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5733151153586176/summary/summary.txt

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


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] mptcp: sockopt: use new helper for TCP_DEFER_ACCEPT

Posted by Paolo Abeni 3 years, 4 months ago

On Wed, 2022-09-28 at 11:18 +0200, Matthieu Baerts wrote:
> mptcp_setsockopt_sol_tcp_defer() was doing the same thing as
> mptcp_setsockopt_first_sf_only() except for the returned code in case of
> error.
> 
> Ignoring the error is needed to mimic how TCP_DEFER_ACCEPT is handled
> when used with "plain" TCP sockets.
> 
> The specific function for TCP_DEFER_ACCEPT can be replaced by the new
> mptcp_setsockopt_first_sf_only() helper and errors can be ignored to
> stay compatible with TCP. A bit of cleanup.
> 
> Suggested-by: Paolo Abeni <pabeni@redhat.com>

You can/should drop the above tag ;)

> Suggested-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
>  net/mptcp/sockopt.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 1857281a0dd5..f85e9bbfe86f 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -758,18 +758,6 @@ static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
>  	return -EOPNOTSUPP;
>  }
>  
> -static int mptcp_setsockopt_sol_tcp_defer(struct mptcp_sock *msk, sockptr_t optval,
> -					  unsigned int optlen)
> -{
> -	struct socket *listener;
> -
> -	listener = __mptcp_nmpc_socket(msk);
> -	if (!listener)
> -		return 0; /* TCP_DEFER_ACCEPT does not fail */
> -
> -	return tcp_setsockopt(listener->sk, SOL_TCP, TCP_DEFER_ACCEPT, optval, optlen);
> -}
> -
>  static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int optname,
>  					  sockptr_t optval, unsigned int optlen)
>  {
> @@ -810,7 +798,9 @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>  	case TCP_NODELAY:
>  		return mptcp_setsockopt_sol_tcp_nodelay(msk, optval, optlen);
>  	case TCP_DEFER_ACCEPT:
> -		return mptcp_setsockopt_sol_tcp_defer(msk, optval, optlen);
> +		/* See tcp.c: TCP_DEFER_ACCEPT does not fail */
> +		mptcp_setsockopt_first_sf_only(msk, SOL_TCP, optname, optval, optlen);
> +		return 0;
>  	case TCP_FASTOPEN_CONNECT:
>  	case TCP_FASTOPEN_NO_COOKIE:
>  		return mptcp_setsockopt_first_sf_only(msk, SOL_TCP, optname,
> 
> base-commit: 6711fbb5e6c8541f84685542127455c18145d559

Nice! LGTM!

Acked-by: Paolo Abeni <pabeni@redhat.com>

Re: [PATCH mptcp-next] mptcp: sockopt: use new helper for TCP_DEFER_ACCEPT

Posted by Matthieu Baerts 3 years, 4 months ago

Hi Paolo,

On 28/09/2022 11:46, Paolo Abeni wrote:
> On Wed, 2022-09-28 at 11:18 +0200, Matthieu Baerts wrote:
>> mptcp_setsockopt_sol_tcp_defer() was doing the same thing as
>> mptcp_setsockopt_first_sf_only() except for the returned code in case of
>> error.
>>
>> Ignoring the error is needed to mimic how TCP_DEFER_ACCEPT is handled
>> when used with "plain" TCP sockets.
>>
>> The specific function for TCP_DEFER_ACCEPT can be replaced by the new
>> mptcp_setsockopt_first_sf_only() helper and errors can be ignored to
>> stay compatible with TCP. A bit of cleanup.
>>
>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> 
> You can/should drop the above tag ;)

You suggested to remove mptcp_setsockopt_sol_tcp_defer() but I
understand your request :)

Thank you for the review!

I can wait for Mat to review it as he suggested the whole code :-)

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