On Mon, Jul 15, 2013 at 12:43 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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.
>
Looks fine.
>
> > + 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.
>
I see that now.
Looks good to me.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
