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

[Hitoshi Mitake]

At Wed, 18 Dec 2013 15:11:18 +0800,
Liu Yuan wrote:
> 
> 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.
> 
> I don't understand your argument. wq_need_shrink and wq_need_grow will be called
> in both cases and this is actually IRRELEVANT to the critial section.
> 
> Tow locks doesn't reduce the critical sections, no? With this assumption, we can
> go next question, more locks are btter for the same size critical section? No, if
> this is true, we can add a lock per line to the critical sections to improve
> the performance.

As you say, my patch fails to remove the large critical seciton. But it seems
that current work queue has performance problems related to multi threading.
I'll post the next version with quantitative evaluation result in the near
future. But it will take time.

BTW, please don't say "looks good to me" easily. Overmuch round trip of patch
submission is very time consuming.

Thanks,
Hitoshi