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 <[email protected]> > > > > > > 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 <[email protected]> > > > --- > > > 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 <[email protected]> Date: Wed, 18 Dec 2013 13:31:34 +0800 Subject: [PATCH] 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 <[email protected]> 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..16a7692 100644 --- a/lib/work.c +++ b/lib/work.c @@ -33,13 +33,120 @@ #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; +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; + } +} + +static void trace_set_tid_map(void) +{ + 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(void) +{ + 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(void) {} +static inline void trace_clear_tid_map(void) {} + +#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,48 +262,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 +317,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(); 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(); pthread_mutex_unlock(&wi->pending_lock); pthread_detach(pthread_self()); sd_debug("destroy thread %s %d, %zu", wi->name, tid, @@ -302,16 +356,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 +365,22 @@ 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}; -- sheepdog mailing list [email protected] http://lists.wpkg.org/mailman/listinfo/sheepdog
