Message-ID: <20200726165033.k3psh2erpjkrq3ks@skbuf>
Date:   Sun, 26 Jul 2020 19:50:33 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
Cc:     netdev@...r.kernel.org, stephen@...workplumber.org,
        davem@...emloft.net, roopa@...ulusnetworks.com, amarao@...vers.com,
        jiri@...nulli.us
Subject: Re: [RFC iproute2] ip: bridge: use -human to convert time-related
 values to seconds

On Sun, Jul 26, 2020 at 07:34:06PM +0300, Nikolay Aleksandrov wrote:
> On 26/07/2020 19:21, Vladimir Oltean wrote:
> > Hi Nikolay,
> > 
> > On Sun, Jul 26, 2020 at 01:43:23PM +0300, Nikolay Aleksandrov wrote:
> >> We have been printing and expecting the time-related bridge options
> >> scaled by USER_HZ which is confusing to users and hasn't been documented
> >> anywhere. Unfortunately that has been the case for years and people who
> >> use ip-link are scaling the values themselves by now. In order to help
> >> anyone who wants to show and set the values in normal time (seconds) we
> >> can use the ip -h[uman] argument. When it is supplied all values will be
> >> shown and expected in their normal time (currently all in seconds).
> >> The man page is also adjusted everywhere to explain the current scaling
> >> and the new option.
> >>
> >> Signed-off-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
> >> ---
> >> To avoid printing the values twice (i.e. the _ms solution) we can use
> >> the -human argument to scale them properly. Obviously -human is an alias
> >> right now for -human-readable, that's why I refer to it only as -human
> >> in the ip-link man page since we are now using it for setting values, too.
> >> Alternatively this can be any new option such as -Timescale.
> >>
> >> What do you think ?
> >>
> > 
> > The old ip-link also accepts commands of the form:
> > "ip -h link set br0 type bridge ageing_time 300"
> > 
> 
> That's why I mentioned the new option like -Timescale or some other unused,
> so you can be sure. I just didn't want to add a whole new ip argument just
> for this mess.
> 

That would have been more effective at eliminating the confusion than
-human, but I understand the desire to not introduce an entirely new
option.

> > so I'm not sure, with my user hat on, how can I know whether the
> > particular iproute2 binary I'm using understood what I meant or not.
> > 
> 
> Then I'll just stick to the original plan of duplicating the values and adding _ms
> version for them. That won't require any new options at least, it will just take
> more screen space.
> 

In a way, I think this is better.
If more time-related options ever appear in ip-link, I think that
introducing a flag like '-Timescale' now would force us to support
values in the old format (not scaled by USER_HZ) for those new options
too. But, if we were to introduce ageing_time_ms, etc, then the only
thing a new option would have to do is to name itself using a similar
pattern (with the unit of measurement at the end). Then it would be
clear that the non-scaled input isn't available.

But, all of this is just my 2 'bani' (Romanian, less valuable version of
cents). Let's hear from more people.

