[libvirt] [PATCH V4] Expose resource control capabilites on cache bank

[Martin Kletzander]

On Tue, Apr 11, 2017 at 09:48:54AM +0200, Peter Krempa wrote:
>On Tue, Apr 11, 2017 at 13:44:26 +0800, Eli Qiao wrote:
>> This patch is based on Martin's cache branch.
>>
>> * This patch amends the cache bank capability as follow:
>>
>> <cache>
>>   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
>>     <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
>>   </bank>
>>   <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'>
>>     <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/>
>>   </bank>
>> </cache>
>>
>> For CDP enabled on x86 arch, we will have:
>> <cache>
>>   <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'>
>>     <control min='768' unit='KiB' scope='CODE' max_allocation='4'/>
>>     <control min='768' unit='KiB' scope='DATA' max_allocation='4'/>
>>   </bank>
>> ...
>>
>> * Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
>> case.
>>
>> * Along with vircaps2xmltest case updated.
>>
>> P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I
>> keep it capital upper as I need to use a macro to convert from enum to
>> string easily.
>
>We dont need to do that. The attributes should be lower case. The code
>can convert it to anything it needs.
>
>>
>> Signed-off-by: Eli Qiao <liyong.qiao at intel.com>
>> ---

[...]

>> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>> index 416dd1a..c6641d1 100644
>> --- a/src/conf/capabilities.c
>> +++ b/src/conf/capabilities.c

[...]

>> @@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>>           */
>>          virBufferAsprintf(buf,
>>                            "<bank id='%u' level='%u' type='%s' "
>> -                          "size='%llu' unit='%s' cpus='%s'/>\n",
>> +                          "size='%llu' unit='%s' cpus='%s'",
>>                            bank->id, bank->level,
>>                            virCacheTypeToString(bank->type),
>>                            bank->size >> (kilos * 10),
>>                            kilos ? "KiB" : "B",
>>                            cpus_str);
>>
>> +        /* Format control */
>> +        virBufferAdjustIndent(&controlBuf, indent + 4);
>
>This looks wrong. You should increase/decrease the indent by some
>number. The old value should not be used.
>

This is used everywhere in the code with childrenBuf, it's pretty widely
used and I think readable as well.  I agree with the rest of the review,
though.

>> +        for (j = 0; j < bank->ncontrols; j++) {
>> +            bool min_kilos = !(bank->controls[j]->min % 1024);
>> +            virBufferAsprintf(&controlBuf,
>> +                              "<control min='%llu' unit='%s' "
>> +                              "scope='%s' max_allocation='%u'/>\n",
>> +                              bank->controls[j]->min >> (min_kilos * 10),
>> +                              min_kilos ? "KiB" : "B",
>> +                              virResctrlCacheTypeToString(bank->controls[j]->scope),
>> +                              bank->controls[j]->max_allocation);
>> +        }
>> +
>> +        /* Put it all together */
>
>You don't need to point out obvious things.
>
>> +        if (virBufferUse(&controlBuf)) {
>
>This logic looks weird and opaque. Can you decide by any other means
>than the filling of the buffer?
>

Same here.

>> +            virBufferAddLit(buf, ">\n");
>> +            virBufferAddBuffer(buf, &controlBuf);
>> +            virBufferAddLit(buf, "</bank>\n");
>> +
>> +        } else {
>> +            virBufferAddLit(buf, "/>\n");
>> +        }
>> +
>> +
>> +        virBufferFreeAndReset(&controlBuf);
>>          VIR_FREE(cpus_str);
>>      }
>>

[...]

>> diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
>> index f590249..db7de4c 100644
>> --- a/tests/vircaps2xmltest.c
>> +++ b/tests/vircaps2xmltest.c
>> @@ -58,6 +59,13 @@ test_virCapabilities(const void *opaque)
>>                      data->resctrl ? "/system" : "") < 0)
>>          goto cleanup;
>>
>> +    if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
>> +                    abs_srcdir,
>> +                    data->resctrl ? data->filename : "foo") < 0)
>
>"foo"? Some testing code leftover?
>

This should just be:

+    if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
+                    abs_srcdir, data->filename) < 0)

I think I said that in v3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170411/a443cc03/attachment-0001.sig>