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 -- sheepdog mailing list sheepdog@lists.wpkg.org http://lists.wpkg.org/mailman/listinfo/sheepdog