[sheepdog] [PATCH v4 1/3] work: protect nr_threads by wi->workers_lock

[Hitoshi Mitake]

At Wed, 18 Dec 2013 14:57:11 +0800,
Liu Yuan wrote:
> 
> On Wed, Dec 18, 2013 at 03:43:50PM +0900, Hitoshi Mitake wrote:
> > At Wed, 18 Dec 2013 14:39:19 +0800,
> > Liu Yuan wrote:
> > > 
> > > On Wed, Dec 18, 2013 at 03:04:21PM +0900, Hitoshi Mitake wrote:
> > > > Previous protection scheme of wi->nr_thread in work.c was
> > > > unclear because wi->startup_lock was also used for protecting it
> > > > during workqueue grow/shrink. This patch let work.c protect
> > > > wi->nr_thread by the new wi->workers_lock.
> > > 
> > > how about merge ->workers_lock and ->pending_lock into a single lock? It looks
> > > neater.
> > 
> > I don't agree with it. Merging the locks will enlarge the critical section and
> > it will harm performance (the problem current work queue mechanism
> > has).
> 
> one lock 'enlarge the critical section' is invalid. I am not convinced for two
> locks works better than one, e.g,
> 
> 	if (wq_need_grow(wi)) <-- grab and release workers_lock
> 		/* double the thread pool size */
> 		create_worker_threads(wi, wi->nr_threads * 2); <-- grab and release workers_lock again
> 
> 	# then grab and release pending_lock
> 	pthread_mutex_lock(&wi->pending_lock); 
> 	list_add_tail(&work->w_list, &wi->q.pending_list);
> 	pthread_mutex_unlock(&wi->pending_lock);
> 
> So queue_work will spend lot of time functions call compete for grab/release locks.
> so queue_work() can't benefit these two locks at all because it equals to following
> case:
> 
> 	pthread_mutex_lock(&wi->pending_lock);
> 	if (wq_need_grow(wi))
> 		/* double the thread pool size */
> 		create_worker_threads(wi, wi->nr_threads * 2);
> 
> 	list_add_tail(&work->w_list, &wi->q.pending_list);
> 	pthread_mutex_unlock(&wi->pending_lock);
> 
> Another spot is
> 
> 		pthread_mutex_lock(&wi->pending_lock);
> 		if (wq_need_shrink(wi)) {
> 			pthread_mutex_unlock(&wi->pending_lock);
> 
> 			pthread_mutex_lock(&wi->workers_lock);
> 			wi->nr_threads--;
> 			pthread_mutex_unlock(&wi->workers_lock);
> 
> which equals to
> 
> 		pthread_mutex_lock(&wi->pending_lock);
> 		if (wq_need_shrink(wi)) {
> 			wi->nr_threads--;
> 			pthread_mutex_unlock(&wi->pending_lock);
> 
> 
> I think in above examples, two locks works worse than a single lock, both has
> the same critical sections, no more no less and your approach introduces extra
> more calls on lock/unlock diferent locks.

grow/shrink of work queue are not frequent events. If they happen frequently, it
means the design of the work queue is broken. Frequent call of pthread_create()
is clearly harmful for performance. So my approach doesn't produce more
lock/release than the previous code.

Thanks,
Hitoshi