On Fri, Jul 12, 2013 at 04:46:33PM -0700, Gurucharan Shetty wrote:
> On Fri, Jul 12, 2013 at 2:54 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > Signed-off-by: Ben Pfaff <b...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
