[sheepdog] [PATCH v2 1/4] lib, sheep: exclude stuff for tracing when it is not enabled

Liu Yuan namei.unix at gmail.com
Wed Dec 18 06:49:16 CET 2013


On Wed, Dec 18, 2013 at 01:35:21PM +0800, Liu Yuan wrote:
> On Wed, Dec 18, 2013 at 01:13:15PM +0900, Hitoshi Mitake wrote:
> > At Wed, 18 Dec 2013 11:36:41 +0800,
> > Liu Yuan wrote:
> > > 
> > > On Tue, Dec 17, 2013 at 11:42:53PM +0900, Hitoshi Mitake wrote:
> > > > From: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > > > 
> > > > The current build process of sheepdog compiles stuff for tracing even
> > > > if tracing is disabled. Basically they are not harmful but causes
> > > > memory consumption (tid_map), we should exlucde them when tracing is
> > > > disabled.
> > > > 
> > > > In addition, this patch adds a new mutex tid_map_lock for protecting
> > > > tid_map. Previous work.c used wi->pending_lock also for protecting the
> > > > bitmap. This protection scheme is very confusing.
> > > > 
> > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > > > ---
> > > >  include/work.h      |  7 +++++--
> > > >  lib/Makefile.am     |  4 ++++
> > > >  lib/work.c          | 58 ++++++++++++++++++++++++++++++++++++++++-------------
> > > >  sheep/Makefile.am   |  1 +
> > > >  sheep/trace/trace.c |  1 +
> > > >  5 files changed, 55 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/include/work.h b/include/work.h
> > > > index a5808b5..0cb3313 100644
> > > > --- a/include/work.h
> > > > +++ b/include/work.h
> > > > @@ -61,9 +61,12 @@ static inline bool is_worker_thread(void)
> > > >  int init_work_queue(size_t (*get_nr_nodes)(void));
> > > >  struct work_queue *create_work_queue(const char *name, enum wq_thread_control);
> > > >  struct work_queue *create_ordered_work_queue(const char *name);
> > > > -void suspend_worker_threads(void);
> > > > -void resume_worker_threads(void);
> > > >  void queue_work(struct work_queue *q, struct work *work);
> > > >  bool work_queue_empty(struct work_queue *q);
> > > >  
> > > > +#ifdef ENABLE_TRACE
> > > > +void suspend_worker_threads(void);
> > > > +void resume_worker_threads(void);
> > > > +#endif	/* BUILD_TRACE */
> > > > +
> > > >  #endif
> > > > diff --git a/lib/Makefile.am b/lib/Makefile.am
> > > > index a681167..5204879 100644
> > > > --- a/lib/Makefile.am
> > > > +++ b/lib/Makefile.am
> > > > @@ -11,6 +11,10 @@ if BUILD_SHA1_HW
> > > >  libsheepdog_a_SOURCES	+= sha1_ssse3.S
> > > >  endif
> > > >  
> > > > +if BUILD_TRACE
> > > > +AM_CPPFLAGS		+= -DENABLE_TRACE
> > > > +endif
> > > > +
> > > >  # support for GNU Flymake
> > > >  check-syntax:
> > > >  	$(COMPILE) -fsyntax-only $(CHK_SOURCES)
> > > > diff --git a/lib/work.c b/lib/work.c
> > > > index 6933e1a..84eb727 100644
> > > > --- a/lib/work.c
> > > > +++ b/lib/work.c
> > > > @@ -33,13 +33,19 @@
> > > >  #include "work.h"
> > > >  #include "event.h"
> > > >  
> > > > +#ifdef ENABLE_TRACE
> > > > +
> > > >  #define TID_MAX_DEFAULT 0x8000 /* default maximum tid for most systems */
> > > >  
> > > >  static size_t tid_max;
> > > >  static unsigned long *tid_map;
> > > > +static pthread_mutex_t tid_map_lock = PTHREAD_MUTEX_INITIALIZER;
> > > > +
> > > >  static int resume_efd;
> > > >  static int ack_efd;
> > > >  
> > > > +#endif	/* ENABLE_TRACE */
> > > > +
> > > >  /*
> > > >   * The protection period from shrinking work queue.  This is necessary
> > > >   * to avoid many calls of pthread_create.  Without it, threads are
> > > > @@ -155,6 +161,8 @@ static int create_worker_threads(struct worker_info *wi, size_t nr_threads)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +#ifdef ENABLE_TRACE
> > > > +
> > > >  void suspend_worker_threads(void)
> > > >  {
> > > >  	struct worker_info *wi;
> > > > @@ -197,6 +205,18 @@ void resume_worker_threads(void)
> > > >  	}
> > > >  }
> > > >  
> > > > +static void suspend(int num)
> > > > +{
> > > > +	int uninitialized_var(value);
> > > > +
> > > > +	eventfd_xwrite(ack_efd, 1); /* ack of suspend */
> > > > +	value = eventfd_xread(resume_efd);
> > > > +	assert(value == 1);
> > > > +	eventfd_xwrite(ack_efd, 1); /* ack of resume */
> > > > +}
> > > > +
> > > > +#endif	/* ENABLE_TRACE */
> > > > +
> > > >  void queue_work(struct work_queue *q, struct work *work)
> > > >  {
> > > >  	struct worker_info *wi = container_of(q, struct worker_info, q);
> > > > @@ -252,7 +272,9 @@ static void *worker_routine(void *arg)
> > > >  	/* started this thread */
> > > >  	pthread_mutex_unlock(&wi->startup_lock);
> > > >  
> > > > -	pthread_mutex_lock(&wi->pending_lock);
> > > > +#ifdef ENABLE_TRACE
> > > > +
> > > > +	pthread_mutex_lock(&tid_map_lock);
> > > >  	if (tid > tid_max) {
> > > >  		size_t old_tid_max = tid_max;
> > > >  
> > > > @@ -263,14 +285,22 @@ static void *worker_routine(void *arg)
> > > >  		tid_map = alloc_bitmap(tid_map, old_tid_max, tid_max);
> > > >  	}
> > > >  	set_bit(tid, tid_map);
> > > > -	pthread_mutex_unlock(&wi->pending_lock);
> > > > +	pthread_mutex_unlock(&tid_map_lock);
> > > > +
> > > > +#endif	/* ENABLE_TRACE */
> > > 
> > > How about grouping these lines into a function? This really messes up the code
> > > and hard to maintain.
> > > 
> > > Then 
> > > 
> > > #ifdef ENABLE_TRACE
> > > functions() { do something }
> > > #else
> > > functions() { do nothing }
> > > #endif
> > 
> > I don't think the above scheme is suitable for this case. It is hard to call the
> > exlucded lines "functions". Separating these lines will increase maintenance
> > cost than current simple #ifdef scheme.
> 
> How about following patch?
> 
> 
> From: Liu Yuan <namei.unix at gmail.com>
> Date: Wed, 18 Dec 2013 13:31:34 +0800
> Subject: [PATCH] work queue: make trace related code compile aware

I fixed some compile problems and please try this v2:


>From bfe979dcfdba47466320f826cdb586f47116085d Mon Sep 17 00:00:00 2001
From: Liu Yuan <namei.unix at gmail.com>
Date: Wed, 18 Dec 2013 13:48:00 +0800
Subject: [PATCH v2] work queue: make trace related code compile aware

This patches removes trace code if trace is not enabled. Based on Hitoshi Mitake
original patch:

- lib, sheep: exclude stuff for tracing when it is not enabled

Signed-off-by: Liu Yuan <namei.unix at gmail.com>

diff --git a/include/work.h b/include/work.h
index a5808b5..0cb3313 100644
--- a/include/work.h
+++ b/include/work.h
@@ -61,9 +61,12 @@ static inline bool is_worker_thread(void)
 int init_work_queue(size_t (*get_nr_nodes)(void));
 struct work_queue *create_work_queue(const char *name, enum wq_thread_control);
 struct work_queue *create_ordered_work_queue(const char *name);
-void suspend_worker_threads(void);
-void resume_worker_threads(void);
 void queue_work(struct work_queue *q, struct work *work);
 bool work_queue_empty(struct work_queue *q);
 
+#ifdef ENABLE_TRACE
+void suspend_worker_threads(void);
+void resume_worker_threads(void);
+#endif	/* BUILD_TRACE */
+
 #endif
diff --git a/lib/Makefile.am b/lib/Makefile.am
index a681167..5204879 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -11,6 +11,10 @@ if BUILD_SHA1_HW
 libsheepdog_a_SOURCES	+= sha1_ssse3.S
 endif
 
+if BUILD_TRACE
+AM_CPPFLAGS		+= -DENABLE_TRACE
+endif
+
 # support for GNU Flymake
 check-syntax:
 	$(COMPILE) -fsyntax-only $(CHK_SOURCES)
diff --git a/lib/work.c b/lib/work.c
index 6933e1a..50b57a5 100644
--- a/lib/work.c
+++ b/lib/work.c
@@ -33,13 +33,6 @@
 #include "work.h"
 #include "event.h"
 
-#define TID_MAX_DEFAULT 0x8000 /* default maximum tid for most systems */
-
-static size_t tid_max;
-static unsigned long *tid_map;
-static int resume_efd;
-static int ack_efd;
-
 /*
  * The protection period from shrinking work queue.  This is necessary
  * to avoid many calls of pthread_create.  Without it, threads are
@@ -79,6 +72,121 @@ static size_t (*wq_get_nr_nodes)(void);
 
 static void *worker_routine(void *arg);
 
+#ifdef ENABLE_TRACE
+
+#define TID_MAX_DEFAULT 0x8000 /* default maximum tid for most systems */
+
+static size_t tid_max;
+static unsigned long *tid_map;
+static pthread_mutex_t tid_map_lock = PTHREAD_MUTEX_INITIALIZER;
+
+static int resume_efd;
+static int ack_efd;
+
+void suspend_worker_threads(void)
+{
+	struct worker_info *wi;
+	int tid;
+
+	list_for_each_entry(wi, &worker_info_list, worker_info_siblings) {
+		pthread_mutex_lock(&wi->pending_lock);
+	}
+
+	FOR_EACH_BIT(tid, tid_map, tid_max) {
+		if (unlikely(tkill(tid, SIGUSR2) < 0))
+			panic("%m");
+	}
+
+	/*
+	 * Wait for all the worker thread to suspend.  We cannot use
+	 * wi->nr_threads here because some thread may have not called set_bit()
+	 * yet (then, the thread doesn't recieve SIGUSR2).
+	 */
+	FOR_EACH_BIT(tid, tid_map, tid_max) {
+		eventfd_xread(ack_efd);
+	}
+}
+
+void resume_worker_threads(void)
+{
+	struct worker_info *wi;
+	int nr_threads = 0, tid;
+
+	FOR_EACH_BIT(tid, tid_map, tid_max) {
+		nr_threads++;
+	}
+
+	eventfd_xwrite(resume_efd, nr_threads);
+	for (int i = 0; i < nr_threads; i++)
+		eventfd_xread(ack_efd);
+
+	list_for_each_entry(wi, &worker_info_list, worker_info_siblings) {
+		pthread_mutex_unlock(&wi->pending_lock);
+	}
+}
+
+static void suspend(int num)
+{
+	int uninitialized_var(value);
+
+	eventfd_xwrite(ack_efd, 1); /* ack of suspend */
+	value = eventfd_xread(resume_efd);
+	assert(value == 1);
+	eventfd_xwrite(ack_efd, 1); /* ack of resume */
+}
+
+static int wq_trace_init(void)
+{
+	tid_max = TID_MAX_DEFAULT;
+	tid_map = alloc_bitmap(NULL, 0, tid_max);
+
+	resume_efd = eventfd(0, EFD_SEMAPHORE);
+	ack_efd = eventfd(0, EFD_SEMAPHORE);
+
+	if (resume_efd < 0 || ack_efd < 0) {
+		sd_err("failed to create event fds: %m");
+		return -1;
+	}
+
+	/* trace uses this signal to suspend the worker threads */
+	if (install_sighandler(SIGUSR2, suspend, false) < 0) {
+		sd_debug("%m");
+		return -1;
+	}
+	return 0;
+}
+
+static void trace_set_tid_map(int tid)
+{
+	pthread_mutex_lock(&tid_map_lock);
+	if (tid > tid_max) {
+		size_t old_tid_max = tid_max;
+
+		/* enlarge bitmap size */
+		while (tid > tid_max)
+			tid_max *= 2;
+
+		tid_map = alloc_bitmap(tid_map, old_tid_max, tid_max);
+	}
+	set_bit(tid, tid_map);
+	pthread_mutex_unlock(&tid_map_lock);
+}
+
+static void trace_clear_tid_map(int tid)
+{
+	pthread_mutex_lock(&tid_map_lock);
+	clear_bit(tid, tid_map);
+	pthread_mutex_unlock(&tid_map_lock);
+}
+
+#else
+
+static inline int wq_trace_init(void) { return 0; }
+static inline void trace_set_tid_map(int tid) {}
+static inline void trace_clear_tid_map(int tid) {}
+
+#endif	/* ENABLE_TRACE */
+
 static uint64_t get_msec_time(void)
 {
 	struct timeval tv;
@@ -155,48 +263,6 @@ static int create_worker_threads(struct worker_info *wi, size_t nr_threads)
 	return 0;
 }
 
-void suspend_worker_threads(void)
-{
-	struct worker_info *wi;
-	int tid;
-
-	list_for_each_entry(wi, &worker_info_list, worker_info_siblings) {
-		pthread_mutex_lock(&wi->pending_lock);
-	}
-
-	FOR_EACH_BIT(tid, tid_map, tid_max) {
-		if (unlikely(tkill(tid, SIGUSR2) < 0))
-			panic("%m");
-	}
-
-	/*
-	 * Wait for all the worker thread to suspend.  We cannot use
-	 * wi->nr_threads here because some thread may have not called set_bit()
-	 * yet (then, the thread doesn't recieve SIGUSR2).
-	 */
-	FOR_EACH_BIT(tid, tid_map, tid_max) {
-		eventfd_xread(ack_efd);
-	}
-}
-
-void resume_worker_threads(void)
-{
-	struct worker_info *wi;
-	int nr_threads = 0, tid;
-
-	FOR_EACH_BIT(tid, tid_map, tid_max) {
-		nr_threads++;
-	}
-
-	eventfd_xwrite(resume_efd, nr_threads);
-	for (int i = 0; i < nr_threads; i++)
-		eventfd_xread(ack_efd);
-
-	list_for_each_entry(wi, &worker_info_list, worker_info_siblings) {
-		pthread_mutex_unlock(&wi->pending_lock);
-	}
-}
-
 void queue_work(struct work_queue *q, struct work *work)
 {
 	struct worker_info *wi = container_of(q, struct worker_info, q);
@@ -252,25 +318,14 @@ static void *worker_routine(void *arg)
 	/* started this thread */
 	pthread_mutex_unlock(&wi->startup_lock);
 
-	pthread_mutex_lock(&wi->pending_lock);
-	if (tid > tid_max) {
-		size_t old_tid_max = tid_max;
-
-		/* enlarge bitmap size */
-		while (tid > tid_max)
-			tid_max *= 2;
-
-		tid_map = alloc_bitmap(tid_map, old_tid_max, tid_max);
-	}
-	set_bit(tid, tid_map);
-	pthread_mutex_unlock(&wi->pending_lock);
-
+	trace_set_tid_map(tid);
 	while (true) {
 
 		pthread_mutex_lock(&wi->pending_lock);
 		if (wq_need_shrink(wi)) {
 			wi->nr_threads--;
-			clear_bit(tid, tid_map);
+
+			trace_clear_tid_map(tid);
 			pthread_mutex_unlock(&wi->pending_lock);
 			pthread_detach(pthread_self());
 			sd_debug("destroy thread %s %d, %zu", wi->name, tid,
@@ -302,16 +357,6 @@ retest:
 	pthread_exit(NULL);
 }
 
-static void suspend(int num)
-{
-	int uninitialized_var(value);
-
-	eventfd_xwrite(ack_efd, 1); /* ack of suspend */
-	value = eventfd_xread(resume_efd);
-	assert(value == 1);
-	eventfd_xwrite(ack_efd, 1); /* ack of resume */
-}
-
 int init_work_queue(size_t (*get_nr_nodes)(void))
 {
 	int ret;
@@ -321,28 +366,21 @@ int init_work_queue(size_t (*get_nr_nodes)(void))
 	if (wq_get_nr_nodes)
 		nr_nodes = wq_get_nr_nodes();
 
-	tid_max = TID_MAX_DEFAULT;
-	tid_map = alloc_bitmap(NULL, 0, tid_max);
-
-	resume_efd = eventfd(0, EFD_SEMAPHORE);
-	ack_efd = eventfd(0, EFD_SEMAPHORE);
 	efd = eventfd(0, EFD_NONBLOCK);
-	if (resume_efd < 0 || ack_efd < 0 || efd < 0) {
-		sd_err("failed to create event fds: %m");
-		return 1;
-	}
-
-	/* trace uses this signal to suspend the worker threads */
-	if (install_sighandler(SIGUSR2, suspend, false) < 0) {
-		sd_debug("%m");
+	if (efd < 0) {
+		sd_err("failed to create event fd: %m");
 		return -1;
 	}
 
+	ret = wq_trace_init();
+	if (ret < 0)
+		return ret;
+
 	ret = register_event(efd, worker_thread_request_done, NULL);
 	if (ret) {
 		sd_err("failed to register event fd %m");
 		close(efd);
-		return 1;
+		return -1;
 	}
 
 	return 0;
diff --git a/sheep/Makefile.am b/sheep/Makefile.am
index 3cfec53..5fff697 100644
--- a/sheep/Makefile.am
+++ b/sheep/Makefile.am
@@ -44,6 +44,7 @@ sheep_SOURCES		+= cluster/shepherd.c
 endif
 
 if BUILD_TRACE
+AM_CPPFLAGS		+= -DENABLE_TRACE
 sheep_SOURCES		+= trace/trace.c trace/mcount.S trace/graph.c trace/checker.c
 endif
 
diff --git a/sheep/trace/trace.c b/sheep/trace/trace.c
index f4f11e7..937dc72 100644
--- a/sheep/trace/trace.c
+++ b/sheep/trace/trace.c
@@ -14,6 +14,7 @@
 #include <bfd.h>
 
 #include "trace.h"
+#include "work.h"
 
 /* Intel recommended one for 5 bytes nops (nopl 0x0(%rax,%rax,1)) */
 static const unsigned char NOP5[INSN_SIZE] = {0x0f, 0x1f, 0x44, 0x00, 0x00};



More information about the sheepdog mailing list