diff mbox series

[v4,net-next,14/14] net: sysctl: introduce sysctl SYSCTL_FIVE

Message ID 20241021215910.59767-15-chia-yu.chang@nokia-bell-labs.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series AccECN protocol preparation patch series | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 549 this patch: 549
netdev/build_tools success Errors and warnings before: 157 (+1) this patch: 157 (+1)
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 1021 this patch: 1021
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 14641 this patch: 14641
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 39 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 25 this patch: 25
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-10-23--12-00 (tests: 777)

Commit Message

Chia-Yu Chang (Nokia) Oct. 21, 2024, 9:59 p.m. UTC 
From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>

Add SYSCTL_FIVE for new AccECN feedback modes of net.ipv4.tcp_ecn.

Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
---
 include/linux/sysctl.h | 17 +++++++++--------
 kernel/sysctl.c        |  3 ++-
 2 files changed, 11 insertions(+), 9 deletions(-)

Comments

Paolo Abeni Oct. 29, 2024, 12:26 p.m. UTC | #1 
On 10/21/24 23:59, chia-yu.chang@nokia-bell-labs.com wrote:
> From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> 
> Add SYSCTL_FIVE for new AccECN feedback modes of net.ipv4.tcp_ecn.

How many sysctl entries will use such value? If just one you are better
off not introducing the new sysctl value and instead using a static
constant in the tcp code.

Also this patch makes the commit message in the previous one incorrect.
Please adjust that.

Side note: on new version, you should include the changelog in the
affected patches, after a '---' separator, to help the reviewers.

Thanks,

Paolo
Ilpo Järvinen Oct. 29, 2024, 9:29 p.m. UTC | #2 
On Mon, 21 Oct 2024, chia-yu.chang@nokia-bell-labs.com wrote:

> From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> 
> Add SYSCTL_FIVE for new AccECN feedback modes of net.ipv4.tcp_ecn.
> 
> Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> ---
>  include/linux/sysctl.h | 17 +++++++++--------
>  kernel/sysctl.c        |  3 ++-
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index aa4c6d44aaa0..37c95a70c10e 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -37,21 +37,22 @@ struct ctl_table_root;
>  struct ctl_table_header;
>  struct ctl_dir;
>  
> -/* Keep the same order as in fs/proc/proc_sysctl.c */
> +/* Keep the same order as in kernel/sysctl.c */
>  #define SYSCTL_ZERO			((void *)&sysctl_vals[0])
>  #define SYSCTL_ONE			((void *)&sysctl_vals[1])
>  #define SYSCTL_TWO			((void *)&sysctl_vals[2])
>  #define SYSCTL_THREE			((void *)&sysctl_vals[3])
>  #define SYSCTL_FOUR			((void *)&sysctl_vals[4])
> -#define SYSCTL_ONE_HUNDRED		((void *)&sysctl_vals[5])
> -#define SYSCTL_TWO_HUNDRED		((void *)&sysctl_vals[6])
> -#define SYSCTL_ONE_THOUSAND		((void *)&sysctl_vals[7])
> -#define SYSCTL_THREE_THOUSAND		((void *)&sysctl_vals[8])
> -#define SYSCTL_INT_MAX			((void *)&sysctl_vals[9])
> +#define SYSCTL_FIVE			((void *)&sysctl_vals[5])
> +#define SYSCTL_ONE_HUNDRED		((void *)&sysctl_vals[6])
> +#define SYSCTL_TWO_HUNDRED		((void *)&sysctl_vals[7])
> +#define SYSCTL_ONE_THOUSAND		((void *)&sysctl_vals[8])
> +#define SYSCTL_THREE_THOUSAND		((void *)&sysctl_vals[9])
> +#define SYSCTL_INT_MAX			((void *)&sysctl_vals[10])
>  
>  /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
> -#define SYSCTL_MAXOLDUID		((void *)&sysctl_vals[10])
> -#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[11])
> +#define SYSCTL_MAXOLDUID		((void *)&sysctl_vals[11])
> +#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[12])
>  
>  extern const int sysctl_vals[];
>  
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 79e6cb1d5c48..68b6ca67a0c6 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -82,7 +82,8 @@
>  #endif
>  
>  /* shared constants to be used in various sysctls */
> -const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, 1000, 3000, INT_MAX, 65535, -1 };
> +const int sysctl_vals[] = { 0, 1, 2, 3, 4, 5, 100, 200, 1000, 3000, INT_MAX,
> +			   65535, -1 };
>  EXPORT_SYMBOL(sysctl_vals);
>  
>  const unsigned long sysctl_long_vals[] = { 0, 1, LONG_MAX };

Hi,

I know I suggested you to put this change into this first batch of 
AccECN patches but I've since come to other thoughts.

I think this should be moved to very tail of AccECN changes in the series
and joined together with the part of change which allows setting 
net.ipv4.tcp_ecn to those higher values. Currently the latter is done in 
the AccECN negotion patch (IIRC) but that part should be moved into a 
separate patch with this change only after all AccECN patches have been 
included to prevent enabling AccECN in incomplete form.

