Not sure I follow. The goal of instant_stats_could_have_changed is to track whether the source data that we want to feed into the database could have changed since we last composed a transaction to write it. Whether the database itself has changed is not an issue.
Can you expand on your proposal? On Tue, Mar 19, 2013 at 02:43:21PM -0700, Ethan Jackson wrote: > The instant_stats_could_have_changed variable seems a bit of a kludge to > me. It seems it would be cleaner if instant_stats_run() maintained the > idl_seqno at the time of the 'instant_txn' creation. If that sequence > number is out of date, we know we need to wait until instant_next_txn. > Thoughts? > > Ethan > > > On Tue, Mar 19, 2013 at 2:04 PM, Ben Pfaff <[email protected]> wrote: > > > Some information in the database must be kept as up-to-date as > > possible to allow controllers to respond rapidly to network outages. > > We call these statistics "instant" stats. > > > > Until now, the instant stats have been updated on every trip through > > the main loop. This work scales with the number of interfaces that > > ovs-vswitchd manages. With CFM enabled on 5000 interfaces, even with > > a low transmission rate, we see ovs-vswitchd using 100% CPU just to > > maintain statistics, even with no actual changes. > > > > This commit rate-limits updates to instant stats to at most 10 times > > per second. Earlier tests I did with similar patches showed a major > > reduction in CPU usage. I have not rerun those tests with this patch, > > but I expect that the CPU usage should similarly decline. > > > > CC: Ram Jothikumar <[email protected]> > > Signed-off-by: Ben Pfaff <[email protected]> > > --- > > vswitchd/bridge.c | 75 > > +++++++++++++++++++++++++++++++++++++++++++++++----- > > 1 files changed, 67 insertions(+), 8 deletions(-) > > > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > > index 9449879..6dc3db2 100644 > > --- a/vswitchd/bridge.c > > +++ b/vswitchd/bridge.c > > @@ -2019,17 +2019,61 @@ refresh_controller_status(void) > > > > ofproto_free_ofproto_controller_info(&info); > > } > > + > > +/* "Instant" stats. > > + * > > + * Some information in the database must be kept as up-to-date as > > possible to > > + * allow controllers to respond rapidly to network outages. We call these > > + * statistics "instant" stats. > > + * > > + * We wish to update these statistics every INSTANT_INTERVAL_MSEC > > milliseconds, > > + * assuming that they've changed. The only means we have to determine > > whether > > + * they have changed are: > > + * > > + * - Try to commit changes to the database. If nothing changed, then > > + * ovsdb_idl_txn_commit() returns TXN_UNCHANGED, otherwise some other > > + * value. > > + * > > + * - instant_stats_run() is called late in the run loop, after anything > > that > > + * might change any of the instant stats. > > + * > > + * We use these two facts together to avoid waking the process up every > > + * INSTANT_INTERVAL_MSEC whether there is any change or not. > > + */ > > + > > +/* Minimum interval between writing updates to the instant stats to the > > + * database. */ > > +#define INSTANT_INTERVAL_MSEC 100 > > + > > +/* Current instant stats database transaction, NULL if there is no ongoing > > + * transaction. */ > > +static struct ovsdb_idl_txn *instant_txn; > > + > > +/* Next time (in msec on monotonic clock) at which we will update the > > instant > > + * stats. */ > > +static long long int instant_next_txn = LLONG_MIN; > > + > > +/* True if the run loop has run since we last saw that the instant stats > > were > > + * unchanged, that is, this is true if we need to wake up at > > 'instant_next_txn' > > + * to refresh the instant stats. */ > > +static bool instant_stats_could_have_changed; > > > > static void > > -refresh_instant_stats(void) > > +instant_stats_run(void) > > { > > - static struct ovsdb_idl_txn *txn = NULL; > > + enum ovsdb_idl_txn_status status; > > > > - if (!txn) { > > + instant_stats_could_have_changed = true; > > + > > + if (!instant_txn) { > > struct bridge *br; > > > > - txn = ovsdb_idl_txn_create(idl); > > + if (time_msec() < instant_next_txn) { > > + return; > > + } > > + instant_next_txn = time_msec() + INSTANT_INTERVAL_MSEC; > > > > + instant_txn = ovsdb_idl_txn_create(idl); > > HMAP_FOR_EACH (br, node, &all_bridges) { > > struct iface *iface; > > struct port *port; > > @@ -2078,12 +2122,26 @@ refresh_instant_stats(void) > > } > > } > > > > - if (ovsdb_idl_txn_commit(txn) != TXN_INCOMPLETE) { > > - ovsdb_idl_txn_destroy(txn); > > - txn = NULL; > > + status = ovsdb_idl_txn_commit(instant_txn); > > + if (status != TXN_INCOMPLETE) { > > + ovsdb_idl_txn_destroy(instant_txn); > > + instant_txn = NULL; > > + } > > + if (status == TXN_UNCHANGED) { > > + instant_stats_could_have_changed = false; > > } > > } > > > > +static void > > +instant_stats_wait(void) > > +{ > > + if (instant_txn) { > > + ovsdb_idl_txn_wait(instant_txn); > > + } else if (instant_stats_could_have_changed) { > > + poll_timer_wait_until(instant_next_txn); > > + } > > +} > > + > > /* Performs periodic activity required by bridges that needs to be done > > with > > * the least possible latency. > > * > > @@ -2254,7 +2312,7 @@ bridge_run(void) > > } > > > > run_system_stats(); > > - refresh_instant_stats(); > > + instant_stats_run(); > > } > > > > void > > @@ -2286,6 +2344,7 @@ bridge_wait(void) > > } > > > > system_stats_wait(); > > + instant_stats_wait(); > > } > > > > /* Adds some memory usage statistics for bridges into 'usage', for use > > with > > -- > > 1.7.2.5 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > http://openvswitch.org/mailman/listinfo/dev > > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
