Re: [libvirt] [PATCH resend V10 02/12] Resctrl: expose cache information to capabilities

On Mon, Mar 06, 2017 at 06:06:31PM +0800, Eli Qiao wrote: > This patch expose cache information to host's capabilites xml. > > For l3 cache allocation > <cache> > <bank id='0' type='l3' size='56320' unit='KiB' cpus='0-21,44-65'> > <control min='2816' reserved='2816' unit='KiB' scope='L3'/> > </bank> > <bank id='1' type='l3' size='56320' unit='KiB' cpus='22-43,66-87'> > <control min='2816' reserved='2816' unit='KiB' scope='L3'/> > </bank> > </cache> > > For l3 cache allocation supported cdp(seperate data/code): > <cache> > <bank id='0' type='l3' size='56320' unit='KiB' cpus='0-21,44-65'> > <control min='2816' reserved='2816' unit='KiB' scope='L3DATA'/> > <control min='2816' reserved='2816' unit='KiB' scope='L3CODE'/> > I know this was discussed before, but why having a vector value in a single attribute? Why don't we split it to two scalars? It will also be SOO much more readable and close to what other tools expose, e.g.: <control scope="data"/> <control scope="instruction"/> or <control scope="both"/> I also skipped the 'l3' because that is already in the <bank/> and will never be different, right?

