[ovs-dev] [threads v2 07/13] system-stats: Move into separate thread.

Ben Pfaff blp at nicira.com
Mon Jul 15 19:43:09 UTC 2013


On Fri, Jul 12, 2013 at 04:46:33PM -0700, Gurucharan Shetty wrote:
> On Fri, Jul 12, 2013 at 2:54 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> >  vswitchd/system-stats.c |  105
> > +++++++++++++++++++++++++++++++++++++---------
> >  1 files changed, 84 insertions(+), 21 deletions(-)
> >
> > diff --git a/vswitchd/system-stats.c b/vswitchd/system-stats.c
> > index e7c1d73..ed63899 100644
> > --- a/vswitchd/system-stats.c
> > +++ b/vswitchd/system-stats.c
> > @@ -35,7 +35,9 @@
> >  #include "dirs.h"
> >  #include "dynamic-string.h"
> >  #include "json.h"
> > +#include "latch.h"
> >  #include "ofpbuf.h"
> > +#include "ovs-thread.h"
> >  #include "poll-loop.h"
> >  #include "shash.h"
> >  #include "smap.h"
> > @@ -504,17 +506,32 @@ get_filesys_stats(struct smap *stats OVS_UNUSED)
> >
> >  #define SYSTEM_STATS_INTERVAL (5 * 1000) /* In milliseconds. */
> >
> > -/* The next time to wake up, or LLONG_MAX if stats are disabled. */
> > -static long long int next_refresh = LLONG_MAX;
> > +static pthread_mutex_t mutex = PTHREAD_ADAPTIVE_MUTEX_INITIALIZER;
> > +static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
> > +static struct latch latch;
> > +static bool enabled;
> > +static bool started;
> > +static struct smap *system_stats;
> > +
> > +static void *system_stats_thread_func(void *);
> > +static void discard_stats(void);
> >
> >  /* Enables or disables system stats collection, according to 'enable'. */
> >  void
> >  system_stats_enable(bool enable)
> >  {
> > -    if (!enable) {
> > -        next_refresh = LLONG_MAX;
> > -    } else if (next_refresh == LLONG_MAX) {
> > -        next_refresh = time_msec();
> > +    if (enabled != enable) {
> > +        xpthread_mutex_lock(&mutex);
> > +        if (enable) {
> > +            if (!started) {
> 
> I do not see "started" getting changed anywhere else.

Oops.  It wasn't changed anywhere, in fact.

I added 'started = true;' here.

> +                xpthread_create(NULL, NULL, system_stats_thread_func,
> > NULL);
> > +                latch_init(&latch);
> > +            }
> > +            discard_stats();
> > +            xpthread_cond_signal(&cond);
> > +        }
> > +        enabled = enable;
> > +        xpthread_mutex_unlock(&mutex);
> >      }
> >  }
> >
> > @@ -529,23 +546,22 @@ system_stats_enable(bool enable)
> >  struct smap *
> >  system_stats_run(void)
> >  {
> > -    if (time_msec() >= next_refresh) {
> > -        struct smap *stats;
> > +    struct smap *stats = NULL;
> >
> > -        stats = xmalloc(sizeof *stats);
> > -        smap_init(stats);
> > -        get_cpu_cores(stats);
> > -        get_load_average(stats);
> > -        get_memory_stats(stats);
> > -        get_process_stats(stats);
> > -        get_filesys_stats(stats);
> > -
> > -        next_refresh = time_msec() + SYSTEM_STATS_INTERVAL;
> > +    xpthread_mutex_lock(&mutex);
> > +    if (system_stats) {
> > +        latch_poll(&latch);
> >
> > -        return stats;
> > +        if (enabled) {
> > +            stats = system_stats;
> > +            system_stats = NULL;
> > +        } else {
> > +            discard_stats();
> > +        }
> >      }
> > +    xpthread_mutex_unlock(&mutex);
> >
> > -    return NULL;
> > +    return stats;
> >  }
> >
> >  /* Causes poll_block() to wake up when system_stats_run() needs to be
> > @@ -553,7 +569,54 @@ system_stats_run(void)
> >  void
> >  system_stats_wait(void)
> >  {
> > -    if (next_refresh != LLONG_MAX) {
> > -        poll_timer_wait_until(next_refresh);
> > +    if (enabled) {
> > +        latch_wait(&latch);
> > +    }
> > +}
> > +
> > +static void
> > +discard_stats(void)
> > +{
> > +    if (system_stats) {
> > +        smap_destroy(system_stats);
> > +        free(system_stats);
> > +        system_stats = NULL;
> > +    }
> > +}
> > +
> > +static void * NO_RETURN
> > +system_stats_thread_func(void *arg OVS_UNUSED)
> > +{
> > +    pthread_detach(pthread_self());
> > +
> > +    for (;;) {
> > +        long long int next_refresh;
> > +        struct smap *stats;
> > +
> > +        xpthread_mutex_lock(&mutex);
> > +        while (!enabled) {
> > +            xpthread_cond_wait(&cond, &mutex);
> > +        }
> > +        xpthread_mutex_unlock(&mutex);
> > +
> > +        stats = xmalloc(sizeof *stats);
> > +        smap_init(stats);
> > +        get_cpu_cores(stats);
> > +        get_load_average(stats);
> > +        get_memory_stats(stats);
> > +        get_process_stats(stats);
> > +        get_filesys_stats(stats);
> > +
> > +        xpthread_mutex_lock(&mutex);
> > +        discard_stats();
> > +        system_stats = stats;
> > +        latch_set(&latch);
> > +        xpthread_mutex_unlock(&mutex);
> > +
> > +        next_refresh = time_msec() + SYSTEM_STATS_INTERVAL;
> > +        do {
> > +            poll_timer_wait_until(next_refresh);
> >
> The  poll_timer_wait_until function sets the value of "loop->timeout_when".
> The way I see it, it is not protected by any locks and can be overwritten
> by multiple threads at the same time.

There is a thread-specific instance of struct poll_loop.  See
poll_loop() at the end of poll-loop.c.



More information about the dev mailing list