At Tue, 22 Oct 2013 16:03:40 +0800, Liu Yuan wrote: > > On Tue, Oct 22, 2013 at 04:51:29PM +0900, Hitoshi Mitake wrote: > > 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. > > I think it would be better to put them together or it is very hard to maintain > the work.c later.
Of course I agree with putting them in work.c togather. But the tracing stuff should be excluded when a sheep binary doesn't support tracing. They will be dead code on most production deployments. > Another question is if we really need this patch, use the > complexity to trade some of extra overhead of code? > Yes, allocating never used tid_map can be avoided. I think these ifdefs are not expensive. Thanks, Hitoshi -- sheepdog mailing list [email protected] http://lists.wpkg.org/mailman/listinfo/sheepdog
