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

["Ilpo Järvinen" <ij@kernel.org>]

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.)

-- 
 i.