[libvirt] [PATCH 4/9] conf: Replace 'virResctrlAllocIsEmpty' with @n==0 for indicating no resctrl allocation in configuration file

[Huaqiang,Wang]

On 2019年05月27日 23:27, Michal Privoznik wrote:
> On 5/23/19 11:34 AM, Wang Huaqiang wrote:
>> @n denotes the number of <cache> element under <cputune/cachetune>
>> element in function 'virDomainCachetuneDefParse'  or the number of
>> <node> element under <cputune/memorytune> element in function
>> virDomainMemorytuneDefParse'.
>> Originally it is using 'virResctrlAllocIsEmpty' function to judge
>> if no resctrl allocation defined in <cputune/cachetune> or
>> <cputune/memorytune> element, this role could be replaced with
>> checking if @n is zero or not.
>>
>> This replacement is more efficient and avoiding a long function
>> calling path.
>>
>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
>> ---
>>   src/conf/domain_conf.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 9c95467..dcfd2dd 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -19360,7 +19360,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>>         /* If no <cache> element or <monitor> element in <cachetune>, 
>> do not
>>        * append any resctrl element */
>> -    if (!resctrl->nmonitors && virResctrlAllocIsEmpty(alloc)) {
>> +    if (!resctrl->nmonitors && n == 0) {
>>           ret = 0;
>>           goto cleanup;
>>       }
>> @@ -19550,7 +19550,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
>>               return -1;
>>       }
>>   -    if (virResctrlAllocIsEmpty(alloc))
>> +    if (n == 0)
>>           return 0;
>>         /*
>>
>
> Is this a good idea? virResctrlAllocIsEmpty does more than plain n == 0.

Yes, I have omitted some details, actually using virResctrlAllocIsEmpty will
cause some logic error in original code in processing memory bandwidth
groups which is not fully test

When detecting the configurations from XML file, the cache allocation 
and monitor
settings are first parsed and then the memory bandwidth allocation settings.

The desired logic when paring  resource control group settings are:

** For cache resctrl groups **

Cache resctrl groups, both allocation group and monitor groups, are firstly
parsed, if any cache allocation group (the <cputune>/<cachetune>/<cache>
element) or monitor group (the <cputune>/<cachetune>/<monitor> element)
is parsed, it requires to create and register a @def->resctrls element.

So, in this case, @n represents the number of <cputune>/<cachetune>/<cache>
element, and @resctrl->nmonitors denotes the number of
<cputune>/<cachetune>/<monitor> element. If @n==0 and
@resctrl->nmonitors==0, then it tells no cache allocation and cache monitor
groups are defined in XML file, it is not necessary to create any 
@def->resctrls
objects.

We need to ensure that one or more allocation groups will be created
in underlying util/resctrl module if value of @n is not zero, this could be
guaranteed by current XML schemas rules.

If we do not consider the performance, calling virResctrlAllocIsEmpty 
here is
logically correct because each cache allocation group will register an 
'allocation'
object in util/resctrl module. If codes runs without error, then @n 
represents the
allocation objects in util/resctrl module.

So, for cache resctrl groups, there is no need to call 
virResctrlAllocIsEmpty.

** For memory bandwidth resctrl groups **

It hopes to append a @resctrl element to @def->resctrls array if some
valid <cputune>/<memorytune>/<bank> elements have been found, and
the @n in this part of code represents the number of elements. It is no
need to call virResctrlAllocIsEmpty if we make sure each
<cputune>/<memorytune>/<bank> item in XML file is valid and will
create some allocation group.

Another thing I haven't mentioned that using virResctrlAllocIsEmpty here
will make error. If some cache allocations are created, the function will
return 'NOT empty' regardless the number of memory bandwidth allocations
ordered in the XML file. This is logically wrong.

>
> Michal

Thanks for comments.
Huaqiang