Re: [Libguestfs] [PATCH nbdkit 5/5] vddk: Implement parallel thread model

On 10/28/21 13:57, Richard W.M. Jones wrote: 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