(This comment is orthogonal to Paolo's suggestion to use static constant.
So whichever form is chosen, it should be with the net.ipv4.tcp_ecn 
change at the end of AccECN changes.)
Joel Granados Oct. 31, 2024, 2:08 p.m. UTC | #3 
On Mon, Oct 21, 2024 at 11:59:10PM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> 
> Add SYSCTL_FIVE for new AccECN feedback modes of net.ipv4.tcp_ecn.
> 
> Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> ---
>  include/linux/sysctl.h | 17 +++++++++--------
>  kernel/sysctl.c        |  3 ++-
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index aa4c6d44aaa0..37c95a70c10e 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -37,21 +37,22 @@ struct ctl_table_root;
>  struct ctl_table_header;
>  struct ctl_dir;
>  
> -/* Keep the same order as in fs/proc/proc_sysctl.c */
> +/* Keep the same order as in kernel/sysctl.c */
>  #define SYSCTL_ZERO			((void *)&sysctl_vals[0])
>  #define SYSCTL_ONE			((void *)&sysctl_vals[1])
>  #define SYSCTL_TWO			((void *)&sysctl_vals[2])
>  #define SYSCTL_THREE			((void *)&sysctl_vals[3])
>  #define SYSCTL_FOUR			((void *)&sysctl_vals[4])
> -#define SYSCTL_ONE_HUNDRED		((void *)&sysctl_vals[5])
> -#define SYSCTL_TWO_HUNDRED		((void *)&sysctl_vals[6])
> -#define SYSCTL_ONE_THOUSAND		((void *)&sysctl_vals[7])
> -#define SYSCTL_THREE_THOUSAND		((void *)&sysctl_vals[8])
> -#define SYSCTL_INT_MAX			((void *)&sysctl_vals[9])
> +#define SYSCTL_FIVE			((void *)&sysctl_vals[5])
Is it necessary to insert the value instead of appending it to the end
of sysctl_vals? I would actually consider Paolo Abeni's suggestion to
just use a constant if you are using it only in one place.

> +#define SYSCTL_ONE_HUNDRED		((void *)&sysctl_vals[6])
> +#define SYSCTL_TWO_HUNDRED		((void *)&sysctl_vals[7])
> +#define SYSCTL_ONE_THOUSAND		((void *)&sysctl_vals[8])
> +#define SYSCTL_THREE_THOUSAND		((void *)&sysctl_vals[9])
> +#define SYSCTL_INT_MAX			((void *)&sysctl_vals[10])
>  
>  /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
> -#define SYSCTL_MAXOLDUID		((void *)&sysctl_vals[10])
> -#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[11])
> +#define SYSCTL_MAXOLDUID		((void *)&sysctl_vals[11])
> +#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[12])
>  
>  extern const int sysctl_vals[];
>  
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 79e6cb1d5c48..68b6ca67a0c6 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -82,7 +82,8 @@
>  #endif
>  
>  /* shared constants to be used in various sysctls */
> -const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, 1000, 3000, INT_MAX, 65535, -1 };
> +const int sysctl_vals[] = { 0, 1, 2, 3, 4, 5, 100, 200, 1000, 3000, INT_MAX,
> +			   65535, -1 };
>  EXPORT_SYMBOL(sysctl_vals);
>  
>  const unsigned long sysctl_long_vals[] = { 0, 1, LONG_MAX };
> -- 
> 2.34.1
>
Chia-Yu Chang (Nokia) Oct. 31, 2024, 3:44 p.m. UTC | #4 
Hi Paolo and Joel,

	We will remove this patch as we check this will be only used by tcp_ecn in the upcoming patch.

Brs,
Chia-Yu

-----Original Message-----
From: Joel Granados <joel.granados@kernel.org> 
Sent: Thursday, October 31, 2024 3:09 PM
To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; dsahern@kernel.org; netfilter-devel@vger.kernel.org; kadlec@netfilter.org; coreteam@netfilter.org; pablo@netfilter.org; bpf@vger.kernel.org; linux-fsdevel@vger.kernel.org; kees@kernel.org; mcgrof@kernel.org; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white@cablelabs.com; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com; cheshire@apple.com; rs.ietf@gmx.at; Jason_Livingood@comcast.com; vidhi_goel@apple.com
Subject: Re: [PATCH v4 net-next 14/14] net: sysctl: introduce sysctl SYSCTL_FIVE

