Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank
On Tuesday, 11 April 2017 at 4:10 PM, Martin Kletzander wrote: > This should just be: > > + if (virAsprintf(, "%s/vircaps2xmldata/linux-%s/resctrl", > + abs_srcdir, data->filename) < 0) > yep, stupid me. I get it now. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank
On Tuesday, 11 April 2017 at 3:48 PM, 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:
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > For CDP enabled on x86 arch, we will have:
> >
> >
> >
> >
> >
> > ...
> >
> > * 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.
>
>
what I did is that expose what the host’s sys/fs/resctrl/info looks like, if
people
think we should use lower case, I am okay to change.
>
> >
> > Signed-off-by: Eli Qiao > (mailto:liyong.q...@intel.com)>
> > ---
> >
>
>
> [...]
>
> > diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> > index 2080953..bfed4f8 100644
> > --- a/docs/schemas/capability.rng
> > +++ b/docs/schemas/capability.rng
> > @@ -277,6 +277,26 @@
> >
> >
> >
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > + BOTH
> > + CODE
> > + DATA
> >
>
>
> Why are the values all caps? We prefer lowercase attributes in the XML.
>
Answer in previous reply.
>
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> >
> >
> >
> > 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
> > @@ -52,6 +52,7 @@
> > #define VIR_FROM_THIS VIR_FROM_CAPABILITIES
> >
> > #define SYSFS_SYSTEM_PATH "/sys/devices/system/"
> > +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"
> >
> > VIR_LOG_INIT("conf.capabilities")
> >
> > @@ -873,6 +874,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> > virCapsHostCacheBankPtr *caches)
> > {
> > size_t i = 0;
> > + size_t j = 0;
> > + int indent = virBufferGetIndent(buf, false);
> > + virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
> >
> > if (!ncaches)
> > return 0;
> > @@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> > */
> > virBufferAsprintf(buf,
> > " > - "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(, indent + 4);
> >
>
>
> This looks wrong. You should increase/decrease the indent by some
> number. The old value should not be used.
>
I have test case covered, please check
tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
>
> > + for (j = 0; j < bank->ncontrols; j++) {
> > + bool min_kilos = !(bank->controls[j]->min % 1024);
> > + virBufferAsprintf(,
> > + " > + "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.
Just copy from other place, I am okay to remove it.
> > + if (virBufferUse()) {
>
>
> This logic looks weird and opaque. Can you decide by any other means
> than the filling of the buffer?
>
>
It’s same meaning with bank->ncontrols > 0
this testfile will help you to easy understand what’s doing here.
tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
I am okay to change if you feel it’s hard to get understand.
>
> > + virBufferAddLit(buf, ">\n");
> > + virBufferAddBuffer(buf, );
> > + virBufferAddLit(buf, "\n");
> > +
> > + } else {
> > + virBufferAddLit(buf, "/>\n");
> > + }
> > +
> > +
> > + virBufferFreeAndReset();
> > VIR_FREE(cpus_str);
> > }
> >
> > @@ -1513,13 +1542,101 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr
> > a,
> > void
> > virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
> > {
> > + size_t i;
> > +
> > if (!ptr)
> > return;
> >
> > virBitmapFree(ptr->cpus);
> > + for (i = 0; i < ptr->ncontrols; i++)
> > + VIR_FREE(ptr->controls[i]);
> > + VIR_FREE(ptr->controls);
> > VIR_FREE(ptr);
> > }
> >
> > +VIR_ENUM_IMPL(virResctrlCache, VIR_RESCTRL_CACHE_TYPE_LAST,
> > + "BOTH",
> > + "CODE",
> > + "DATA")
> > +
> > +/* test which TYPE of cache control supported
> > + * -1: don't support
> > + * 0: cat
> > + * 1: cdp
> > + */
> > +static int
> > +virCapabilitiesGetCacheControlType(virCapsHostCacheBankPtr bank)
> > +{
> > + int ret = -1;
> > + char *path = NULL;
> > + if (virAsprintf(,
> > + SYSFS_RESCTRL_PATH "info/L%u",
> > + bank->level) < 0)
> > + return -1;
> > +
> > + if (virFileExists(path)) {
> > + ret =
Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank
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:
For CDP enabled on x86 arch, we will have:
...
* 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
---
[...]
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,
"\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(, 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(,
+ "\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()) {
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, );
+virBufferAddLit(buf, "\n");
+
+} else {
+virBufferAddLit(buf, "/>\n");
+}
+
+
+virBufferFreeAndReset();
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(, "%s/vircaps2xmldata/linux-%s/resctrl",
+abs_srcdir,
+data->resctrl ? data->filename : "foo") < 0)
"foo"? Some testing code leftover?
This should just be:
+if (virAsprintf(, "%s/vircaps2xmldata/linux-%s/resctrl",
+abs_srcdir, data->filename) < 0)
I think I said that in v3
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank
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:
>
>
>
>
>
>
>
>
>
>
> For CDP enabled on x86 arch, we will have:
>
>
>
>
>
> ...
>
> * 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
> ---
[...]
> diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> index 2080953..bfed4f8 100644
> --- a/docs/schemas/capability.rng
> +++ b/docs/schemas/capability.rng
> @@ -277,6 +277,26 @@
>
>
>
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> + BOTH
> + CODE
> + DATA
Why are the values all caps? We prefer lowercase attributes in the XML.
> +
> +
> +
> +
> +
> +
> +
>
>
>
> 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
> @@ -52,6 +52,7 @@
> #define VIR_FROM_THIS VIR_FROM_CAPABILITIES
>
> #define SYSFS_SYSTEM_PATH "/sys/devices/system/"
> +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"
>
> VIR_LOG_INIT("conf.capabilities")
>
> @@ -873,6 +874,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> virCapsHostCacheBankPtr *caches)
> {
> size_t i = 0;
> +size_t j = 0;
> +int indent = virBufferGetIndent(buf, false);
> +virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
>
> if (!ncaches)
> return 0;
> @@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> */
> virBufferAsprintf(buf,
>" - "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(, indent + 4);
This looks wrong. You should increase/decrease the indent by some
number. The old value should not be used.
> +for (j = 0; j < bank->ncontrols; j++) {
> +bool min_kilos = !(bank->controls[j]->min % 1024);
> +virBufferAsprintf(,
> + " + "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()) {
This logic looks weird and opaque. Can you decide by any other means
than the filling of the buffer?
> +virBufferAddLit(buf, ">\n");
> +virBufferAddBuffer(buf, );
> +virBufferAddLit(buf, "\n");
> +
> +} else {
> +virBufferAddLit(buf, "/>\n");
> +}
> +
> +
> +virBufferFreeAndReset();
> VIR_FREE(cpus_str);
> }
>
> @@ -1513,13 +1542,101 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a,
> void
> virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
> {
> +size_t i;
> +
> if (!ptr)
> return;
>
> virBitmapFree(ptr->cpus);
> +for (i = 0; i < ptr->ncontrols; i++)
> +VIR_FREE(ptr->controls[i]);
> +VIR_FREE(ptr->controls);
> VIR_FREE(ptr);
> }
>
> +VIR_ENUM_IMPL(virResctrlCache, VIR_RESCTRL_CACHE_TYPE_LAST,
> + "BOTH",
> + "CODE",
> + "DATA")
> +
> +/* test which TYPE of cache control supported
> + * -1: don't support
> + * 0: cat
> + * 1: cdp
> + */
> +static int
> +virCapabilitiesGetCacheControlType(virCapsHostCacheBankPtr bank)
> +{
> +int ret = -1;
> +char *path = NULL;
> +if (virAsprintf(,
> +SYSFS_RESCTRL_PATH "info/L%u",
> +bank->level) < 0)
> +return -1;
> +
> +if (virFileExists(path)) {
> +
[libvirt] [PATCH V4] Expose resource control capabilites on cache bank
This patch is based on Martin's cache branch.
* This patch amends the cache bank capability as follow:
For CDP enabled on x86 arch, we will have:
...
* 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.
Signed-off-by: Eli Qiao
---
docs/schemas/capability.rng| 20 +++
src/conf/capabilities.c| 135 -
src/conf/capabilities.h| 26 +++-
.../vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus | 1 +
.../linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask | 1 +
.../resctrl/info/L3CODE/min_cbm_bits | 1 +
.../resctrl/info/L3CODE/num_closids| 1 +
.../linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask | 1 +
.../resctrl/info/L3DATA/min_cbm_bits | 1 +
.../resctrl/info/L3DATA/num_closids| 1 +
.../linux-resctrl-cdp/resctrl/manualres/cpus | 1 +
.../linux-resctrl-cdp/resctrl/manualres/schemata | 2 +
.../linux-resctrl-cdp/resctrl/manualres/tasks | 0
.../linux-resctrl-cdp/resctrl/schemata | 2 +
.../linux-resctrl-cdp/resctrl/tasks| 0
tests/vircaps2xmldata/linux-resctrl-cdp/system | 1 +
.../vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml | 55 +
tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 8 +-
tests/vircaps2xmltest.c| 10 ++
19 files changed, 261 insertions(+), 6 deletions(-)
create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus
create mode 100755
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask
create mode 100755
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/min_cbm_bits
create mode 100755
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/num_closids
create mode 100755
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask
create mode 100755
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/min_cbm_bits
create mode 100755
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/num_closids
create mode 100644
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/cpus
create mode 100644
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/schemata
create mode 100644
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/tasks
create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/schemata
create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/tasks
create mode 12 tests/vircaps2xmldata/linux-resctrl-cdp/system
create mode 100644 tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
index 2080953..bfed4f8 100644
--- a/docs/schemas/capability.rng
+++ b/docs/schemas/capability.rng
@@ -277,6 +277,26 @@
+
+
+
+
+
+
+
+
+
+
+ BOTH
+ CODE
+ DATA
+
+
+
+
+
+
+
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
@@ -52,6 +52,7 @@
#define VIR_FROM_THIS VIR_FROM_CAPABILITIES
#define SYSFS_SYSTEM_PATH "/sys/devices/system/"
+#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"
VIR_LOG_INIT("conf.capabilities")
@@ -873,6 +874,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
virCapsHostCacheBankPtr *caches)
{
size_t i = 0;
+size_t j = 0;
+int indent = virBufferGetIndent(buf, false);
+virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
if (!ncaches)
return 0;
@@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
*/
virBufferAsprintf(buf,
"\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(, indent + 4);
+for (j = 0; j < bank->ncontrols; j++) {
+bool min_kilos = !(bank->controls[j]->min % 1024);
+virBufferAsprintf(,
+ "\n",
+ bank->controls[j]->min >> (min_kilos * 10),
+
