[libvirt] [PATCH v2 09/12] Add host cache information into capabilities

[Erik Skultety]

On Wed, Apr 05, 2017 at 04:36:32PM +0200, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
> I'm adding only info about L3 caches, we can add more later, but for
> now that's more than enough.

I think it's worth putting this into the commit message, along with a snippet
introducing the XML update.

[...]

> +static int
> +virCapabilitiesFormatCaches(virBufferPtr buf,
> +                            size_t ncaches,
> +                            virCapsHostCacheBankPtr *caches)
> +{
> +    size_t i = 0;
> +
> +    if (!ncaches)
> +        return 0;
> +
> +    virBufferAddLit(buf, "<cache>\n");
> +    virBufferAdjustIndent(buf, 2);
> +
> +    for (i = 0; i < ncaches; i++) {
> +        virCapsHostCacheBankPtr bank = caches[i];
> +        char *cpus_str = virBitmapFormat(bank->cpus);
> +        bool kilos = !(bank->size % 1024);

I'd suggest kibs/kibis/kibi but not kilos.

> +
> +int
> +virCapabilitiesInitCaches(virCapsPtr caps)
> +{
> +    size_t i = 0;
> +    virBitmapPtr cpus = NULL;
> +    ssize_t pos = -1;
> +    DIR *dirp = NULL;
> +    int ret = -1;
> +    char *path = NULL;
> +    char *type = NULL;
> +    struct dirent *ent = NULL;
> +    virCapsHostCacheBankPtr bank = NULL;
> +
> +    /* Minimum level to expose in capabilities.  Can be lowered or removed (with
> +     * the appropriate code below), but should not be increased, because we'd
> +     * lose information. */
> +    const int cache_min_level = 3;
> +
> +    /* offline CPUs don't provide cache info */
> +    if (virFileReadValueBitmap(&cpus, SYSFS_SYSTEM_PATH "cpu/online") < 0)
> +        return -1;
> +
> +    while ((pos = virBitmapNextSetBit(cpus, pos)) >= 0) {
> +        int rv = -1;
> +
> +        VIR_FREE(path);
> +        if (virAsprintf(&path, SYSFS_SYSTEM_PATH "cpu/cpu%zd/cache/", pos) < 0)
> +            goto cleanup;
> +
> +        rv = virDirOpenIfExists(&dirp, path);
> +        if (rv < 0)
> +            goto cleanup;
> +
> +        if (!dirp)
> +            continue;
> +
> +        while ((rv = virDirRead(dirp, &ent, path)) > 0) {
> +            int tmp_i;
> +            char *tmp_c;
> +
> +            if (!STRPREFIX(ent->d_name, "index"))
> +                continue;
> +
> +            if (VIR_ALLOC(bank) < 0)
> +                goto cleanup;
> +
> +            if (virFileReadValueUint(&bank->id,
> +                                     SYSFS_SYSTEM_PATH "cpu/cpu%zd/cache/%s/id",
> +                                     pos, ent->d_name) < 0)
> +                goto cleanup;
> +
> +            if (virFileReadValueUint(&bank->level,
> +                                     SYSFS_SYSTEM_PATH
> +                                     "cpu/cpu%zd/cache/%s/level",
> +                                     pos, ent->d_name) < 0)
> +                goto cleanup;
> +
> +            if (virFileReadValueString(&type,
> +                                       SYSFS_SYSTEM_PATH
> +                                       "cpu/cpu%zd/cache/%s/type",
> +                                       pos, ent->d_name) < 0)
> +                goto cleanup;
> +
> +            if (virFileReadValueScaledInt(&bank->size,
> +                                          SYSFS_SYSTEM_PATH
> +                                          "cpu/cpu%zd/cache/%s/size",
> +                                          pos, ent->d_name) < 0)
> +                goto cleanup;
> +
> +            if (virFileReadValueBitmap(&bank->cpus,
> +                                       SYSFS_SYSTEM_PATH
> +                                       "cpu/cpu%zd/cache/%s/shared_cpu_list",
> +                                       pos, ent->d_name) < 0)
> +                goto cleanup;
> +
> +            if (bank->level < cache_min_level) {
> +                virCapsHostCacheBankFree(bank);
> +                bank = NULL;
> +                continue;
> +            }

I'd say, make the bank->level read first, then perform ^this check and then
continue with the rest of the checks...you know, to save a few cycles...

> +
> +            for (tmp_c = type; *tmp_c != '\0'; tmp_c++)
> +                *tmp_c = c_tolower(*tmp_c);
> +
> +            tmp_i = virCacheTypeFromString(type);
> +            if (tmp_i < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Unknown cache type '%s'"), type);
> +                VIR_FREE(type);
> +                goto cleanup;
> +            }

I'd maybe add a small static function handling ^this hunk (or add a symmetric
virStringToLower method), but that's a bikeshed for another day.

> +            bank->type = tmp_i;
> +
> +            for (i = 0; i < caps->host.ncaches; i++) {
> +                if (virCapsHostCacheBankEquals(bank, caps->host.caches[i]))
> +                    break;
> +            }
> +            if (i == caps->host.ncaches) {
> +                if (VIR_APPEND_ELEMENT(caps->host.caches,
> +                                       caps->host.ncaches,
> +                                       bank) < 0) {
> +                    goto cleanup;
> +                }
> +            }

I'd wrap ^this here as well, a small boolean (possibly inline) method called
virCapsHostCacheBankExists, if you get false, then add it to the list, but feel
free to disagree with me on this.

ACK if you enhance the commit message.

Erik