| Message ID | 20240213223135.85957-1-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [v1,net-next] net: Deprecate SO_DEBUG and reclaim SOCK_DBG bit. | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 60 lines checked |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ | success | Success! ✅ |
Hi Kuniyuki,
Thank you for your modifications, that's great!
Our CI (GitHub Action) did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7893771323
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b82e0568acae
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-normal
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 (NGI0 Core)
Hi Kuniyuki,
Thank you for your modifications, that's great!
Our CI (Cirrus) did some validations with a debug kernel and here is its report:
- KVM Validation: debug (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5969355108777984
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5969355108777984/summary/summary.txt
- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5471411028885504
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5471411028885504/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b82e0568acae
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 (NGI0 Core)
On Tue, 2024-02-13 at 14:31 -0800, Kuniyuki Iwashima wrote: Hi Kuniyuki, I'm adding the newly appointed SMC maintainers to chime in. > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 > ("net: > remove SOCK_DEBUG macro") removed the macro. > > Now is the time to deprecate the oldest socket option. > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- > include/net/sock.h | 1 - > net/core/sock.c | 6 +++--- > net/mptcp/sockopt.c | 4 +--- > net/smc/af_smc.c | 5 ++--- > 4 files changed, 6 insertions(+), 10 deletions(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index 88bf810394a5..0a58dc861908 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1194,10 +1194,9 @@ int sk_setsockopt(struct sock *sk, int level, > int optname, > > switch (optname) { > case SO_DEBUG: > + /* deprecated, but kept for compatibility. */ Not 100% sure about the language - but isn't the DEBUG feature *removed* rather than just *deprecated*? > if (val && !sockopt_capable(CAP_NET_ADMIN)) > ret = -EACCES; > - else > - sock_valbool_flag(sk, SOCK_DBG, valbool); > break; > case SO_REUSEADDR: > sk->sk_reuse = (valbool ? SK_CAN_REUSE : > SK_NO_REUSE); > @@ -1619,7 +1618,8 @@ int sk_getsockopt(struct sock *sk, int level, > int optname, > > switch (optname) { > case SO_DEBUG: > - v.val = sock_flag(sk, SOCK_DBG); > + /* deprecated. */ Same here. > + v.val = 0; > break; > > case SO_DONTROUTE: > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c > index da37e4541a5d..f6d90eef3d7c 100644 > --- a/net/mptcp/sockopt.c > +++ b/net/mptcp/sockopt.c > @@ -81,7 +81,7 @@ static void mptcp_sol_socket_sync_intval(struct > mptcp_sock *msk, int optname, in > > switch (optname) { > case SO_DEBUG: > - sock_valbool_flag(ssk, SOCK_DBG, !!val); > + /* deprecated. */ and here. > break; > case SO_KEEPALIVE: > if (ssk->sk_prot->keepalive) > @@ -1458,8 +1458,6 @@ static void sync_socket_options(struct > mptcp_sock *msk, struct sock *ssk) > sk_dst_reset(ssk); > } > > - sock_valbool_flag(ssk, SOCK_DBG, sock_flag(sk, SOCK_DBG)); > - > if (inet_csk(sk)->icsk_ca_ops != inet_csk(ssk)->icsk_ca_ops) > tcp_set_congestion_control(ssk, msk->ca_name, false, > true); > __tcp_sock_set_cork(ssk, !!msk->cork); > diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c > index 66763c74ab76..062e16a2766a 100644 > --- a/net/smc/af_smc.c > +++ b/net/smc/af_smc.c > @@ -445,7 +445,6 @@ static int smc_bind(struct socket *sock, struct > sockaddr *uaddr, > (1UL << SOCK_LINGER) | \ > (1UL << SOCK_BROADCAST) | \ > (1UL << SOCK_TIMESTAMP) | \ > - (1UL << SOCK_DBG) | \ > (1UL << SOCK_RCVTSTAMP) | \ > (1UL << SOCK_RCVTSTAMPNS) | \ > (1UL << SOCK_LOCALROUTE) | \ > @@ -511,8 +510,8 @@ static void smc_copy_sock_settings_to_clc(struct > smc_sock *smc) > > #define SK_FLAGS_CLC_TO_SMC ((1UL << SOCK_URGINLINE) | \ > (1UL << SOCK_KEEPOPEN) | \ > - (1UL << SOCK_LINGER) | \ > - (1UL << SOCK_DBG)) > + (1UL << SOCK_LINGER)) > + > /* copy only settings and flags relevant for smc from clc to smc > socket */ > static void smc_copy_sock_settings_to_smc(struct smc_sock *smc) > { The SMC changes look good to me, so feel free to add my Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
Hi Kuniyuki,
Thank you for your modifications, that's great!
Our CI (GitHub Action) did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7898465398
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/fbdc306e8139
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-normal
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 (NGI0 Core)
Hi Kuniyuki,
Thank you for your modifications, that's great!
Our CI (Cirrus) did some validations with a debug kernel and here is its report:
- KVM Validation: debug (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/4629533685448704
- Summary: https://api.cirrus-ci.com/v1/artifact/task/4629533685448704/summary/summary.txt
- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5755433592291328
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5755433592291328/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/fbdc306e8139
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 (NGI0 Core)
Hi Kuniyuki, On 13/02/2024 23:31, Kuniyuki Iwashima wrote: > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: > remove SOCK_DEBUG macro") removed the macro. > > Now is the time to deprecate the oldest socket option. Thank you for looking at this! My review here below is only about the modifications related to MPTCP. (...) > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c > index da37e4541a5d..f6d90eef3d7c 100644 > --- a/net/mptcp/sockopt.c > +++ b/net/mptcp/sockopt.c > @@ -81,7 +81,7 @@ static void mptcp_sol_socket_sync_intval(struct mptcp_sock *msk, int optname, in > > switch (optname) { > case SO_DEBUG: > - sock_valbool_flag(ssk, SOCK_DBG, !!val); > + /* deprecated. */ If it is now a NOOP, maybe better to: - remove SO_DEBUG from mptcp_sol_socket_sync_intval() and mptcp_setsockopt_sol_socket_int() - move it just above the "return 0" in mptcp_setsockopt_sol_socket() with the "deprecated" (or "removed") comment By doing that, we avoid a lock, plus going through the list of subflows for nothing. WDYT? > break; > case SO_KEEPALIVE: > if (ssk->sk_prot->keepalive) > @@ -1458,8 +1458,6 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk) > sk_dst_reset(ssk); > } > > - sock_valbool_flag(ssk, SOCK_DBG, sock_flag(sk, SOCK_DBG)); > - > if (inet_csk(sk)->icsk_ca_ops != inet_csk(ssk)->icsk_ca_ops) > tcp_set_congestion_control(ssk, msk->ca_name, false, true); > __tcp_sock_set_cork(ssk, !!msk->cork); The rest looks good to me. Cheers, Matt
From: Gerd Bayer <gbayer@linux.ibm.com> Date: Wed, 14 Feb 2024 09:14:33 +0100 > On Tue, 2024-02-13 at 14:31 -0800, Kuniyuki Iwashima wrote: > > Hi Kuniyuki, > > I'm adding the newly appointed SMC maintainers to chime in. > > > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 > > ("net: > > remove SOCK_DEBUG macro") removed the macro. > > > > Now is the time to deprecate the oldest socket option. > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > --- > > include/net/sock.h | 1 - > > net/core/sock.c | 6 +++--- > > net/mptcp/sockopt.c | 4 +--- > > net/smc/af_smc.c | 5 ++--- > > 4 files changed, 6 insertions(+), 10 deletions(-) > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > index 88bf810394a5..0a58dc861908 100644 > > --- a/net/core/sock.c > > +++ b/net/core/sock.c > > @@ -1194,10 +1194,9 @@ int sk_setsockopt(struct sock *sk, int level, > > int optname, > > > > switch (optname) { > > case SO_DEBUG: > > + /* deprecated, but kept for compatibility. */ > Not 100% sure about the language - but isn't the DEBUG feature > *removed* rather than just *deprecated*? SO_DEBUG is still here, and the user won't get -ENOPROTOOPT. That's why I wrote deprecated, no strong preference though. > > > if (val && !sockopt_capable(CAP_NET_ADMIN)) > > ret = -EACCES; > > - else > > - sock_valbool_flag(sk, SOCK_DBG, valbool); > > break; > > case SO_REUSEADDR: > > sk->sk_reuse = (valbool ? SK_CAN_REUSE : > > SK_NO_REUSE); > > @@ -1619,7 +1618,8 @@ int sk_getsockopt(struct sock *sk, int level, > > int optname, > > > > switch (optname) { > > case SO_DEBUG: > > - v.val = sock_flag(sk, SOCK_DBG); > > + /* deprecated. */ > Same here. > > > + v.val = 0; > > break; > > > > case SO_DONTROUTE: > > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c > > index da37e4541a5d..f6d90eef3d7c 100644 > > --- a/net/mptcp/sockopt.c > > +++ b/net/mptcp/sockopt.c > > @@ -81,7 +81,7 @@ static void mptcp_sol_socket_sync_intval(struct > > mptcp_sock *msk, int optname, in > > > > switch (optname) { > > case SO_DEBUG: > > - sock_valbool_flag(ssk, SOCK_DBG, !!val); > > + /* deprecated. */ > and here. > > > break; > > case SO_KEEPALIVE: > > if (ssk->sk_prot->keepalive) > > @@ -1458,8 +1458,6 @@ static void sync_socket_options(struct > > mptcp_sock *msk, struct sock *ssk) > > sk_dst_reset(ssk); > > } > > > > - sock_valbool_flag(ssk, SOCK_DBG, sock_flag(sk, SOCK_DBG)); > > - > > if (inet_csk(sk)->icsk_ca_ops != inet_csk(ssk)->icsk_ca_ops) > > tcp_set_congestion_control(ssk, msk->ca_name, false, > > true); > > __tcp_sock_set_cork(ssk, !!msk->cork); > > diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c > > index 66763c74ab76..062e16a2766a 100644 > > --- a/net/smc/af_smc.c > > +++ b/net/smc/af_smc.c > > @@ -445,7 +445,6 @@ static int smc_bind(struct socket *sock, struct > > sockaddr *uaddr, > > (1UL << SOCK_LINGER) | \ > > (1UL << SOCK_BROADCAST) | \ > > (1UL << SOCK_TIMESTAMP) | \ > > - (1UL << SOCK_DBG) | \ > > (1UL << SOCK_RCVTSTAMP) | \ > > (1UL << SOCK_RCVTSTAMPNS) | \ > > (1UL << SOCK_LOCALROUTE) | \ > > @@ -511,8 +510,8 @@ static void smc_copy_sock_settings_to_clc(struct > > smc_sock *smc) > > > > #define SK_FLAGS_CLC_TO_SMC ((1UL << SOCK_URGINLINE) | \ > > (1UL << SOCK_KEEPOPEN) | \ > > - (1UL << SOCK_LINGER) | \ > > - (1UL << SOCK_DBG)) > > + (1UL << SOCK_LINGER)) > > + > > /* copy only settings and flags relevant for smc from clc to smc > > socket */ > > static void smc_copy_sock_settings_to_smc(struct smc_sock *smc) > > { > The SMC changes look good to me, so feel free to add my > Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com> Thanks!
From: Matthieu Baerts <matttbe@kernel.org> Date: Wed, 14 Feb 2024 10:32:58 +0100 > Hi Kuniyuki, > > On 13/02/2024 23:31, Kuniyuki Iwashima wrote: > > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: > > remove SOCK_DEBUG macro") removed the macro. > > > > Now is the time to deprecate the oldest socket option. > > Thank you for looking at this! > > My review here below is only about the modifications related to MPTCP. > > (...) > > > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c > > index da37e4541a5d..f6d90eef3d7c 100644 > > --- a/net/mptcp/sockopt.c > > +++ b/net/mptcp/sockopt.c > > @@ -81,7 +81,7 @@ static void mptcp_sol_socket_sync_intval(struct mptcp_sock *msk, int optname, in > > > > switch (optname) { > > case SO_DEBUG: > > - sock_valbool_flag(ssk, SOCK_DBG, !!val); > > + /* deprecated. */ > > If it is now a NOOP, maybe better to: > > - remove SO_DEBUG from mptcp_sol_socket_sync_intval() and > mptcp_setsockopt_sol_socket_int() > > - move it just above the "return 0" in mptcp_setsockopt_sol_socket() > with the "deprecated" (or "removed") comment > > By doing that, we avoid a lock, plus going through the list of subflows > for nothing. > > WDYT? Sounds good! And I can apply the similar change to the general setsockopt() not to take lock_sock(). > > > break; > > case SO_KEEPALIVE: > > if (ssk->sk_prot->keepalive) > > @@ -1458,8 +1458,6 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk) > > sk_dst_reset(ssk); > > } > > > > - sock_valbool_flag(ssk, SOCK_DBG, sock_flag(sk, SOCK_DBG)); > > - > > if (inet_csk(sk)->icsk_ca_ops != inet_csk(ssk)->icsk_ca_ops) > > tcp_set_congestion_control(ssk, msk->ca_name, false, true); > > __tcp_sock_set_cork(ssk, !!msk->cork); > > The rest looks good to me. Thanks!
On Tue, Feb 13, 2024 at 3:32 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: > remove SOCK_DEBUG macro") removed the macro. > > Now is the time to deprecate the oldest socket option. > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- I would like to kindly implore you to please not remove the functionality of the SO_DEBUG socket option. This socket option is a key mechanism that the Google TCP team uses for automated testing of Linux TCP, including BBR congestion control. Widely used tools like netperf allow users to enable the SO_DEBUG socket option via the command line (-g in netperf). Then debugging code in the kernel can use the SOCK_DBG bit to decide whether to take special actions, such as logging debug information, which can be used to generate graphs or assertions about correct internal behavior. For example, the transperf network testing tool that our team open-sourced - https://github.com/google/transperf - uses the netperf -g/SO_DEBUG mechanism to trigger debug logging that we use for testing, troubleshooting, analysis, and development. The SO_DEBUG mechanism is nice in that it works well no matter what policy an application or benchmarking tool uses for choosing other attributes (like port numbers) that could conceivably be used to point out connections that should receive debug treatment. For example, most benchmarking or production workloads will effectively end up with random port numbers, which makes port numbers hard to use for triggering debug treatment. This mechanism is very simple and battle-tested, it works well, and IMHO it would be a tragedy to remove it. It would cause our team meaningful headaches to replace it. Please keep the SO_DEBUG socket option functionality as-is. :-) Thanks for your consideration on this! Best regards, neal
From: Neal Cardwell <ncardwell@google.com> Date: Thu, 15 Feb 2024 12:57:35 -0700 > On Tue, Feb 13, 2024 at 3:32 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: > > remove SOCK_DEBUG macro") removed the macro. > > > > Now is the time to deprecate the oldest socket option. > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > --- > > I would like to kindly implore you to please not remove the > functionality of the SO_DEBUG socket option. This socket option is a > key mechanism that the Google TCP team uses for automated testing of > Linux TCP, including BBR congestion control. > > Widely used tools like netperf allow users to enable the SO_DEBUG > socket option via the command line (-g in netperf). Then debugging > code in the kernel can use the SOCK_DBG bit to decide whether to take > special actions, such as logging debug information, which can be used > to generate graphs or assertions about correct internal behavior. For > example, the transperf network testing tool that our team open-sourced > - https://github.com/google/transperf - uses the netperf -g/SO_DEBUG > mechanism to trigger debug logging that we use for testing, > troubleshooting, analysis, and development. > > The SO_DEBUG mechanism is nice in that it works well no matter what > policy an application or benchmarking tool uses for choosing other > attributes (like port numbers) that could conceivably be used to point > out connections that should receive debug treatment. For example, most > benchmarking or production workloads will effectively end up with > random port numbers, which makes port numbers hard to use for > triggering debug treatment. > > This mechanism is very simple and battle-tested, it works well, and > IMHO it would be a tragedy to remove it. It would cause our team > meaningful headaches to replace it. Please keep the SO_DEBUG socket > option functionality as-is. :-) > > Thanks for your consideration on this! Oh that's an interesting use case! I didn't think of out-of-tree uses. Sure, I'll drop the patch. Thanks!
On Thu, Feb 15, 2024 at 1:16 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > From: Neal Cardwell <ncardwell@google.com> > Date: Thu, 15 Feb 2024 12:57:35 -0700 > > On Tue, Feb 13, 2024 at 3:32 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > > > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > > > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: > > > remove SOCK_DEBUG macro") removed the macro. > > > > > > Now is the time to deprecate the oldest socket option. > > > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > > --- > > > > I would like to kindly implore you to please not remove the > > functionality of the SO_DEBUG socket option. This socket option is a > > key mechanism that the Google TCP team uses for automated testing of > > Linux TCP, including BBR congestion control. > > > > Widely used tools like netperf allow users to enable the SO_DEBUG > > socket option via the command line (-g in netperf). Then debugging > > code in the kernel can use the SOCK_DBG bit to decide whether to take > > special actions, such as logging debug information, which can be used > > to generate graphs or assertions about correct internal behavior. For > > example, the transperf network testing tool that our team open-sourced > > - https://github.com/google/transperf - uses the netperf -g/SO_DEBUG > > mechanism to trigger debug logging that we use for testing, > > troubleshooting, analysis, and development. > > > > The SO_DEBUG mechanism is nice in that it works well no matter what > > policy an application or benchmarking tool uses for choosing other > > attributes (like port numbers) that could conceivably be used to point > > out connections that should receive debug treatment. For example, most > > benchmarking or production workloads will effectively end up with > > random port numbers, which makes port numbers hard to use for > > triggering debug treatment. > > > > This mechanism is very simple and battle-tested, it works well, and > > IMHO it would be a tragedy to remove it. It would cause our team > > meaningful headaches to replace it. Please keep the SO_DEBUG socket > > option functionality as-is. :-) > > > > Thanks for your consideration on this! > > Oh that's an interesting use case! > I didn't think of out-of-tree uses. > Sure, I'll drop the patch. > > Thanks! Great! Thank you! neal
diff --git a/include/net/sock.h b/include/net/sock.h index a9d99a9c583f..e20d55a36f9c 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -909,7 +909,6 @@ enum sock_flags { SOCK_TIMESTAMP, SOCK_ZAPPED, SOCK_USE_WRITE_QUEUE, /* whether to call sk->sk_write_space in sock_wfree */ - SOCK_DBG, /* %SO_DEBUG setting */ SOCK_RCVTSTAMP, /* %SO_TIMESTAMP setting */ SOCK_RCVTSTAMPNS, /* %SO_TIMESTAMPNS setting */ SOCK_LOCALROUTE, /* route locally only, %SO_DONTROUTE setting */ diff --git a/net/core/sock.c b/net/core/sock.c index 88bf810394a5..0a58dc861908 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1194,10 +1194,9 @@ int sk_setsockopt(struct sock *sk, int level, int optname, switch (optname) { case SO_DEBUG: + /* deprecated, but kept for compatibility. */ if (val && !sockopt_capable(CAP_NET_ADMIN)) ret = -EACCES; - else - sock_valbool_flag(sk, SOCK_DBG, valbool); break; case SO_REUSEADDR: sk->sk_reuse = (valbool ? SK_CAN_REUSE : SK_NO_REUSE); @@ -1619,7 +1618,8 @@ int sk_getsockopt(struct sock *sk, int level, int optname, switch (optname) { case SO_DEBUG: - v.val = sock_flag(sk, SOCK_DBG); + /* deprecated. */ + v.val = 0; break; case SO_DONTROUTE: diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index da37e4541a5d..f6d90eef3d7c 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -81,7 +81,7 @@ static void mptcp_sol_socket_sync_intval(struct mptcp_sock *msk, int optname, in switch (optname) { case SO_DEBUG: - sock_valbool_flag(ssk, SOCK_DBG, !!val); + /* deprecated. */ break; case SO_KEEPALIVE: if (ssk->sk_prot->keepalive) @@ -1458,8 +1458,6 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk) sk_dst_reset(ssk); } - sock_valbool_flag(ssk, SOCK_DBG, sock_flag(sk, SOCK_DBG)); - if (inet_csk(sk)->icsk_ca_ops != inet_csk(ssk)->icsk_ca_ops) tcp_set_congestion_control(ssk, msk->ca_name, false, true); __tcp_sock_set_cork(ssk, !!msk->cork); diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index 66763c74ab76..062e16a2766a 100644 --- a/net/smc/af_smc.c +++ b/net/smc/af_smc.c @@ -445,7 +445,6 @@ static int smc_bind(struct socket *sock, struct sockaddr *uaddr, (1UL << SOCK_LINGER) | \ (1UL << SOCK_BROADCAST) | \ (1UL << SOCK_TIMESTAMP) | \ - (1UL << SOCK_DBG) | \ (1UL << SOCK_RCVTSTAMP) | \ (1UL << SOCK_RCVTSTAMPNS) | \ (1UL << SOCK_LOCALROUTE) | \ @@ -511,8 +510,8 @@ static void smc_copy_sock_settings_to_clc(struct smc_sock *smc) #define SK_FLAGS_CLC_TO_SMC ((1UL << SOCK_URGINLINE) | \ (1UL << SOCK_KEEPOPEN) | \ - (1UL << SOCK_LINGER) | \ - (1UL << SOCK_DBG)) + (1UL << SOCK_LINGER)) + /* copy only settings and flags relevant for smc from clc to smc socket */ static void smc_copy_sock_settings_to_smc(struct smc_sock *smc) {
Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: remove SOCK_DEBUG macro") removed the macro. Now is the time to deprecate the oldest socket option. Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> --- include/net/sock.h | 1 - net/core/sock.c | 6 +++--- net/mptcp/sockopt.c | 4 +--- net/smc/af_smc.c | 5 ++--- 4 files changed, 6 insertions(+), 10 deletions(-)