2:43 a.m.
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;
>>> + }
[...]