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

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(a)redhat.com&gt; >> Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; >> --- >> 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?