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: > > <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(a)intel.com&gt; > ---

> 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.

> + 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?

> + 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?