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

[Liu Yuan]

On Wed, Dec 18, 2013 at 04:03:17PM +0900, Hitoshi Mitake wrote:
> 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.

You ineed add at least two extra calls, one is pthread_mutex_unlock(&wi->pending_lock);
another is pthread_mutex_lock(&wi->workers_lock).

Thanks
Yuan