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

[Liu Yuan]

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.

Thanks
Yuan