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

[Wang, Huaqiang]

On 10/12/2018 10:40 PM, John Ferlan wrote:
> [...]
>
>>>>   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?
Yes. Here I am only interested in the last two digital numbers after 
second character '_', which
is the node id.

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

Do you mean there steps in total?

     if (!(str_id = STRSKIP(ent->d_name, "mon_L")))
         continue;

      str_id = strchr(str_id , '_');
      if (!str_id)
           continue;

      if (!(str_id = STRSKIP(str_id, "_")))/* instead of STRSKIP(ent->d_name, '_') */
         continue;

      if (virStrToLong_uip(str_id, NULL, 0, &id) < 0)
           goto cleanup;

> The first STRSKIP is to get to the "level"#, right?

Yes.  But I skipped the verify for cache level, we only support L3 cache 
monitoring.

>   The second to the
> "id"#. Maybe the variables should be named that way to make it clear
> along with some comments.

Good suggestion.
I'll directly use 'node_id' for the name, and code would look like these:

           /* Looking for directory that contains the resource utilization
            * information file. The directory name is arranged in format
            * "mon_<node_name>_<node_id>". For example, "mon_L3_00" and
            * "mon_L3_01" are two target directories for a two nodes system
            * with resource utilization data file for each node 
respectively.
            */
           if (ent->d_type != DT_DIR)
continue;

           /* Looking for directory has a prefix 'mon_L' */
           if (!(node_id = STRSKIP(end->d_name, "mon_L")))
               continue;

           /* Looking for directory has another '_' */
           node_id = strchr(node_id, '_');
           if (!node_id)
               continue;

           /* Skip the character '_' */
           if (!(node_id = STRSKIP(node_id, "_")))
               continue;

           /* The node ID number should be here, parsing it. */
           if (virStrToLong_uip(node_id, NULL, 0, &id) < 0)
               goto cleanup;


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

I'll using the _virResctrlMonitorStats structure, and sorting the output 
with qsort as you suggested.

>
> John

Thanks for review.
Huaqiang
>>>
>>>> +
>>>> +        /* 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;
>>>> +            }
> [...]