> >>  ip/iplink_bridge.c    | 49 +++++++++++++++++++++++++------------------
> >>  man/man8/ip-link.8.in | 33 +++++++++++++++++------------
> >>  2 files changed, 49 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
> >> index 3e81aa059cb3..8313cdbd0a92 100644
> >> --- a/ip/iplink_bridge.c
> >> +++ b/ip/iplink_bridge.c
> >> @@ -24,6 +24,7 @@
> >>  
> >>  static unsigned int xstats_print_attr;
> >>  static int filter_index;
> >> +static int hz = 1;
> >>  
> >>  static void print_explain(FILE *f)
> >>  {
> >> @@ -64,6 +65,9 @@ static void print_explain(FILE *f)
> >>  		"		  [ nf_call_arptables NF_CALL_ARPTABLES ]\n"
> >>  		"\n"
> >>  		"Where: VLAN_PROTOCOL := { 802.1Q | 802.1ad }\n"
> >> +		"Note that all time-related options are scaled by USER_HZ (%d), in order to\n"
> >> +		"set and show them as seconds please use the ip -h[uman] argument.\n",
> >> +		get_user_hz()
> >>  	);
> >>  }
> >>  
> >> @@ -85,31 +89,34 @@ static int bridge_parse_opt(struct link_util *lu, int argc, char **argv,
> >>  {
> >>  	__u32 val;
> >>  
> >> +	if (human_readable)
> >> +		hz = get_user_hz();
> >> +
> >>  	while (argc > 0) {
> >>  		if (matches(*argv, "forward_delay") == 0) {
> >>  			NEXT_ARG();
> >>  			if (get_u32(&val, *argv, 0))
> >>  				invarg("invalid forward_delay", *argv);
> >>  
> >> -			addattr32(n, 1024, IFLA_BR_FORWARD_DELAY, val);
> >> +			addattr32(n, 1024, IFLA_BR_FORWARD_DELAY, val * hz);
> >>  		} else if (matches(*argv, "hello_time") == 0) {
> >>  			NEXT_ARG();
> >>  			if (get_u32(&val, *argv, 0))
> >>  				invarg("invalid hello_time", *argv);
> >>  
> >> -			addattr32(n, 1024, IFLA_BR_HELLO_TIME, val);
> >> +			addattr32(n, 1024, IFLA_BR_HELLO_TIME, val * hz);
> >>  		} else if (matches(*argv, "max_age") == 0) {
> >>  			NEXT_ARG();
> >>  			if (get_u32(&val, *argv, 0))
> >>  				invarg("invalid max_age", *argv);
> >>  
> >> -			addattr32(n, 1024, IFLA_BR_MAX_AGE, val);
> >> +			addattr32(n, 1024, IFLA_BR_MAX_AGE, val * hz);
> >>  		} else if (matches(*argv, "ageing_time") == 0) {
> >>  			NEXT_ARG();
> >>  			if (get_u32(&val, *argv, 0))
> >>  				invarg("invalid ageing_time", *argv);
> > 
> > Also, you seem to have skipped updating the docs for ageing_time.
> > 
> 
> Right, it's an rfc for a discussion only, I'm not posting it for applying. :)
> It's not tested or polished in any way.
> 

Ok, I did test it, but this thing stood out to me.

> >>  
> >> -			addattr32(n, 1024, IFLA_BR_AGEING_TIME, val);
> >> +			addattr32(n, 1024, IFLA_BR_AGEING_TIME, val * hz);
> >>  		} else if (matches(*argv, "stp_state") == 0) {
> >>  			NEXT_ARG();
> >>  			if (get_u32(&val, *argv, 0))
> >> @@ -266,7 +273,7 @@ static int bridge_parse_opt(struct link_util *lu, int argc, char **argv,
> >>  				       *argv);
> >>  
> >>  			addattr64(n, 1024, IFLA_BR_MCAST_LAST_MEMBER_INTVL,
> >> -				  mcast_last_member_intvl);
> >> +				  mcast_last_member_intvl * hz);
> >>  		} else if (matches(*argv, "mcast_membership_interval") == 0) {
> >>  			__u64 mcast_membership_intvl;
> >>  
> >> @@ -276,7 +283,7 @@ static int bridge_parse_opt(struct link_util *lu, int argc, char **argv,
> >>  				       *argv);
> >>  
> >>  			addattr64(n, 1024, IFLA_BR_MCAST_MEMBERSHIP_INTVL,
> >> -				  mcast_membership_intvl);
> >> +				  mcast_membership_intvl * hz);
> >>  		} else if (matches(*argv, "mcast_querier_interval") == 0) {
> >>  			__u64 mcast_querier_intvl;
> >>  
> >> @@ -286,7 +293,7 @@ static int bridge_parse_opt(struct link_util *lu, int argc, char **argv,
> >>  				       *argv);
> >>  
> >>  			addattr64(n, 1024, IFLA_BR_MCAST_QUERIER_INTVL,
> >> -				  mcast_querier_intvl);
> >> +				  mcast_querier_intvl * hz);
> >>  		} else if (matches(*argv, "mcast_query_interval") == 0) {
> >>  			__u64 mcast_query_intvl;
> >>  
> >> @@ -296,7 +303,7 @@ static int bridge_parse_opt(struct link_util *lu, int argc, char **argv,
> >>  				       *argv);
> >>  
> >>  			addattr64(n, 1024, IFLA_BR_MCAST_QUERY_INTVL,
> >> -				  mcast_query_intvl);
> >> +				  mcast_query_intvl * hz);
> >>  		} else if (!matches(*argv, "mcast_query_response_interval")) {
> >>  			__u64 mcast_query_resp_intvl;
> >>  
> >> @@ -306,7 +313,7 @@ static int bridge_parse_opt(struct link_util *lu, int argc, char **argv,
> >>  				       *argv);
> >>  
> >>  			addattr64(n, 1024, IFLA_BR_MCAST_QUERY_RESPONSE_INTVL,
> >> -				  mcast_query_resp_intvl);
> >> +				  mcast_query_resp_intvl * hz);
> >>  		} else if (!matches(*argv, "mcast_startup_query_interval")) {
> >>  			__u64 mcast_startup_query_intvl;
> >>  
> >> @@ -316,7 +323,7 @@ static int bridge_parse_opt(struct link_util *lu, int argc, char **argv,
> >>  				       *argv);
> >>  
> >>  			addattr64(n, 1024, IFLA_BR_MCAST_STARTUP_QUERY_INTVL,
> >> -				  mcast_startup_query_intvl);
> >> +				  mcast_startup_query_intvl * hz);
> >>  		} else if (matches(*argv, "mcast_stats_enabled") == 0) {
> >>  			__u8 mcast_stats_enabled;
> >>  
> >> @@ -406,30 +413,32 @@ static void bridge_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> >>  {
> >>  	if (!tb)
> >>  		return;
> >> +	if (human_readable)
> >> +		hz = get_user_hz();
> >>  
> >>  	if (tb[IFLA_BR_FORWARD_DELAY])
> >>  		print_uint(PRINT_ANY,
> >>  			   "forward_delay",
> >>  			   "forward_delay %u ",
> >> -			   rta_getattr_u32(tb[IFLA_BR_FORWARD_DELAY]));
> >> +			   rta_getattr_u32(tb[IFLA_BR_FORWARD_DELAY]) / hz);
> >>  
> >>  	if (tb[IFLA_BR_HELLO_TIME])
> >>  		print_uint(PRINT_ANY,
> >>  			   "hello_time",
> >>  			   "hello_time %u ",
> >> -			   rta_getattr_u32(tb[IFLA_BR_HELLO_TIME]));
> >> +			   rta_getattr_u32(tb[IFLA_BR_HELLO_TIME]) / hz);
> >>  
> >>  	if (tb[IFLA_BR_MAX_AGE])
> >>  		print_uint(PRINT_ANY,
> >>  			   "max_age",
> >>  			   "max_age %u ",
> >> -			   rta_getattr_u32(tb[IFLA_BR_MAX_AGE]));
> >> +			   rta_getattr_u32(tb[IFLA_BR_MAX_AGE]) / hz);
> >>  
> >>  	if (tb[IFLA_BR_AGEING_TIME])
> >>  		print_uint(PRINT_ANY,
> >>  			   "ageing_time",
> >>  			   "ageing_time %u ",
> >> -			   rta_getattr_u32(tb[IFLA_BR_AGEING_TIME]));
> >> +			   rta_getattr_u32(tb[IFLA_BR_AGEING_TIME]) / hz);
> >>  
> >>  	if (tb[IFLA_BR_STP_STATE])
> >>  		print_uint(PRINT_ANY,
> >> @@ -605,37 +614,37 @@ static void bridge_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> >>  		print_lluint(PRINT_ANY,
> >>  			     "mcast_last_member_intvl",
> >>  			     "mcast_last_member_interval %llu ",
> >> -			     rta_getattr_u64(tb[IFLA_BR_MCAST_LAST_MEMBER_INTVL]));
> >> +			     rta_getattr_u64(tb[IFLA_BR_MCAST_LAST_MEMBER_INTVL]) / hz);
> >>  
> >>  	if (tb[IFLA_BR_MCAST_MEMBERSHIP_INTVL])
> >>  		print_lluint(PRINT_ANY,
> >>  			     "mcast_membership_intvl",
> >>  			     "mcast_membership_interval %llu ",
> >> -			     rta_getattr_u64(tb[IFLA_BR_MCAST_MEMBERSHIP_INTVL]));
> >> +			     rta_getattr_u64(tb[IFLA_BR_MCAST_MEMBERSHIP_INTVL]) / hz);
> >>  
> >>  	if (tb[IFLA_BR_MCAST_QUERIER_INTVL])
> >>  		print_lluint(PRINT_ANY,
> >>  			     "mcast_querier_intvl",
> >>  			     "mcast_querier_interval %llu ",
> >> -			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERIER_INTVL]));
> >> +			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERIER_INTVL]) / hz);
> >>  
> >>  	if (tb[IFLA_BR_MCAST_QUERY_INTVL])
> >>  		print_lluint(PRINT_ANY,
> >>  			     "mcast_query_intvl",
> >>  			     "mcast_query_interval %llu ",
> >> -			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERY_INTVL]));
> >> +			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERY_INTVL]) / hz);
> >>  
> >>  	if (tb[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL])
> >>  		print_lluint(PRINT_ANY,
> >>  			     "mcast_query_response_intvl",
> >>  			     "mcast_query_response_interval %llu ",
> >> -			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL]));
> >> +			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL]) / hz);
> >>  
> >>  	if (tb[IFLA_BR_MCAST_STARTUP_QUERY_INTVL])
> >>  		print_lluint(PRINT_ANY,
> >>  			     "mcast_startup_query_intvl",
> >>  			     "mcast_startup_query_interval %llu ",
> >> -			     rta_getattr_u64(tb[IFLA_BR_MCAST_STARTUP_QUERY_INTVL]));
> >> +			     rta_getattr_u64(tb[IFLA_BR_MCAST_STARTUP_QUERY_INTVL]) / hz);
> >>  
> >>  	if (tb[IFLA_BR_MCAST_STATS_ENABLED])
> >>  		print_uint(PRINT_ANY,
> >> diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
> >> index c6bd2c530547..efd8a877f7a3 100644
> >> --- a/man/man8/ip-link.8.in
> >> +++ b/man/man8/ip-link.8.in
> >> @@ -1431,9 +1431,11 @@ corresponds to the 2010 version of the HSR standard. Option "1" activates the
> >>  BRIDGE Type Support
> >>  For a link of type
> >>  .I BRIDGE
> >> -the following additional arguments are supported:
> >> +the following additional arguments are supported. Note that by default time-related options are scaled by USER_HZ (100), use -h[uman] argument to convert them from seconds when
> >> +setting and to seconds when using show.
> >> +
> >>  
> >> -.BI "ip link add " DEVICE " type bridge "
> >> +.BI "ip [-human] link add " DEVICE " type bridge "
> >>  [
> >>  .BI ageing_time " AGEING_TIME "
> >>  ] [
> >> @@ -1523,21 +1525,22 @@ address format, ie an address of the form 01:80:C2:00:00:0X, with X
> >>   in [0, 4..f].
> >>  
> >>  .BI forward_delay " FORWARD_DELAY "
> >> -- set the forwarding delay in seconds, ie the time spent in LISTENING
> >> +- set the forwarding delay, ie the time spent in LISTENING
> >>  state (before moving to LEARNING) and in LEARNING state (before
> >>  moving to FORWARDING). Only relevant if STP is enabled. Valid values
> >> -are between 2 and 30.
> >> +when -h[uman] is used (in seconds) are between 2 and 30.
> >>  
> >>  .BI hello_time " HELLO_TIME "
> >> -- set the time in seconds between hello packets sent by the bridge,
> >> -when it is a root bridge or a designated bridges.
> >> -Only relevant if STP is enabled. Valid values are between 1 and 10.
> >> +- set the time between hello packets sent by the bridge, when
> >> +it is a root bridge or a designated bridges.
> >> +Only relevant if STP is enabled. Valid values when -h[uman] is used
> >> +(in seconds) are between 1 and 10.
> >>  
> >>  .BI max_age " MAX_AGE "
> >>  - set the hello packet timeout, ie the time in seconds until another
> >>  bridge in the spanning tree is assumed to be dead, after reception of
> >>  its last hello message. Only relevant if STP is enabled. Valid values
> >> -are between 6 and 40.
> >> +when -h[uman] is used (in seconds) are between 6 and 40.
> >>  
> >>  .BI stp_state " STP_STATE "
> >>  - turn spanning tree protocol on
> >> @@ -1619,7 +1622,7 @@ IGMP querier, ie sending of multicast queries by the bridge (default: disabled).
> >>  after this delay has passed, the bridge will start to send its own queries
> >>  (as if
> >>  .BI mcast_querier
> >> -was enabled).
> >> +was enabled). When -h[uman] is used the value will be interpreted as seconds.
> >>  
> >>  .BI mcast_hash_elasticity " HASH_ELASTICITY "
> >>  - set multicast database hash elasticity, ie the maximum chain length
> >> @@ -1636,25 +1639,29 @@ message has been received (defaults to 2).
> >>  
> >>  .BI mcast_last_member_interval " LAST_MEMBER_INTERVAL "
> >>  - interval between queries to find remaining members of a group,
> >> -after a "leave" message is received.
> >> +after a "leave" message is received. When -h[uman] is used the value
> >> +will be interpreted as seconds.
> >>  
> >>  .BI mcast_startup_query_count " STARTUP_QUERY_COUNT "
> >>  - set the number of IGMP queries to send during startup phase (defaults to 2).
> >>  
> >>  .BI mcast_startup_query_interval " STARTUP_QUERY_INTERVAL "
> >> -- interval between queries in the startup phase.
> >> +- interval between queries in the startup phase. When -h[uman] is used the
> >> +value will be interpreted as seconds.
> >>  
> >>  .BI mcast_query_interval " QUERY_INTERVAL "
> >>  - interval between queries sent by the bridge after the end of the
> >> -startup phase.
> >> +startup phase. When -h[uman] is used the value will be interpreted as seconds.
> >>  
> >>  .BI mcast_query_response_interval " QUERY_RESPONSE_INTERVAL "
> >>  - set the Max Response Time/Maximum Response Delay for IGMP/MLD
> >> -queries sent by the bridge.
> >> +queries sent by the bridge. When -h[uman] is used the value will
> >> +be interpreted as seconds.
> >>  
> >>  .BI mcast_membership_interval " MEMBERSHIP_INTERVAL "
> >>  - delay after which the bridge will leave a group,
> >>  if no membership reports for this group are received.
> >> +When -h[uman] is used the value will be interpreted as seconds.
> >>  
> >>  .BI mcast_stats_enabled " MCAST_STATS_ENABLED "
> >>  - enable
> >> -- 
> >> 2.25.4
> >>
> > 
> > Thanks,
> > -Vladimir
> > 
> 

Thanks,
-Vladimir

Powered by blists - more mailing lists