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

[Hitoshi Mitake]

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 <mitake.hitoshi at lab.ntt.co.jp>
> > ---
> >  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