> </bank> > <bank id='1' type='l3' size='56320' unit='KiB' cpus='22-43,66-87'> > <control min='2816' reserved='2816' unit='KiB' scope='L3DATA'/> > <control min='2816' reserved='2816' unit='KiB' scope='L3CODE'/> > </bank> > </cache> > > RFC on mailing list. > https://www.redhat.com/archives/libvir-list/2017-January/msg00644.html > > Signed-off-by: Eli Qiao <liyong.qiao(a)intel.com (mailto:liyong.qiao@intel.com)> > --- > src/conf/capabilities.c | 56 ++++++++++++++++++++++++++++++++++++++ > src/conf/capabilities.h | 23 ++++++++++++++++ > src/libvirt_private.syms | 3 +++ > src/nodeinfo.c | 64 ++++++++++++++++++++++++++++++++++++++++++++ > src/nodeinfo.h | 1 + > src/qemu/qemu_capabilities.c | 8 ++++++ > src/qemu/qemu_driver.c | 4 +++ > 7 files changed, 159 insertions(+) > > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c > index 9ab343b..23e21e4 100644 > --- a/src/conf/capabilities.c > +++ b/src/conf/capabilities.c > @@ -198,6 +198,18 @@ virCapabilitiesClearSecModel(virCapsHostSecModelPtr secmodel) > } > > static void > +virCapabilitiesClearCacheBank(virCapsHostCacheBankPtr cachebank) > +{ > + size_t i; > + for (i = 0; i < cachebank->ncontrol; i++) > + VIR_FREE(cachebank->control[i].scope); > + > + VIR_FREE(cachebank->type); > + VIR_FREE(cachebank->cpus); > +} > + > + > +static void > virCapabilitiesDispose(void *object) > { > virCapsPtr caps = object; > @@ -221,6 +233,10 @@ virCapabilitiesDispose(void *object) > virCapabilitiesClearSecModel(&caps->host.secModels[i]); > VIR_FREE(caps->host.secModels); > > + for (i = 0; i < caps->host.ncachebank; i++) > + virCapabilitiesClearCacheBank(caps->host.cachebank[i]); > + VIR_FREE(caps->host.cachebank); > + > VIR_FREE(caps->host.netprefix); > VIR_FREE(caps->host.pagesSize); > virCPUDefFree(caps->host.cpu); > @@ -844,6 +860,41 @@ virCapabilitiesFormatNUMATopology(virBufferPtr buf, > return 0; > } > > +static int > +virCapabilitiesFormatCache(virBufferPtr buf, > + size_t ncachebank, > + virCapsHostCacheBankPtr *cachebank) > +{ > + size_t i; > + size_t j; > + > + virBufferAddLit(buf, "<cache>\n"); > + virBufferAdjustIndent(buf, 2); > + > + for (i = 0; i < ncachebank; i++) { > + virBufferAsprintf(buf, > + "<bank id='%u' type='%s' size='%llu' unit='KiB' cpus='%s'>\n", > + cachebank[i]->id, > + cachebank[i]->type, > + cachebank[i]->size, > + cachebank[i]->cpus); > + > + virBufferAdjustIndent(buf, 2); > + for (j = 0; j < cachebank[i]->ncontrol; j++) { > + virBufferAsprintf(buf, > + "<control min='%llu' reserved='%llu' unit='KiB' scope='%s'/>\n", > + cachebank[i]->control[j].min, > + cachebank[i]->control[j].reserved, > + cachebank[i]->control[j].scope); > + } > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</bank>\n"); > + } > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</cache>\n"); > + return 0; > +} > + > /** > * virCapabilitiesFormatXML: > * @caps: capabilities to format > @@ -931,6 +982,11 @@ virCapabilitiesFormatXML(virCapsPtr caps) > virBufferAddLit(&buf, "</migration_features>\n"); > } > > + if (caps->host.ncachebank && > + virCapabilitiesFormatCache(&buf, caps->host.ncachebank, > + caps->host.cachebank) < 0) > + return NULL; > + > if (caps->host.netprefix) > virBufferAsprintf(&buf, "<netprefix>%s</netprefix>\n", > caps->host.netprefix); > diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h > index cfdc34a..b446de5 100644 > --- a/src/conf/capabilities.h > +++ b/src/conf/capabilities.h > @@ -138,6 +138,25 @@ struct _virCapsHostSecModel { > virCapsHostSecModelLabelPtr labels; > }; > > +typedef struct _virCapsHostCacheControl virCapsHostCacheControl; > +typedef virCapsHostCacheControl *virCapsHostCacheControlPtr; > +struct _virCapsHostCacheControl { > + unsigned long long min; > + unsigned long long reserved; > + char* scope; > +}; > + > +typedef struct _virCapsHostCacheBank virCapsHostCacheBank; > +typedef virCapsHostCacheBank *virCapsHostCacheBankPtr; > +struct _virCapsHostCacheBank { > + unsigned int id; > + char* type; > + char* cpus; > + unsigned long long size; > + size_t ncontrol; > + virCapsHostCacheControlPtr control; > +}; > + > typedef struct _virCapsHost virCapsHost; > typedef virCapsHost *virCapsHostPtr; > struct _virCapsHost { > @@ -160,6 +179,10 @@ struct _virCapsHost { > size_t nsecModels; > virCapsHostSecModelPtr secModels; > > + size_t ncachebank; > + size_t ncachebank_max; > + virCapsHostCacheBankPtr *cachebank; > + > char *netprefix; > virCPUDefPtr cpu; > int nPagesSize; /* size of pagesSize array */ > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index bb7c3ad..b8445ef 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1125,6 +1125,7 @@ virLogManagerNew; > # nodeinfo.h > nodeCapsInitNUMA; > nodeGetInfo; > +virCapsInitCache; > virHostCPUGetCount; > virHostCPUGetKVMMaxVCPUs; > virHostCPUGetMap; > @@ -2322,8 +2323,10 @@ virRandomInt; > > # util/virresctrl.h > virResCtrlAvailable; > +virResCtrlGet; > virResCtrlInit; > > + > # util/virrotatingfile.h > virRotatingFileReaderConsume; > virRotatingFileReaderFree; > diff --git a/src/nodeinfo.c b/src/nodeinfo.c > index f2ded02..a001cd0 100644 > --- a/src/nodeinfo.c > +++ b/src/nodeinfo.c > @@ -48,6 +48,7 @@ > #include "virstring.h" > #include "virnuma.h" > #include "virlog.h" > +#include "virresctrl.h" > > #define VIR_FROM_THIS VIR_FROM_NONE > > @@ -416,3 +417,66 @@ nodeCapsInitNUMA(virCapsPtr caps) > VIR_FREE(pageinfo); > return ret; > } > + > +int > +virCapsInitCache(virCapsPtr caps) > +{ > + size_t i, j; > + virResCtrlPtr resctrl; > + virCapsHostCacheBankPtr bank; > + > + for (i = 0; i < VIR_RDT_RESOURCE_LAST; i++) { > + /* L3DATA and L3CODE share L3 resources */ > + if (i == VIR_RDT_RESOURCE_L3CODE) > + continue; > + > + resctrl = virResCtrlGet(i); > + > + if (resctrl->enabled) { > + for (j = 0; j < resctrl->num_banks; j++) { > + if (VIR_RESIZE_N(caps->host.cachebank, caps->host.ncachebank_max, > + caps->host.ncachebank, 1) < 0) > + return -1; > + > + if (VIR_ALLOC(bank) < 0) > + return -1; > + > + bank->id = resctrl->cache_banks[j].host_id; > + if (VIR_STRDUP(bank->type, resctrl->cache_level) < 0) > + goto err; > + if (VIR_STRDUP(bank->cpus, virBitmapFormat(resctrl->cache_banks[j].cpu_mask)) < 0) > + goto err; > + bank->size = resctrl->cache_banks[j].cache_size; > + /*L3DATA and L3CODE shares L3 cache resources, so fill them to the control element*/ > + if (i == VIR_RDT_RESOURCE_L3DATA) { > + if (VIR_EXPAND_N(bank->control, bank->ncontrol, 2) < 0) > + goto err; > + > + bank->control[0].min = virResCtrlGet(VIR_RDT_RESOURCE_L3DATA)->cache_banks[j].cache_min; > + bank->control[0].reserved = bank->control[0].min * (virResCtrlGet(VIR_RDT_RESOURCE_L3DATA)->min_cbm_bits); > + if (VIR_STRDUP(bank->control[0].scope, > + virResCtrlGet(VIR_RDT_RESOURCE_L3DATA)->name) < 0) > + goto err; > + > + bank->control[1].min = virResCtrlGet(VIR_RDT_RESOURCE_L3CODE)->cache_banks[j].cache_min; > + bank->control[1].reserved = bank->control[1].min * (virResCtrlGet(VIR_RDT_RESOURCE_L3CODE)->min_cbm_bits); > + if (VIR_STRDUP(bank->control[1].scope, > + virResCtrlGet(VIR_RDT_RESOURCE_L3CODE)->name) < 0) > + goto err; > + } else { > + if (VIR_EXPAND_N(bank->control, bank->ncontrol, 1) < 0) > + goto err; > + bank->control[0].min = resctrl->cache_banks[j].cache_min; > + bank->control[0].reserved = bank->control[0].min * resctrl->min_cbm_bits; > + if (VIR_STRDUP(bank->control[0].scope, resctrl->name) < 0) > + goto err; > + } > + caps->host.cachebank[caps->host.ncachebank++] = bank; > + } > + } > + } > + return 0; > + err: > + VIR_FREE(bank); > + return -1; > +} > diff --git a/src/nodeinfo.h b/src/nodeinfo.h > index 3c4dc46..5eb0f83 100644 > --- a/src/nodeinfo.h > +++ b/src/nodeinfo.h > @@ -28,5 +28,6 @@ > > int nodeGetInfo(virNodeInfoPtr nodeinfo); > int nodeCapsInitNUMA(virCapsPtr caps); > +int virCapsInitCache(virCapsPtr caps); > > #endif /* __VIR_NODEINFO_H__*/ > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 359a0d8..a0d7254 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -1100,6 +1100,11 @@ virQEMUCapsInitCPU(virCapsPtr caps, > goto cleanup; > } > > +static int > +virQEMUCapsInitCache(virCapsPtr caps) > +{ > + return virCapsInitCache(caps); > +} > > static int > virQEMUCapsInitPages(virCapsPtr caps) > @@ -1146,6 +1151,9 @@ virCapsPtr virQEMUCapsInit(virQEMUCapsCachePtr cache) > if (virQEMUCapsInitCPU(caps, hostarch) < 0) > VIR_WARN("Failed to get host CPU"); > > + if (virQEMUCapsInitCache(caps) < 0) > + VIR_WARN("Failed to get host cache"); > + > /* Add the power management features of the host */ > if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) > VIR_WARN("Failed to get host power management capabilities"); > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 77d8175..3bfb4ec 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -105,6 +105,7 @@ > #include "vircgroup.h" > #include "virperf.h" > #include "virnuma.h" > +#include "virresctrl.h" > #include "dirname.h" > #include "network/bridge_driver.h" > > @@ -849,6 +850,9 @@ qemuStateInitialize(bool privileged, > run_gid = cfg->group; > } > > + if (virResCtrlAvailable() && virResCtrlInit() < 0) > + VIR_WARN("Faild to initialize resource control."); > + > s/Faild/Failed/ Also the Available() call seems unnecessary since Init() is graceful to unavailability IIRC.

Anyway if this is to be done once per daemon lifetime, it should be wrapped using VIR_ONCE (see VIR_ONCE_GLOBAL_INIT all over the codebase). I believe I mentioned this already in some previous version.

> qemu_driver->qemuCapsCache = virQEMUCapsCacheNew(cfg->libDir, > cfg->cacheDir, > run_uid, > -- > 1.9.1 > > -- > libvir-list mailing list > libvir-list(a)redhat.com (mailto:libvir-list@redhat.com) > https://www.redhat.com/mailman/listinfo/libvir-list >