On Mon, Jun 14, 2021 at 09:08:58PM +0200, Ilya Maximets wrote: > On 6/11/21 12:49 AM, Ben Pfaff wrote: > > The documentation for inactivity_probe says this: > > > > inactivity_probe: optional integer > > Maximum number of milliseconds of idle time on connec‐ > > tion to controller before sending an inactivity probe > > message. If Open vSwitch does not communicate with the > > controller for the specified number of seconds, it will > > send a probe. If a response is not received for the same > > additional amount of time, Open vSwitch assumes the con‐ > > nection has been broken and attempts to reconnect. De‐ > > fault is implementation-specific. A value of 0 disables > > inactivity probes. > > > > This means that a value of 0 should disable inactivity probes and any > > other value should be in milliseconds. The code in bridge.c was > > actually interpreting it as any value between 0 and 999 disabling > > inactivity probes. That was surprising when I accidentally configured > > it to 5 or to 10, not remembering that it was in milliseconds, and > > disabled them entirely. This fixes the problem. > > > > Signed-off-by: Ben Pfaff <[email protected]> > > --- > > vswitchd/bridge.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > > index 5ed7e8234354..9c1ee3cf06fe 100644 > > --- a/vswitchd/bridge.c > > +++ b/vswitchd/bridge.c > > @@ -3965,8 +3965,10 @@ bridge_configure_remotes(struct bridge *br, > > *oc = (struct ofproto_controller) { > > .type = get_controller_ofconn_type(c->target, c->type), > > .max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8, > > - .probe_interval = (c->inactivity_probe > > - ? *c->inactivity_probe / 1000 : 5), > > + .probe_interval = (!c->inactivity_probe ? 5 > > + : !*c->inactivity_probe ? 0 > > + : *c->inactivity_probe < 1000 ? 1 > > + : *c->inactivity_probe / 1000), > > .band = ((!c->connection_mode > > || !strcmp(c->connection_mode, "in-band")) > > && !disable_in_band > > > > This change looks fine in terms that it fixes disabling of the > inactivity probe, so: > > Acked-by: Ilya Maximets <[email protected]>
Thanks, applied. > OTOH, it's unclear why the DB schema allows configuration in > milliseconds if the implementation doesn't honor the configured > value rounding it up or down. It's because I figured we'd eventually make the code support millisecond resolution but somehow it never mattered enough to implement it. Early on, OVS's notion of timer was only 1-second resolution because it used to be fairly expensive to get the current time, so there was a complicated thing going on with timer signals that caused OVS to retrieve the current time again. This was a mess, so when Linux put gettimeofday into the VDSO, making it cheap, we dropped all the nastiness and just switched to direct calls, which in turn made millisecond (or higher!) resolution cheap. But I never went back and changed all the calls to time_now() to use time_msec() instead. Perhaps that's a project that's still worth doing. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
