Actually you're right, I had confused myself. Patch is good as is.
Acked-by: Ethan Jackson <et...@nicira.com>
On Tue, Mar 19, 2013 at 2:46 PM, Ben Pfaff <b...@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 <b...@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 <rjothiku...@nicira.com>
> > > Signed-off-by: Ben Pfaff <b...@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@openvswitch.org
> > > http://openvswitch.org/mailman/listinfo/dev
> > >
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
