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