Re: [RFT iproute2] iplink_bridge: scale all time values by USER_HZ

[Vladimir Oltean <olteanv@gmail.com>]

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