* Bug in iproute2 man page (or in iproute itself)
@ 2020-07-24 12:45 George Shuklin
2020-07-24 15:35 ` Stephen Hemminger
2020-07-24 16:15 ` [RFT iproute2] iplink_bridge: scale all time values by USER_HZ Stephen Hemminger
0 siblings, 2 replies; 14+ messages in thread
From: George Shuklin @ 2020-07-24 12:45 UTC (permalink / raw)
To: netdev
Hello.
I'm writing Ansible module for iproute, and I found some discrepancies
between man page and actual behavior for ip link add type bridge.
man page said:
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.
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.
In reality 'ip link add type bridge' requires hello_time to be at least
100, and max_age to be at least 600. I suspect there is a missing x100
multiplier, either in docs, or in the code.
(I'm not sure where I should send bugreports for iproute2).
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Bug in iproute2 man page (or in iproute itself)
2020-07-24 12:45 Bug in iproute2 man page (or in iproute itself) George Shuklin
@ 2020-07-24 15:35 ` Stephen Hemminger
2020-07-24 16:15 ` [RFT iproute2] iplink_bridge: scale all time values by USER_HZ Stephen Hemminger
1 sibling, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2020-07-24 15:35 UTC (permalink / raw)
To: George Shuklin; +Cc: netdev
On Fri, 24 Jul 2020 15:45:20 +0300
George Shuklin <amarao@servers.com> wrote:
> Hello.
>
> I'm writing Ansible module for iproute, and I found some discrepancies
> between man page and actual behavior for ip link add type bridge.
>
> man page said:
>
> 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.
>
> 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.
>
> In reality 'ip link add type bridge' requires hello_time to be at least
> 100, and max_age to be at least 600. I suspect there is a missing x100
> multiplier, either in docs, or in the code.
>
> (I'm not sure where I should send bugreports for iproute2).
>
Good catch all the time related values in netlink API should be
scaled by the user hz value (which is 100).
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFT iproute2] iplink_bridge: scale all time values by USER_HZ
2020-07-24 12:45 Bug in iproute2 man page (or in iproute itself) George Shuklin
2020-07-24 15:35 ` Stephen Hemminger
@ 2020-07-24 16:15 ` Stephen Hemminger
2020-07-24 16:24 ` nikolay
1 sibling, 1 reply; 14+ messages in thread
From: Stephen Hemminger @ 2020-07-24 16:15 UTC (permalink / raw)
To: George Shuklin; +Cc: netdev, nikolay, jiri, amarao
The bridge portion of ip command was not scaling so the
values were off.
The netlink API's for setting and reading timers all conform
to the kernel standard of scaling the values by USER_HZ (100).
Fixes: 28d84b429e4e ("add bridge master device support")
Fixes: 7f3d55922645 ("iplink: bridge: add support for IFLA_BR_MCAST_MEMBERSHIP_INTVL")
Fixes: 10082a253fb2 ("iplink: bridge: add support for IFLA_BR_MCAST_LAST_MEMBER_INTVL")
Fixes: 1f2244b851dd ("iplink: bridge: add support for IFLA_BR_MCAST_QUERIER_INTVL")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
Compile tested only.
ip/iplink_bridge.c | 45 ++++++++++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
index 3e81aa059cb3..48495a08c484 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 unsigned int hz;
static void print_explain(FILE *f)
{
@@ -85,19 +86,22 @@ static int bridge_parse_opt(struct link_util *lu, int argc, char **argv,
{
__u32 val;
+ if (!hz)
+ 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))
@@ -109,7 +113,7 @@ static int bridge_parse_opt(struct link_util *lu, int argc, char **argv,
if (get_u32(&val, *argv, 0))
invarg("invalid ageing_time", *argv);
- 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 +270,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 +280,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 +290,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 +300,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 +310,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 +320,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;
@@ -407,29 +411,32 @@ static void bridge_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
if (!tb)
return;
+ if (!hz)
+ 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 +612,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,
--
2.27.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFT iproute2] iplink_bridge: scale all time values by USER_HZ
2020-07-24 16:15 ` [RFT iproute2] iplink_bridge: scale all time values by USER_HZ Stephen Hemminger
@ 2020-07-24 16:24 ` nikolay
2020-07-24 19:05 ` Stephen Hemminger
2020-07-25 0:31 ` David Miller
0 siblings, 2 replies; 14+ messages in thread
From: nikolay @ 2020-07-24 16:24 UTC (permalink / raw)
To: Stephen Hemminger, George Shuklin; +Cc: netdev, jiri, amarao
On 24 July 2020 19:15:17 EEST, Stephen Hemminger <stephen@networkplumber.org> wrote:
>
>The bridge portion of ip command was not scaling so the
>values were off.
>
>The netlink API's for setting and reading timers all conform
>to the kernel standard of scaling the values by USER_HZ (100).
>
>Fixes: 28d84b429e4e ("add bridge master device support")
>Fixes: 7f3d55922645 ("iplink: bridge: add support for
>IFLA_BR_MCAST_MEMBERSHIP_INTVL")
>Fixes: 10082a253fb2 ("iplink: bridge: add support for
>IFLA_BR_MCAST_LAST_MEMBER_INTVL")
>Fixes: 1f2244b851dd ("iplink: bridge: add support for
>IFLA_BR_MCAST_QUERIER_INTVL")
>Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>---
While I agree this should have been done from the start, it's too late to change.
We'll break everyone using these commands.
We have been discussing to add _ms version of all these which do the proper scaling. I'd prefer that, it's least disruptive
to users.
Every user of the old commands scales the values by now.
>
>Compile tested only.
>
>
> ip/iplink_bridge.c | 45 ++++++++++++++++++++++++++-------------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
>diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
>index 3e81aa059cb3..48495a08c484 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 unsigned int hz;
>
> static void print_explain(FILE *f)
> {
>@@ -85,19 +86,22 @@ static int bridge_parse_opt(struct link_util *lu,
>int argc, char **argv,
> {
> __u32 val;
>
>+ if (!hz)
>+ 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))
>@@ -109,7 +113,7 @@ static int bridge_parse_opt(struct link_util *lu,
>int argc, char **argv,
> if (get_u32(&val, *argv, 0))
> invarg("invalid ageing_time", *argv);
>
>- 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 +270,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 +280,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 +290,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 +300,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 +310,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 +320,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;
>
>@@ -407,29 +411,32 @@ static void bridge_print_opt(struct link_util
>*lu, FILE *f, struct rtattr *tb[])
> if (!tb)
> return;
>
>+ if (!hz)
>+ 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 +612,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,
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFT iproute2] iplink_bridge: scale all time values by USER_HZ
2020-07-24 16:24 ` nikolay
@ 2020-07-24 19:05 ` Stephen Hemminger
2020-07-24 22:51 ` Nikolay Aleksandrov
2020-07-25 0:31 ` David Miller
1 sibling, 1 reply; 14+ messages in thread
From: Stephen Hemminger @ 2020-07-24 19:05 UTC (permalink / raw)
To: nikolay; +Cc: George Shuklin, netdev, jiri
On Fri, 24 Jul 2020 19:24:35 +0300
nikolay@cumulusnetworks.com wrote:
> On 24 July 2020 19:15:17 EEST, Stephen Hemminger <stephen@networkplumber.org> wrote:
> >
> >The bridge portion of ip command was not scaling so the
> >values were off.
> >
> >The netlink API's for setting and reading timers all conform
> >to the kernel standard of scaling the values by USER_HZ (100).
> >
> >Fixes: 28d84b429e4e ("add bridge master device support")
> >Fixes: 7f3d55922645 ("iplink: bridge: add support for
> >IFLA_BR_MCAST_MEMBERSHIP_INTVL")
> >Fixes: 10082a253fb2 ("iplink: bridge: add support for
> >IFLA_BR_MCAST_LAST_MEMBER_INTVL")
> >Fixes: 1f2244b851dd ("iplink: bridge: add support for
> >IFLA_BR_MCAST_QUERIER_INTVL")
> >Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >---
>
> While I agree this should have been done from the start, it's too late to change.
> We'll break everyone using these commands.
> We have been discussing to add _ms version of all these which do the proper scaling. I'd prefer that, it's least disruptive
> to users.
>
> Every user of the old commands scales the values by now.
So bridge is inconsistent with all other api's in iproute2!
And the bridge option in ip link is scaled differently than the bridge-utils or sysfs.
Maybe an environment variable?
Or add new fixed syntax option and don't show the old syntax?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFT iproute2] iplink_bridge: scale all time values by USER_HZ
2020-07-24 19:05 ` Stephen Hemminger
@ 2020-07-24 22:51 ` Nikolay Aleksandrov
2020-07-24 23:18 ` Vladimir Oltean
0 siblings, 1 reply; 14+ messages in thread
From: Nikolay Aleksandrov @ 2020-07-24 22:51 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: George Shuklin, netdev, jiri
On 24/07/2020 22:05, Stephen Hemminger wrote:
> On Fri, 24 Jul 2020 19:24:35 +0300
> nikolay@cumulusnetworks.com wrote:
>
>> On 24 July 2020 19:15:17 EEST, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>>
>>> The bridge portion of ip command was not scaling so the
>>> values were off.
>>>
>>> The netlink API's for setting and reading timers all conform
>>> to the kernel standard of scaling the values by USER_HZ (100).
>>>
>>> Fixes: 28d84b429e4e ("add bridge master device support")
>>> Fixes: 7f3d55922645 ("iplink: bridge: add support for
>>> IFLA_BR_MCAST_MEMBERSHIP_INTVL")
>>> Fixes: 10082a253fb2 ("iplink: bridge: add support for
>>> IFLA_BR_MCAST_LAST_MEMBER_INTVL")
>>> Fixes: 1f2244b851dd ("iplink: bridge: add support for
>>> IFLA_BR_MCAST_QUERIER_INTVL")
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>
>> While I agree this should have been done from the start, it's too late to change.
>> We'll break everyone using these commands.
>> We have been discussing to add _ms version of all these which do the proper scaling. I'd prefer that, it's least disruptive
>> to users.
>>
>> Every user of the old commands scales the values by now.
>
> So bridge is inconsistent with all other api's in iproute2!
> And the bridge option in ip link is scaled differently than the bridge-utils or sysfs.
>
Yeah, that is not new, it's been like that for years.
> Maybe an environment variable?
> Or add new fixed syntax option and don't show the old syntax?
>
Anything that doesn't disrupt normal processing sounds good.
So it must be opt-in, we can't just change the default overnight.
The _ms version of all values is that - new fixed syntax options for all current options
and anyone who wants to use a "normal" time-based option would use those as they'll be
automatically scaled.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFT iproute2] iplink_bridge: scale all time values by USER_HZ
2020-07-24 22:51 ` Nikolay Aleksandrov
@ 2020-07-24 23:18 ` Vladimir Oltean
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2020-07-24 23:18 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: Stephen Hemminger, George Shuklin, netdev, jiri
On Sat, Jul 25, 2020 at 01:51:02AM +0300, Nikolay Aleksandrov wrote:
> On 24/07/2020 22:05, Stephen Hemminger wrote:
> > On Fri, 24 Jul 2020 19:24:35 +0300
> > nikolay@cumulusnetworks.com wrote:
> >
> >> On 24 July 2020 19:15:17 EEST, Stephen Hemminger <stephen@networkplumber.org> wrote:
> >>>
> >>> The bridge portion of ip command was not scaling so the
> >>> values were off.
> >>>
> >>> The netlink API's for setting and reading timers all conform
> >>> to the kernel standard of scaling the values by USER_HZ (100).
> >>>
> >>> Fixes: 28d84b429e4e ("add bridge master device support")
> >>> Fixes: 7f3d55922645 ("iplink: bridge: add support for
> >>> IFLA_BR_MCAST_MEMBERSHIP_INTVL")
> >>> Fixes: 10082a253fb2 ("iplink: bridge: add support for
> >>> IFLA_BR_MCAST_LAST_MEMBER_INTVL")
> >>> Fixes: 1f2244b851dd ("iplink: bridge: add support for
> >>> IFLA_BR_MCAST_QUERIER_INTVL")
> >>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >>> ---
> >>
> >> While I agree this should have been done from the start, it's too late to change.
> >> We'll break everyone using these commands.
> >> We have been discussing to add _ms version of all these which do the proper scaling. I'd prefer that, it's least disruptive
> >> to users.
> >>
> >> Every user of the old commands scales the values by now.
> >
> > So bridge is inconsistent with all other api's in iproute2!
> > And the bridge option in ip link is scaled differently than the bridge-utils or sysfs.
> >
>
> Yeah, that is not new, it's been like that for years.
>
I remember having reported this quite a while ago:
https://www.spinics.net/lists/netdev/msg567332.html
I got no response, sadly.
The real problem is that the documentation has been wrong for all this
time (that needs to be updated as a separate action). Yet there were
only a few voices here and there to signal that.
So there are 2 options:
- There have been many users who all hid their head in the sand while
mentally multiplying by USER_HZ.
- Almost nobody was using it since it didn't work.
Occam's razor tells me to go with option #2. So fixing it is never too
late.
> > Maybe an environment variable?
> > Or add new fixed syntax option and don't show the old syntax?
> >
>
> Anything that doesn't disrupt normal processing sounds good.
> So it must be opt-in, we can't just change the default overnight.
>
> The _ms version of all values is that - new fixed syntax options for all current options
> and anyone who wants to use a "normal" time-based option would use those as they'll be
> automatically scaled.
>
Right, the problem, of course, is that you'll end up typing a command
and you'll never know what you're gonna get. Old iproute2, old behavior,
new iproute2, new behavior. So you'd need to look up the iproute2 git
history to figure out, or put prints in the kernel. Unpleasant. At
least, with new strings for ageing_time_ms and such, at least you're
going to know based on whether the program is parsing them.
I vote for new options too.
Thanks,
-Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFT iproute2] iplink_bridge: scale all time values by USER_HZ
2020-07-24 16:24 ` nikolay
2020-07-24 19:05 ` Stephen Hemminger
@ 2020-07-25 0:31 ` David Miller
2020-07-26 3:17 ` Stephen Hemminger
1 sibling, 1 reply; 14+ messages in thread
From: David Miller @ 2020-07-25 0:31 UTC (permalink / raw)
To: nikolay; +Cc: stephen, amarao, netdev, jiri
From: nikolay@cumulusnetworks.com
Date: Fri, 24 Jul 2020 19:24:35 +0300
> While I agree this should have been done from the start, it's too late to change.
Agreed.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFT iproute2] iplink_bridge: scale all time values by USER_HZ
2020-07-25 0:31 ` David Miller
@ 2020-07-26 3:17 ` Stephen Hemminger
2020-07-26 10:43 ` [RFC iproute2] ip: bridge: use -human to convert time-related values to seconds Nikolay Aleksandrov
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Hemminger @ 2020-07-26 3:17 UTC (permalink / raw)
To: David Miller; +Cc: nikolay, amarao, netdev, jiri
On Fri, 24 Jul 2020 17:31:12 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: nikolay@cumulusnetworks.com
> Date: Fri, 24 Jul 2020 19:24:35 +0300
>
> > While I agree this should have been done from the start, it's too late to change.
>
> Agreed.
Please fix the man page, the usage and add a warning.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC iproute2] ip: bridge: use -human to convert time-related values to seconds
2020-07-26 3:17 ` Stephen Hemminger
@ 2020-07-26 10:43 ` Nikolay Aleksandrov
2020-07-26 16:21 ` Vladimir Oltean
0 siblings, 1 reply; 14+ messages in thread
From: Nikolay Aleksandrov @ 2020-07-26 10:43 UTC (permalink / raw)
To: netdev; +Cc: stephen, davem, roopa, amarao, olteanv, jiri, Nikolay Aleksandrov
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@cumulusnetworks.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 ?
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);
- 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC iproute2] ip: bridge: use -human to convert time-related values to seconds
2020-07-26 10:43 ` [RFC iproute2] ip: bridge: use -human to convert time-related values to seconds Nikolay Aleksandrov
@ 2020-07-26 16:21 ` Vladimir Oltean
2020-07-26 16:34 ` Nikolay Aleksandrov
0 siblings, 1 reply; 14+ messages in thread
From: Vladimir Oltean @ 2020-07-26 16:21 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, stephen, davem, roopa, amarao, jiri
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@cumulusnetworks.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"
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.
> 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.
>
> - 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC iproute2] ip: bridge: use -human to convert time-related values to seconds
2020-07-26 16:21 ` Vladimir Oltean
@ 2020-07-26 16:34 ` Nikolay Aleksandrov
2020-07-26 16:50 ` Vladimir Oltean
0 siblings, 1 reply; 14+ messages in thread
From: Nikolay Aleksandrov @ 2020-07-26 16:34 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: netdev, stephen, davem, roopa, amarao, jiri
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@cumulusnetworks.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.
> 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.
>> 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.
>>
>> - 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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC iproute2] ip: bridge: use -human to convert time-related values to seconds
2020-07-26 16:34 ` Nikolay Aleksandrov
@ 2020-07-26 16:50 ` Vladimir Oltean
2020-07-27 16:36 ` Stephen Hemminger
0 siblings, 1 reply; 14+ messages in thread
From: Vladimir Oltean @ 2020-07-26 16:50 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, stephen, davem, roopa, amarao, jiri
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@cumulusnetworks.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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC iproute2] ip: bridge: use -human to convert time-related values to seconds
2020-07-26 16:50 ` Vladimir Oltean
@ 2020-07-27 16:36 ` Stephen Hemminger
0 siblings, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2020-07-27 16:36 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: Nikolay Aleksandrov, netdev, davem, roopa, amarao, jiri
On Sun, 26 Jul 2020 19:50:33 +0300
Vladimir Oltean <olteanv@gmail.com> wrote:
> 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@cumulusnetworks.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
This set of idea looks promising.
A couple of suggestions.
1. Don't use the variable hz, it is confusing relative to the rest of the code
base where hz is always the value from get_user_hz(). Some other name would be better.
2. It might be helpful go allow floating point in the command syntax since the value is scaled.
This would allow for smaller values when still scaled.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-07-27 16:37 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 12:45 Bug in iproute2 man page (or in iproute itself) George Shuklin
2020-07-24 15:35 ` Stephen Hemminger
2020-07-24 16:15 ` [RFT iproute2] iplink_bridge: scale all time values by USER_HZ Stephen Hemminger
2020-07-24 16:24 ` nikolay
2020-07-24 19:05 ` Stephen Hemminger
2020-07-24 22:51 ` Nikolay Aleksandrov
2020-07-24 23:18 ` Vladimir Oltean
2020-07-25 0:31 ` David Miller
2020-07-26 3:17 ` Stephen Hemminger
2020-07-26 10:43 ` [RFC iproute2] ip: bridge: use -human to convert time-related values to seconds Nikolay Aleksandrov
2020-07-26 16:21 ` Vladimir Oltean
2020-07-26 16:34 ` Nikolay Aleksandrov
2020-07-26 16:50 ` Vladimir Oltean
2020-07-27 16:36 ` Stephen Hemminger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.