Re: [RFC PATCH net-next v2 02/10] ipv4: Add a sysctl to control multipath hash fields

[Ido Schimmel <idosch@idosch.org>]

On Tue, May 11, 2021 at 09:49:30AM -0600, David Ahern wrote:
> On 5/9/21 9:16 AM, Ido Schimmel wrote:
> > diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> > index a62934b9f15a..da627c4d633a 100644
> > --- a/net/ipv4/sysctl_net_ipv4.c
> > +++ b/net/ipv4/sysctl_net_ipv4.c
> > @@ -19,6 +19,7 @@
> >  #include <net/snmp.h>
> >  #include <net/icmp.h>
> >  #include <net/ip.h>
> > +#include <net/ip_fib.h>
> >  #include <net/route.h>
> >  #include <net/tcp.h>
> >  #include <net/udp.h>
> > @@ -48,6 +49,8 @@ static int ip_ping_group_range_min[] = { 0, 0 };
> >  static int ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
> >  static u32 u32_max_div_HZ = UINT_MAX / HZ;
> >  static int one_day_secs = 24 * 3600;
> > +static u32 fib_multipath_hash_fields_all_mask __maybe_unused =
> > +	FIB_MULTIPATH_HASH_FIELD_ALL_MASK;
> >  
> >  /* obsolete */
> >  static int sysctl_tcp_low_latency __read_mostly;
> > @@ -1052,6 +1055,14 @@ static struct ctl_table ipv4_net_table[] = {
> >  		.extra1		= SYSCTL_ZERO,
> >  		.extra2		= &two,
> >  	},
> > +	{
> > +		.procname	= "fib_multipath_hash_fields",
> > +		.data		= &init_net.ipv4.sysctl_fib_multipath_hash_fields,
> > +		.maxlen		= sizeof(u32),
> > +		.mode		= 0644,
> > +		.proc_handler	= proc_douintvec_minmax,
> > +		.extra2		= &fib_multipath_hash_fields_all_mask,
> 
> no .extra1 means 0 is allowed which effectively disables hashing and
> multipath selection; only the first leg will be used. Is that intended?

I didn't see any reason to forbid it, but I don't see any reason to use
it with '0' either. With this patch:

diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 81343037de06..4fa77f182dcb 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -1078,6 +1078,7 @@ static struct ctl_table ipv4_net_table[] = {
 		.maxlen		= sizeof(u32),
 		.mode		= 0644,
 		.proc_handler	= proc_fib_multipath_hash_fields,
+		.extra1		= SYSCTL_ONE,
 		.extra2		= &fib_multipath_hash_fields_all_mask,
 	},
 #endif

We get:

# sysctl -w net.ipv4.fib_multipath_hash_fields=0
sysctl: setting key "net.ipv4.fib_multipath_hash_fields": Invalid argument

I assume you want to see this change in the next version (and for IPv6)?

> 
> 
> 
> > +	},
> >  #endif
> >  	{
> >  		.procname	= "ip_unprivileged_port_start",
> > 
>