[libvirt] [PATCH 7/9] conf: Refactor virDomainCachetuneDefParse
John Ferlan
jferlan at redhat.com
Thu Jul 26 17:48:23 UTC 2018
On 07/18/2018 03:57 AM, bing.niu at intel.com wrote:
> From: Bing Niu <bing.niu at intel.com>
>
> Refactoring virDomainCachetuneDefParse, extracting vcpus extracting,
> overlapping detecting and new resctrl allocation creating logic.
> Those two logic is common among different resource allocation
> technologies.
>
> Signed-off-by: Bing Niu <bing.niu at intel.com>
> ---
> src/conf/domain_conf.c | 195 +++++++++++++++++++++++++++++++++----------------
> 1 file changed, 131 insertions(+), 64 deletions(-)
>
You could make 3 patches out of this - one for each reduction of
virDomainCachetuneDefParse...
The virDomainFindResctrlAlloc should have used Restune instead of
ResctrlAlloc, right? Of course that changes based on how the naming
patch shakes out. Actually I think perhaps virDomainRestuneVcpuMatch
would be even better (where Restune ends up being whatever shakes out of
the previous patch naming decision).
In this area, I'm wondering what purpose does @new_alloc serve? If
@alloc was already defined, then you added an error message. So the
reality is - @new_alloc is always true.
Secondary to that you've added a new error related to identical vcpus in
the parsing logic. Unfortunately that could make a domain disappear on a
libvirtd restart if for some reason it was already defined in that
manner. If there's a parsing error because two entries had identical
cpus, then there will be an error. Errors would cause a previously
OK/found domain to not be defined. So that type of check (now that the
code has been around for more than a release) would need to go in some
sort of Validation API. This is one of those libvirt define and libvirtd
restart quirks.
Finally, virDomainUpdateRestune should be virDomainRestuneUpdate since
you started with virDomainRestuneParseVcpus and it should go after
virDomainCachetuneDefParseCache
Hope this all makes sense!
John
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 24fefd1..695981c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18884,6 +18884,115 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
>
>
> static int
> +virDomainRestuneParseVcpus(virDomainDefPtr def,
> + xmlNodePtr node,
> + virBitmapPtr *vcpus)
> +{
> + char *vcpus_str = NULL;
> + int ret = -1;
> +
> + vcpus_str = virXMLPropString(node, "vcpus");
> + if (!vcpus_str) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Missing cachetune attribute 'vcpus'"));
> + goto cleanup;
> + }
> + if (virBitmapParse(vcpus_str, vcpus, VIR_DOMAIN_CPUMASK_LEN) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid cachetune attribute 'vcpus' value '%s'"),
> + vcpus_str);
> + goto cleanup;
> + }
> +
> + /* We need to limit the bitmap to number of vCPUs. If there's nothing left,
> + * then we can just clean up and return 0 immediately */
> + virBitmapShrink(*vcpus, def->maxvcpus);
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(vcpus_str);
> + return ret;
> +}
> +
> +
> +static int
> +virDomainFindResctrlAlloc(virDomainDefPtr def,
> + virBitmapPtr vcpus,
> + virResctrlAllocPtr *alloc)
> +{
> + ssize_t i = 0;
> +
> + for (i = 0; i < def->nrestunes; i++) {
> + /* vcpus group has been created, directly use the existing one.
> + * Just updating memory allocation information of that group
> + */
> + if (virBitmapEqual(def->restunes[i]->vcpus, vcpus)) {
> + *alloc = def->restunes[i]->alloc;
> + break;
> + }
> + if (virBitmapOverlaps(def->restunes[i]->vcpus, vcpus)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Overlapping vcpus in restunes"));
> + return -1;
> + }
> + }
> + return 0;
> +}
> +
> +
> +static int
> +virDomainUpdateRestune(virDomainDefPtr def,
> + xmlNodePtr node,
> + virResctrlAllocPtr alloc,
> + virBitmapPtr vcpus,
> + unsigned int flags)
> +{
> + char *vcpus_str = NULL;
> + char *alloc_id = NULL;
> + virDomainRestuneDefPtr tmp_restune = NULL;
> + int ret = -1;
> +
> + if (VIR_ALLOC(tmp_restune) < 0)
> + goto cleanup;
> +
> + /* We need to format it back because we need to be consistent in the naming
> + * even when users specify some "sub-optimal" string there. */
> + vcpus_str = virBitmapFormat(vcpus);
> + if (!vcpus_str)
> + goto cleanup;
> +
> + if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
> + alloc_id = virXMLPropString(node, "id");
> +
> + if (!alloc_id) {
> + /* The number of allocations is limited and the directory structure is flat,
> + * not hierarchical, so we need to have all same allocations in one
> + * directory, so it's nice to have it named appropriately. For now it's
> + * 'vcpus_...' but it's designed in order for it to be changeable in the
> + * future (it's part of the status XML). */
> + if (virAsprintf(&alloc_id, "vcpus_%s", vcpus_str) < 0)
> + goto cleanup;
> + }
> +
> + if (virResctrlAllocSetID(alloc, alloc_id) < 0)
> + goto cleanup;
> +
> + tmp_restune->vcpus = vcpus;
> + tmp_restune->alloc = alloc;
> +
> + if (VIR_APPEND_ELEMENT(def->restunes, def->nrestunes, tmp_restune) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + virDomainRestuneDefFree(tmp_restune);
> + VIR_FREE(alloc_id);
> + VIR_FREE(vcpus_str);
> + return ret;
> +}
> +
> +
> +static int
> virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
> xmlNodePtr node,
> virResctrlAllocPtr alloc)
> @@ -18966,39 +19075,16 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
> xmlNodePtr oldnode = ctxt->node;
> xmlNodePtr *nodes = NULL;
> virBitmapPtr vcpus = NULL;
> - virResctrlAllocPtr alloc = virResctrlAllocNew();
> - virDomainRestuneDefPtr tmp_restune = NULL;
> - char *tmp = NULL;
> - char *vcpus_str = NULL;
> - char *alloc_id = NULL;
> + virResctrlAllocPtr alloc = NULL;
> ssize_t i = 0;
> int n;
> int ret = -1;
> + bool new_alloc = false;
>
> ctxt->node = node;
>
> - if (!alloc)
> - goto cleanup;
> -
> - if (VIR_ALLOC(tmp_restune) < 0)
> - goto cleanup;
> -
> - vcpus_str = virXMLPropString(node, "vcpus");
> - if (!vcpus_str) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("Missing cachetune attribute 'vcpus'"));
> - goto cleanup;
> - }
> - if (virBitmapParse(vcpus_str, &vcpus, VIR_DOMAIN_CPUMASK_LEN) < 0) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Invalid cachetune attribute 'vcpus' value '%s'"),
> - vcpus_str);
> + if (virDomainRestuneParseVcpus(def, node, &vcpus) < 0)
> goto cleanup;
> - }
> -
> - /* We need to limit the bitmap to number of vCPUs. If there's nothing left,
> - * then we can just clean up and return 0 immediately */
> - virBitmapShrink(vcpus, def->maxvcpus);
>
> if (virBitmapIsAllClear(vcpus)) {
> ret = 0;
> @@ -19011,63 +19097,44 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
> goto cleanup;
> }
>
> - for (i = 0; i < n; i++) {
> - if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0)
> + if (virDomainFindResctrlAlloc(def, vcpus, &alloc) < 0)
> + goto cleanup;
> +
> + if (!alloc) {
> + alloc = virResctrlAllocNew();
> + if (!alloc)
> goto cleanup;
> - }
> + new_alloc = true;
> + } else {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Identical vcpus in cachetunes found"));
>
> - if (virResctrlAllocIsEmpty(alloc)) {
> - ret = 0;
> goto cleanup;
> }
>
> - for (i = 0; i < def->nrestunes; i++) {
> - if (virBitmapOverlaps(def->restunes[i]->vcpus, vcpus)) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("Overlapping vcpus in cachetunes"));
> + for (i = 0; i < n; i++) {
> + if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0)
> goto cleanup;
> - }
> }
>
> - /* We need to format it back because we need to be consistent in the naming
> - * even when users specify some "sub-optimal" string there. */
> - VIR_FREE(vcpus_str);
> - vcpus_str = virBitmapFormat(vcpus);
> - if (!vcpus_str)
> + if (virResctrlAllocIsEmpty(alloc)) {
> + ret = 0;
> goto cleanup;
> + }
>
> - if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
> - alloc_id = virXMLPropString(node, "id");
> -
> - if (!alloc_id) {
> - /* The number of allocations is limited and the directory structure is flat,
> - * not hierarchical, so we need to have all same allocations in one
> - * directory, so it's nice to have it named appropriately. For now it's
> - * 'vcpus_...' but it's designed in order for it to be changeable in the
> - * future (it's part of the status XML). */
> - if (virAsprintf(&alloc_id, "vcpus_%s", vcpus_str) < 0)
> + if (new_alloc) {
> + if (virDomainUpdateRestune(def, node, alloc, vcpus, flags) < 0)
> goto cleanup;
> + vcpus = NULL;
> + alloc = NULL;
> }
>
> - if (virResctrlAllocSetID(alloc, alloc_id) < 0)
> - goto cleanup;
> -
> - VIR_STEAL_PTR(tmp_restune->vcpus, vcpus);
> - VIR_STEAL_PTR(tmp_restune->alloc, alloc);
> -
> - if (VIR_APPEND_ELEMENT(def->restunes, def->nrestunes, tmp_restune) < 0)
> - goto cleanup;
> -
> ret = 0;
> cleanup:
> ctxt->node = oldnode;
> - virDomainRestuneDefFree(tmp_restune);
> virObjectUnref(alloc);
> virBitmapFree(vcpus);
> - VIR_FREE(alloc_id);
> - VIR_FREE(vcpus_str);
> VIR_FREE(nodes);
> - VIR_FREE(tmp);
> return ret;
> }
>
>
More information about the libvir-list
mailing list