[ovs-dev] [PATCH] bridge: Rate-limit updates to "instant stats".
Ben Pfaff
blp at nicira.com
Tue Mar 19 21:46:46 UTC 2013
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 <blp at nicira.com> 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 <rjothikumar at nicira.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > 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
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
More information about the dev
mailing list