On Wed, Dec 18, 2013 at 02:49:35PM +0900, Hitoshi Mitake wrote: > At Wed, 18 Dec 2013 13:35:21 +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 <[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]> > > OK, I agree with it. > Reviewed-by: Hitoshi Mitake <[email protected]>
Thanks, I'll apply this patch. Then could you rebase your patch set on top of it if needed? Others look good to me. Thanks Yuan -- sheepdog mailing list [email protected] http://lists.wpkg.org/mailman/listinfo/sheepdog
