Richard W.M. Jones
2021-Oct-28 11:57 UTC
[Libguestfs] [PATCH nbdkit 5/5] vddk: Implement parallel thread model
On Thu, Oct 28, 2021 at 12:39:03PM +0100, Richard W.M. Jones wrote:> On Wed, Oct 27, 2021 at 07:42:31PM +0200, Laszlo Ersek wrote: > > (4) This could be slightly optimized by restricting the condvar signal > > to "queue size is now exactly 1 (i.e., it was empty)". > > I think there's a possible race with that, but let me think about it.Yes - the race is that the worker thread hasn't started up yet / hasn't reached the first point where it's waiting on the condition. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Laszlo Ersek
2021-Oct-28 15:26 UTC
[Libguestfs] [PATCH nbdkit 5/5] vddk: Implement parallel thread model
On 10/28/21 13:57, Richard W.M. Jones wrote:> On Thu, Oct 28, 2021 at 12:39:03PM +0100, Richard W.M. Jones wrote: >> On Wed, Oct 27, 2021 at 07:42:31PM +0200, Laszlo Ersek wrote: >>> (4) This could be slightly optimized by restricting the condvar signal >>> to "queue size is now exactly 1 (i.e., it was empty)". >> >> I think there's a possible race with that, but let me think about it. > > Yes - the race is that the worker thread hasn't started up yet / > hasn't reached the first point where it's waiting on the condition.That's not a race, IMO. producers: + /* Add the command to the command queue. */ + { + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&h->commands_lock); + r = command_queue_append (&h->commands, cmd); + if (r == 0) + pthread_cond_signal (&h->commands_cond); + } consumer: + /* Wait until we are sent at least one command. */ + { + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&h->commands_lock); + while (h->commands.size == 0) + pthread_cond_wait (&h->commands_cond, &h->commands_lock); + cmd = h->commands.ptr[0]; + command_queue_remove (&h->commands, 0); + } Access to the queue is properly protected by mutual exclusion, between the multiple producers and the one consumer. (Also, spurious wakepus are covered well.) The condition variable is only needed for waking up the consumer thread precisely when the queue goes from "empty" to "non-empty" (i.e., upon the transition). Whenever the consumer thread finds the queue "non-empty" -- be that the very first time it ever looks, or just one iteration of its big loop --, the consumer thread never reaches pthread_cond_wait(), so there is no need for a signal. In other words, when a producer finds that, upon having locked the queue and preparing to add a command, the queue is already not empty, it *knows* that the consumer *cannot* be sleeping on the condvar. (It's possible that the consumer has not been rescheduled yet, since it last went to sleep in pthread_cond_wait(), but the producer that turned the queue from empty to non-empty will have signaled it, so there's no need for *this* producer to signal it again.) For example, let's assume we have two producers placing one request each in the queue before the consumer thread gets to look (= gets to acquire the command lock for the very first time). The first producer will signal the condvar, and that signal will be lost. OK. The second producer will not signal the condvar (under my proposal). Then the consumer will not wait on the condvar at all, because by the time it first takes the lock, h->commands.size will be 2. Signaling condvars precisely and exclusively upon state transitions is one of my hobby horses; I apologize for splitting hairs. If I failed to convince you, I can accept that of course (obviously a special case of that is if you can show a bug in my reasoning). Thanks! Laszlo