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

[Liu Yuan]

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 <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.

I think it would be better to put them together or it is very hard to maintain
the work.c later. Another question is if we really need this patch, use the 
complexity to trade some of extra overhead of code?

Thanks
Yuan