Libguestfs - May 2019 - Re: [nbdkit PATCH 1/2] plugins: Add .thread_model callback

On Mon, May 20, 2019 at 07:30:31AM -0500, Eric Blake
wrote:
> +=head2 C<.thread_model>
> +
> + int thread_model (void)
> +
> +This optional callback is called after all the configuration has been
> +passed to the plugin.  It can be used to force a stricter thread model
> +based on configuration, compared to C<THREAD_MODEL>.  See
L</THREADS>
> +below for details.  Attempts to request a looser (more parallel) model
> +are silently ignored.
> +
> +If there is an error, C<.thread_model> should call
C<nbdkit_error>
> +with an error message and return C<-1>.
Two comments:

(1) Do we have an opportunity to change the way the thread model is
specified to allow future expansion.  This might involve returning
something other than the simple int, or could be that we specify
different constants to be returned by this call.

(Actually while making that point I think I've talked myself out of
it.  But probably we should do this in the V3 API, so a note can be
added to that effect at the end of the TODO file.)

(2) Shouldn't it be an error if the thread model returns a more
parallel thread model than the constant?

Anyway patch series generally looks fine to me.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 5/20/19 8:36 AM, Richard W.M. Jones wrote:
> On Mon, May 20, 2019 at 07:30:31AM -0500, Eric Blake wrote:
>> +=head2 C<.thread_model>
>> +
>> + int thread_model (void)
>> +
>> +This optional callback is called after all the configuration has been
>> +passed to the plugin.  It can be used to force a stricter thread model
>> +based on configuration, compared to C<THREAD_MODEL>.  See
L</THREADS>
>> +below for details.  Attempts to request a looser (more parallel) model
>> +are silently ignored.
>> +
>> +If there is an error, C<.thread_model> should call
C<nbdkit_error>
>> +with an error message and return C<-1>.
> 
> Two comments:
> 
> (1) Do we have an opportunity to change the way the thread model is
> specified to allow future expansion.  This might involve returning
> something other than the simple int, or could be that we specify
> different constants to be returned by this call.
> 
> (Actually while making that point I think I've talked myself out of
> it.  But probably we should do this in the V3 API, so a note can be
> added to that effect at the end of the TODO file.)
So far, we've come up with two potential models to add: VDDK's model
where only the main thread is used but where parallel connections can
still be made (that is slightly different than SERIALIZE_ALL_REQUESTS,
not sure where it would lie conceptually between existing models), and a
model that allows parallel requests from a single connection but delays
responses to be in order (perhaps named SERIALIZE_RESPONSES,
conceptually lies between SERIALIZE_REQUESTS and PARALLEL).

We could add a utility helper function that, when given two constants,
returns -1/0/1 depending on whether the first parameter is
stricter/equal/looser than the second, to at least ease any out-of-order
defined integer values back into a hierarchical scheme.  I also wonder
if we need a utility function for a plugin to learn which threading
model ACTUALLY got selected, usable during .open or later (if the plugin
supports PARALLEL but is run with a filter that forces
SERIALIZE_ALL_REQUESTS, the plugin might be able to optimize out the use
of a mutex).
> 
> (2) Shouldn't it be an error if the thread model returns a more
> parallel thread model than the constant?
I thought about that, but my initial idea was to instead just declare
that attempts at a more parallel model are just silently ignored (with
at most a debug message visible during -fv).
> 
> Anyway patch series generally looks fine to me.
> 
> Rich.
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 5/20/19 9:18 AM, Eric Blake wrote:
>> (2) Shouldn't it be an error if the thread model returns a more
>> parallel thread model than the constant?
> 
> I thought about that, but my initial idea was to instead just declare
> that attempts at a more parallel model are just silently ignored (with
> at most a debug message visible during -fv).
I hit send too soon. My biggest reasoning for this? Consider the python
plugin. If we allow a python script to request PARALLEL now (but
silently ignore it, because our C glue code currently defines the
constant to SERIALIZE_ALL_REQUESTS), then that script will instantly
gain parallel support if a future nbdkit version manages to get the C
glue code rewritten to use better re-entrancy to the point of redefining
THREAD_MODEL to PARALLEL after all. (Based on code comments, it may be
doable with some effort for python, although I have less hopes for Ruby).
> 
>>
>> Anyway patch series generally looks fine to me.
Thanks; I'll push shortly.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org