[PATCH v3 10/15] include: Introduce typed params for virDomainSetIOThreadParams wrt pool size

[Daniel P. Berrangé]

On Wed, Jun 29, 2022 at 05:08:01PM +0200, Michal Prívozník wrote:
> On 6/29/22 16:19, Daniel P. Berrangé wrote:
> > On Wed, Jun 08, 2022 at 03:43:04PM +0200, Michal Privoznik wrote:
> >> Our public API offers virDomainSetIOThreadParams() function which
> >> allows users to set various aspects of IOThreads. Introduce two
> >> new typed parameters: VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN and
> >> VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX which will allow users to
> >> modify the thread-pool-min and thread-pool-max attributes of an
> >> iothread.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> >> Reviewed-by: Peter Krempa <pkrempa at redhat.com>
> >> ---
> >>  include/libvirt/libvirt-domain.h | 28 ++++++++++++++++++++++++++++
> >>  1 file changed, 28 insertions(+)
> >>
> >> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> >> index 2aec69bc54..1ea3284e63 100644
> >> --- a/include/libvirt/libvirt-domain.h
> >> +++ b/include/libvirt/libvirt-domain.h
> >> @@ -2499,6 +2499,34 @@ int                  virDomainDelIOThread(virDomainPtr domain,
> >>   */
> >>  # define VIR_DOMAIN_IOTHREAD_POLL_SHRINK "poll_shrink"
> >>  
> >> +/**
> >> + * VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN:
> >> + *
> >> + * Sets the lower bound for thread pool size. A value of -1 disables this bound
> >> + * leaving hypervisor use its default value, though this value is not accepted
> >> + * for running domains. Due to internal implementation it's recommended to set
> >> + * VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN and VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX
> >> + * separately. Accepted type is VIR_TYPED_PARAM_INT.
> > 
> > 
> > What's the story with this comment about setting pool-min and pool-max
> > separately ?
> > 
> > This feels like a impl detail that should never be exposed in the API.
> > 
> > If we need to set them separately with QEMU, then the QEMU driver
> > should make separate QMP calls to set them as needed. The app should
> > never have to care about this.
> 
> The story is that min and max values are set in separate monitor
> commands (qom-set). So depending on the current values, min has to be
> set before max or vice verca. So we would need a logic that gets the
> current values and acts accordingly. While I agree it's not user
> friendly, we can implement the logic later. Or do you want to make it
> before the release?

I'd rather we at least removed this sentence from the public API docs,
and simply treat it as a pending bug fix for next release.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|