Date: Wed, 4 Dec 2013 23:37:05 +0200 From: Mikolaj Golub <trociny@FreeBSD.org> To: David Xu <davidxu@freebsd.org> Cc: freebsd-stable@freebsd.org, Pete French <petefrench@ingresso.co.uk> Subject: Re: Hast locking up under 9.2 Message-ID: <20131204213704.GD4005@gmail.com> In-Reply-To: <529D40B5.5040605@freebsd.org> References: <20131121203711.GA3736@gmail.com> <E1Vjokn-000OuU-1Y@dilbert.ingresso.co.uk> <20131123215950.GA17292@gmail.com> <20131125083223.GE1398@garage.freebsd.pl> <20131125094111.GA22396@gmail.com> <529D40B5.5040605@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Dec 03, 2013 at 10:23:49AM +0800, David Xu wrote:
> Also I found the following code in hastd:
>
> #define QUEUE_INSERT1(hio, name, ncomp) do { \
> bool _wakeup; \
> \
> mtx_lock(&hio_##name##_list_lock[(ncomp)]); \
> _wakeup = TAILQ_EMPTY(&hio_##name##_list[(ncomp)]); \
> TAILQ_INSERT_TAIL(&hio_##name##_list[(ncomp)], (hio), \
> hio_next[(ncomp)]); \
> hio_##name##_list_size[(ncomp)]++; \
> mtx_unlock(&hio_##name##_list_lock[ncomp]); \
> if (_wakeup) \
> cv_broadcast(&hio_##name##_list_cond[(ncomp)]);
>
> Our thread library does optimize the condition variable's wait/signal
> lock contention, we had implemented wait queue morphying, so it is not
> needed to unlock mutex first, then call cv_broadcast, such code really
> increases spurious wakeups, and can be worse in some cases:
> for example, before cv_broadcast is called, the producer thread is
> preempted, and a consumer thread removes elements in the queue and
> sleeps again, and the producer thread is scheduled again and it blindly
> calls cv_broadcast, and a consumer thread then find nothing in the
> queue, and sleeps again.
>
> I think following code is enough for our thread library, and works
> better.
>
> #define QUEUE_INSERT1(hio, name, ncomp) do { \
> mtx_lock(&hio_##name##_list_lock[(ncomp)]); \
> if (TAILQ_EMPTY(&hio_##name##_list[(ncomp)])) \
> cv_broadcast(&hio_##name##_list_cond[(ncomp)]); \
> TAILQ_INSERT_TAIL(&hio_##name##_list[(ncomp)], (hio), \
> hio_next[(ncomp)]); \
> hio_##name##_list_size[(ncomp)]++; \
> mtx_unlock(&hio_##name##_list_lock[ncomp]); \
> } while (0)
Ok. I have updated all three macros we have accordingly:
http://people.freebsd.org/~trociny/patches/hast.queue_insert_wakeup.1.patch
Pawel, what do you think?
--
Mikolaj Golub
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131204213704.GD4005>