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

Gurucharan Shetty shettyg at nicira.com
Fri Jul 12 23:46:33 UTC 2013


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.

>

+                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.

> +            poll_block();
> +        } while (time_msec() < next_refresh);
>      }
>  }
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130712/bf502146/attachment-0003.html>


More information about the dev mailing list