[Some people who received this message don't often get email from joel.granados@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.



On Mon, Oct 21, 2024 at 11:59:10PM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
>
> Add SYSCTL_FIVE for new AccECN feedback modes of net.ipv4.tcp_ecn.
>
> Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> ---
>  include/linux/sysctl.h | 17 +++++++++--------
>  kernel/sysctl.c        |  3 ++-
>  2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index 
> aa4c6d44aaa0..37c95a70c10e 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -37,21 +37,22 @@ struct ctl_table_root;  struct ctl_table_header;  
> struct ctl_dir;
>
> -/* Keep the same order as in fs/proc/proc_sysctl.c */
> +/* Keep the same order as in kernel/sysctl.c */
>  #define SYSCTL_ZERO                  ((void *)&sysctl_vals[0])
>  #define SYSCTL_ONE                   ((void *)&sysctl_vals[1])
>  #define SYSCTL_TWO                   ((void *)&sysctl_vals[2])
>  #define SYSCTL_THREE                 ((void *)&sysctl_vals[3])
>  #define SYSCTL_FOUR                  ((void *)&sysctl_vals[4])
> -#define SYSCTL_ONE_HUNDRED           ((void *)&sysctl_vals[5])
> -#define SYSCTL_TWO_HUNDRED           ((void *)&sysctl_vals[6])
> -#define SYSCTL_ONE_THOUSAND          ((void *)&sysctl_vals[7])
> -#define SYSCTL_THREE_THOUSAND                ((void *)&sysctl_vals[8])
> -#define SYSCTL_INT_MAX                       ((void *)&sysctl_vals[9])
> +#define SYSCTL_FIVE                  ((void *)&sysctl_vals[5])
Is it necessary to insert the value instead of appending it to the end of sysctl_vals? I would actually consider Paolo Abeni's suggestion to just use a constant if you are using it only in one place.

> +#define SYSCTL_ONE_HUNDRED           ((void *)&sysctl_vals[6])
> +#define SYSCTL_TWO_HUNDRED           ((void *)&sysctl_vals[7])
> +#define SYSCTL_ONE_THOUSAND          ((void *)&sysctl_vals[8])
> +#define SYSCTL_THREE_THOUSAND                ((void *)&sysctl_vals[9])
> +#define SYSCTL_INT_MAX                       ((void *)&sysctl_vals[10])
>
>  /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
> -#define SYSCTL_MAXOLDUID             ((void *)&sysctl_vals[10])
> -#define SYSCTL_NEG_ONE                       ((void *)&sysctl_vals[11])
> +#define SYSCTL_MAXOLDUID             ((void *)&sysctl_vals[11])
> +#define SYSCTL_NEG_ONE                       ((void *)&sysctl_vals[12])
>
>  extern const int sysctl_vals[];
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 
> 79e6cb1d5c48..68b6ca67a0c6 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -82,7 +82,8 @@
>  #endif
>
>  /* shared constants to be used in various sysctls */ -const int 
> sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, 1000, 3000, INT_MAX, 65535, 
> -1 };
> +const int sysctl_vals[] = { 0, 1, 2, 3, 4, 5, 100, 200, 1000, 3000, INT_MAX,
> +                        65535, -1 };
>  EXPORT_SYMBOL(sysctl_vals);
>
>  const unsigned long sysctl_long_vals[] = { 0, 1, LONG_MAX };
> --
> 2.34.1
>

--

Joel Granados
diff mbox series

Patch

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index aa4c6d44aaa0..37c95a70c10e 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -37,21 +37,22 @@  struct ctl_table_root;
 struct ctl_table_header;
 struct ctl_dir;
 
-/* Keep the same order as in fs/proc/proc_sysctl.c */
+/* Keep the same order as in kernel/sysctl.c */
 #define SYSCTL_ZERO			((void *)&sysctl_vals[0])
 #define SYSCTL_ONE			((void *)&sysctl_vals[1])
 #define SYSCTL_TWO			((void *)&sysctl_vals[2])
 #define SYSCTL_THREE			((void *)&sysctl_vals[3])
 #define SYSCTL_FOUR			((void *)&sysctl_vals[4])
-#define SYSCTL_ONE_HUNDRED		((void *)&sysctl_vals[5])
-#define SYSCTL_TWO_HUNDRED		((void *)&sysctl_vals[6])
-#define SYSCTL_ONE_THOUSAND		((void *)&sysctl_vals[7])
-#define SYSCTL_THREE_THOUSAND		((void *)&sysctl_vals[8])
-#define SYSCTL_INT_MAX			((void *)&sysctl_vals[9])
+#define SYSCTL_FIVE			((void *)&sysctl_vals[5])
+#define SYSCTL_ONE_HUNDRED		((void *)&sysctl_vals[6])
+#define SYSCTL_TWO_HUNDRED		((void *)&sysctl_vals[7])
+#define SYSCTL_ONE_THOUSAND		((void *)&sysctl_vals[8])
+#define SYSCTL_THREE_THOUSAND		((void *)&sysctl_vals[9])
+#define SYSCTL_INT_MAX			((void *)&sysctl_vals[10])
 
 /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
-#define SYSCTL_MAXOLDUID		((void *)&sysctl_vals[10])
-#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[11])
+#define SYSCTL_MAXOLDUID		((void *)&sysctl_vals[11])
+#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[12])
 
 extern const int sysctl_vals[];
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 79e6cb1d5c48..68b6ca67a0c6 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -82,7 +82,8 @@ 
 #endif
 
 /* shared constants to be used in various sysctls */
-const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, 1000, 3000, INT_MAX, 65535, -1 };
+const int sysctl_vals[] = { 0, 1, 2, 3, 4, 5, 100, 200, 1000, 3000, INT_MAX,
+			   65535, -1 };
 EXPORT_SYMBOL(sysctl_vals);
 
 const unsigned long sysctl_long_vals[] = { 0, 1, LONG_MAX };