[ovs-dev] [PATCH] bridge: Rate-limit updates to "instant stats".
Ben Pfaff
blp at nicira.com
Wed Mar 20 18:34:47 UTC 2013
Applied to branch-1.10.
On Tue, Mar 19, 2013 at 04:32:32PM -0700, Ben Pfaff wrote:
> Thanks, applied to master, I'll apply to branch-1.10 tomorrow if no
> problems show up right away.
>
> On Tue, Mar 19, 2013 at 04:23:27PM -0700, Ethan Jackson wrote:
> > Actually you're right, I had confused myself. Patch is good as is.
> >
> > Acked-by: Ethan Jackson <ethan at nicira.com>
> >
> >
> >
> > On Tue, Mar 19, 2013 at 2:46 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > > 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