[PATCH 07/16] conf: Introduce pool_min and pool_max attributes to IOThread

[Michal Prívozník]

On 6/2/22 10:08, Peter Krempa wrote:
> On Thu, Jun 02, 2022 at 09:17:57 +0200, Michal Privoznik wrote:
>> At least in case of QEMU an IOThread is actually a pool of
>> threads (see iothread_set_aio_context_params() in QEMU's code
>> base). As such, it can have minimal and maximal number of worker
>> threads. Allow setting them in domain XML.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  docs/formatdomain.rst                         |  6 +-
>>  src/conf/domain_conf.c                        | 32 +++++++++-
>>  src/conf/domain_conf.h                        |  3 +
>>  src/conf/schemas/domaincommon.rng             | 10 +++
>>  .../iothreads-ids-pool-sizes.xml              | 61 +++++++++++++++++++
>>  ...iothreads-ids-pool-sizes.x86_64-latest.xml |  1 +
>>  tests/qemuxml2xmltest.c                       |  1 +
>>  7 files changed, 110 insertions(+), 4 deletions(-)
>>  create mode 100644 tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml
>>  create mode 120000 tests/qemuxml2xmloutdata/iothreads-ids-pool-sizes.x86_64-latest.xml
>>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index 312b605a8b..de085f616a 100644
>> --- a/docs/formatdomain.rst
>> +++ b/docs/formatdomain.rst
> 
> [...]
> 
>> @@ -696,6 +696,10 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)`
>>     any predefined ``id``. If there are more ``iothreadids`` defined than
>>     ``iothreads`` defined for the domain, then the ``iothreads`` value will be
>>     adjusted accordingly. :since:`Since 1.2.15`
>> +   The element has two optional attributes ``pool_min`` and ``pool_max`` which
>> +   allow setting lower and upper boundary for number of worker threads for
>> +   given IOThread. :since:`Since 8.4.0`
> 
> 8.5.0
> 

Ooops yes. This is what you get when you write patches but don't finish
them before entering freeze :-)

>> +
> 
> Drop this extra line.
> 
> Also I'd go probably for 'thread_pool_min/max'. In case of iothreads it
> doesn't matter too much, but later I'll object to 'mainloop' qemuism
> being introduced as a term rivaling our established 'emulator'.
> 
> Using just 'pool' there would be a bit too ambiguous IMO.
> 
>>  
>>  
>>  CPU Tuning
> 
> [...]
> 
>>  void virDomainIOThreadIDDefFree(virDomainIOThreadIDDef *def);
>> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
>> index cc598212a8..94035c38e7 100644
>> --- a/src/conf/schemas/domaincommon.rng
>> +++ b/src/conf/schemas/domaincommon.rng
>> @@ -829,6 +829,16 @@
>>                <attribute name="id">
>>                  <ref name="unsignedInt"/>
>>                </attribute>
>> +              <optional>
>> +                <attribute name="pool_min">
>> +                  <ref name="unsignedLong"/>
>> +                </attribute>
>> +              </optional>
>> +              <optional>
>> +                <attribute name="pool_max">
>> +                  <ref name="unsignedLong"/>
>> +                </attribute>
>> +              </optional>
> 
> I wondered how we could have distinguished between a long and int in the
> schema and the answer is ... we don't.
> 
> Additionally using long long even feels a bit overkill. 2 billion
> threads ought to be enough for everybody.

I can switch to int. I don't care that much honestly. I just looked at
what QEMU has and mimicked that. Do you want me to switch? That would
make patch 2 obsolete as nobody we already have virXMLPropInt().

> 
> With the doc fixes ... and potential re-name:
> 
> Reviewed-by: Peter Krempa <pkrempa at redhat.com>
> 

Thanks,
Michal