Richard W.M. Jones
2021-Oct-28 11:39 UTC
[Libguestfs] [PATCH nbdkit 5/5] vddk: Implement parallel thread model
On Wed, Oct 27, 2021 at 07:42:31PM +0200, Laszlo Ersek wrote:> On 10/27/21 14:21, Richard W.M. Jones wrote: > > +/* Queue of asynchronous commands sent to the background thread. */ > > +enum command_type { GET_SIZE, READ, WRITE, FLUSH, CAN_EXTENTS, EXTENTS, STOP }; > > (1) Shouldn't we use a common prefix for these enum constants?If we get a conflict with a header then yes. For example I was disappointed a while back to see that I couldn't create a local variable called (IIRC) "socket" (or maybe "connect") - which is ridiculous! If it's a symbol that needs to be namespaced (eg. it's part of a public API) then absolutely required. But the rest of the time ... I'm not that bothered, saves typing.> > + /* 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); > > + } > > (*shudder* -- the automatic releasing of the mutex, upon exiting the > scope, combined with the explicit signaling of the associated condvar, > is terrible to read. Functionally it is correct, though.)It's a bit awkward in this particular case, but I think this automatic releasing of mutexes in general is *great*. In many cases you put the macro at the top of the function, fire and forget. No need to painfully clean up on every exit path. Of course it does require a C extension ...> (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.> (11a) The switch statement is enormous. Each branch should have a > dedicated helper function.Yup, I did that change already, makes it a lot easier to follow :-) I'll follow up on the rest in the v2 patch. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
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/