At Tue, 22 Oct 2013 15:42:02 +0800, Liu Yuan wrote: > > On Tue, Oct 22, 2013 at 04:13:11PM +0900, Hitoshi Mitake wrote: > > 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. > > > > Signed-off-by: Hitoshi Mitake <[email protected]> > > --- > > include/work.h | 7 +++++-- > > lib/Makefile.am | 4 ++++ > > lib/work.c | 50 > > ++++++++++++++++++++++++++++++++++++++------------ > > sheep/Makefile.am | 1 + > > sheep/trace/trace.c | 1 + > > 5 files changed, 49 insertions(+), 14 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 b6ac290..bab32b9 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 25cf964..d8e154e 100644 > > --- a/lib/work.c > > +++ b/lib/work.c > > @@ -33,6 +33,8 @@ > > #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; > > @@ -40,6 +42,8 @@ static unsigned long *tid_map; > > 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 +159,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 +203,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,6 +270,8 @@ static void *worker_routine(void *arg) > > /* started this thread */ > > pthread_mutex_unlock(&wi->startup_lock); > > > > +#ifdef ENABLE_TRACE > > + > > pthread_mutex_lock(&wi->pending_lock); > > if (tid > tid_max) { > > size_t old_tid_max = tid_max; > > @@ -265,12 +285,18 @@ static void *worker_routine(void *arg) > > set_bit(tid, tid_map); > > pthread_mutex_unlock(&wi->pending_lock); > > > > +#endif /* ENABLE_TRACE */ > > + > > while (true) { > > > > pthread_mutex_lock(&wi->pending_lock); > > if (wq_need_shrink(wi)) { > > wi->nr_threads--; > > + > > +#ifdef ENABLE_TRACE > > clear_bit(tid, tid_map); > > +#endif > > Instead of putting ENABLE_TRACE macros everywhere in the source, I'd suggest > in > them together in following scheme: > > #ifdef ENABLE_TRACE > functions() > { > do something; > } > #else > functions() # whichi will be removed out by compiler automatically > { > do nothing; > } >
If these functions are used by many part of other code, I agree with your style. But in this case the above style adds needless complexities because the excluded code is used by sheep/trace/trace.c. Thanks, Hitoshi -- sheepdog mailing list [email protected] http://lists.wpkg.org/mailman/listinfo/sheepdog
