On Thu, Oct 28, 2021 at 05:26:06PM +0200, Laszlo Ersek wrote:
> 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).
I'll add the check that h->commands.size == 1 before doing the signal.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
