[libvirt] [PATCHv5 09/19] util: Add more interfaces for resctrl monitor

[John Ferlan]

[...]

>>>  virResctrlMonitorCreate(virResctrlAllocPtr alloc,
>>>                          virResctrlMonitorPtr monitor,
>>>                          const char *machinename)
>>> @@ -2534,3 +2565,177 @@ virResctrlMonitorCreate(virResctrlAllocPtr alloc,
>>>      virResctrlUnlock(lockfd);
>>>      return ret;
>>>  }
>>> +
>>> +
>>> +int
>>
>> The eventual only caller never checks status...
> 
> That's true, and I noticed that. The back-end logic is copied from
> virResctrlAllocRemove.

Understood, but that doesn't check status either. Maybe it needs to
change to a void since it uses VIR_ERROR.

> 
> Maybe with a change of removing the following
> virReportSystemError lines.
> 
>>
>>> +virResctrlMonitorRemove(virResctrlMonitorPtr monitor)
>>> +{
>>> +    int ret = 0;
>>> +
>>
>> So unlike Alloc, @monitor cannot be NULL...
> 
> @monitor is not allowed to be NULL. This is guaranteed by the caller.
> 
>>
>>> +    if (!monitor->path)
>>> +        return 0;
>>> +
>>> +    VIR_DEBUG("Removing resctrl monitor%s", monitor->path);
>>> +    if (rmdir(monitor->path) != 0 && errno != ENOENT) {
>>> +        virReportSystemError(errno,
>>> +                             _("Unable to remove %s (%d)"),
>>> +                             monitor->path, errno);
>>> +        ret = -errno;
>>> +        VIR_ERROR(_("Unable to remove %s (%d)"), monitor->path, errno);
>>
>> Either virReportSystemError if you're going to handle the returned
>> status or VIR_ERROR if you're not (and are just ignoring), but not both.
> 
> I would like to remove the virReportSystemError lines and keep the VIR_ERROR line.
> 

I can agree to that along with it being void since it's a best effort.

>>
>>
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +
>>> +int
>>> +virResctrlMonitorSetCacheLevel(virResctrlMonitorPtr monitor,
>>> +                               unsigned int level)
>>> +{
>>> +    /* Only supports cache level 3 CMT */
>>> +    if (level != 3) {
>>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> +                       _("Invalid resctrl monitor cache level"));
>>> +        return -1;
>>> +    }
>>> +
>>> +    monitor->cache_level = level;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +unsigned int
>>> +virResctrlMonitorGetCacheLevel(virResctrlMonitorPtr monitor)
>>> +{
>>> +    return monitor->cache_level;
>>> +}
>>
>> Based on usage, maybe we should just give up on API's like this. Create
>> a VIR_RESCTRL_MONITOR_CACHE_LEVEL (or something like it)... Then use it
>> at least for now when reading/supplying the level. Thus we leave it to
>> future developer to create the API's and handle the new levels...
>>
>> If when we Parse we don't find the constant, then error.
> 
> Do you mean removing the 'virResctrlMonitorSetCacheLevel'
> and 'virResctrlMonitorGetCacheLevel' two functions from here, and processing
> the monitor cache level information directory in domain_conf.c during XML
> parsing and format process.
> 

Yeah, I think I've come to that conclusion. In the Parse code you'd
still parse the value, but then compare against the constant.  In the
Format code, you can just format what you have. Whomever creates a
different level in the future can have the fun of managing range and of
course managing if whatever is being fetched is in the particular cache
level.

>>
>>> +
>>> +
>>> +/*
>>> + * virResctrlMonitorGetStatistic

[...]

>>> +        str_id = strchr(++str_id, '_');
>>> +        if (!str_id)
>>> +            continue;
>>
>> I think all of the above could be replaced by:
>>
>>     if (!(str_id = STRSKIP(ent->d_name, "mon_L")))
>>         continue;
>>
>> I personally am not a fan of pre-auto-incr. I get particularly
>> uncomfortable when it involves pointer arithmetic... But I think it's
>> unnecessary if you use STRSKIP
> 
> The interesting target directory name is like 'MON_L3_00' or 'MON_L3CODE_00'
> if CDP is enabled. The last two numbers after second '_' character is the ID
> number, now we need to locate the second '_' character.
> One STRSKIP is not enough, still need a call of strchr to locate the second '_' in
> following way:
> 
>     if (!(str_id = STRSKIP(ent->d_name, "mon_L")))
>         continue;
> 
>      str_id = strchr(str_id , '_');
>      if (!str_id)
>           continue;

So MON_L3DATA_00 would do the same as would MON_L3FUTURE_00?

Then let's STRSKIP(str_id, "_") - IOW: Skip to the next

The first STRSKIP is to get to the "level"#, right? The second to the
"id"#. Maybe the variables should be named that way to make it clear
along with some comments.

> 
>>
>>
>>> +
>>> +        if (virStrToLong_uip(++str_id, NULL, 0, &id) < 0)
>>> +            goto cleanup;
>>> +
>>> +        rv = virFileReadValueUint(&val, "%s/%s/%s", datapath,
>>> +                                  ent->d_name, resource);
>>> +        if (rv == -2) {
>>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                           _("File '%s/%s/%s' does not exist."),
>>> +                           datapath, ent->d_name, resource);
>>> +        }
>>> +        if (rv < 0)
>>> +            goto cleanup;
>>> +
>>> +        if (VIR_APPEND_ELEMENT(*ids, nids, id) < 0)
>>> +            goto cleanup;
>>> +
>>> +        if (VIR_APPEND_ELEMENT(*vals, nvals, val) < 0)
>>> +            goto cleanup;
>>
>> If you had a single structure for id and val, then...
> How about:
> 
> struct _virResctrlMonitorStats {
>     unsigned int id;
>     unsigned int val;
> };
> 

Your call how you do it, but that self created sort (below) is more what
is objectionable.  I remember peeking quickly - there are plenty of
qsort examples to choose from in libvirt sources.

John

>>
>>
>>> +
>>> +        /* Sort @ids and @vals arrays in the ascending order of id */
>>> +        cur_id_pos = nids - 1;
>>> +        for (i = 0; i < cur_id_pos; i++) {
>>> +            if ((*ids)[cur_id_pos] < (*ids)[i]) {
>>> +                tmp_id = (*ids)[cur_id_pos];
>>> +                tmp_val = (*vals)[cur_id_pos];
>>> +                (*ids)[cur_id_pos] = (*ids)[i];
>>> +                (*vals)[cur_id_pos] = (*vals)[i];
>>> +                (*ids)[i] = tmp_id;
>>> +                (*vals)[i] = tmp_val;
>>> +            }

[...]