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

[Hitoshi Mitake]

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

OK, I agree with it.
Reviewed-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>

Thanks,
Hitoshi