Date: Thu, 05 Dec 2013 10:59:33 +0800 From: David Xu <davidxu@freebsd.org> To: Mikolaj Golub <trociny@FreeBSD.org> Cc: freebsd-stable@freebsd.org, Pete French <petefrench@ingresso.co.uk> Subject: Re: Hast locking up under 9.2 Message-ID: <529FEC15.3080509@freebsd.org> In-Reply-To: <20131204213704.GD4005@gmail.com> 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> <20131204213704.GD4005@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2013/12/05 05:37, Mikolaj Golub wrote:
> 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?
>
No problem. ;-)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?529FEC15.3080509>