Re: [libvirt] [PATCHv5 09/19] util: Add more interfaces for resctrl monitor
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, ) < 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
*
Re: [libvirt] [PATCHv5 09/19] util: Add more interfaces for resctrl monitor
[...]
>>> 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, ) < 0)
>>> +goto cleanup;
>>> +
>>> +rv = virFileReadValueUint(, "%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,
Re: [libvirt] [PATCHv5 09/19] util: Add more interfaces for resctrl monitor
On 10/11/2018 3:13 AM, John Ferlan wrote:
>
>
> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
>> Add interfaces monitor group to support operations such
>> as add PID, set ID, remove group ... etc.
>>
>> The interface for getting cache occupancy information
>> from the monitor is also added.
>>
>> Signed-off-by: Wang Huaqiang
>> ---
>> src/libvirt_private.syms | 6 ++
>> src/util/virresctrl.c| 209
>> ++-
>> src/util/virresctrl.h| 23 ++
>> 3 files changed, 236 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index a878083..c93d19f 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2683,7 +2683,13 @@ virResctrlInfoNew;
>> virResctrlMonitorAddPID;
>> virResctrlMonitorCreate;
>> virResctrlMonitorDeterminePath;
>> +virResctrlMonitorGetCacheLevel;
>> +virResctrlMonitorGetCacheOccupancy;
>> +virResctrlMonitorGetID;
>> virResctrlMonitorNew;
>> +virResctrlMonitorRemove;
>> +virResctrlMonitorSetCacheLevel;
>> +virResctrlMonitorSetID;
>>
>>
>> # util/virrotatingfile.h
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index b3d20cc..fca1f6f 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -225,11 +225,19 @@ virResctrlInfoMonFree(virResctrlInfoMonPtr mon)
>> }
>>
>>
>> +
>> +/*
>> + * virResctrlAlloc and virResctrlMonitor are representing a resource control
>> + * group (in XML under cputune/cachetune and consequently a directory under
>> + * /sys/fs/resctrl). virResctrlAlloc is the data structure for resource
>> + * allocation, while the virResctrlMonitor represents the resource
>> monitoring
>> + * part.
>> + */
>> +
>> /* virResctrlAlloc */
>>
>> /*
>> - * virResctrlAlloc represents one allocation (in XML under
>> cputune/cachetune and
>> - * consequently a directory under /sys/fs/resctrl). Since it can have
>> multiple
>> + * virResctrlAlloc represents one allocation. Since it can have multiple
>
> I would think that perhaps the comments changing here would go earlier
> when virResctrlMonitor was introduced. The comment with the single
> virResctrlAlloc could be changed to "virResctrlAlloc and
> virResctrlMonitor", then merge in what you've typed above.
>
>> * parts of multiple caches allocated it is represented as bunch of nested
>> * sparse arrays (by sparse I mean array of pointers so that each might be
>> NULL
>> * in case there is no allocation for that particular cache allocation
>> (level,
>> @@ -347,6 +355,8 @@ struct _virResctrlMonitor {
>> /* libvirt-generated path in /sys/fs/resctrl for this particular
>> * monitor */
>> char *path;
>> +/* The cache 'level', special for cache monitor */
>> +unsigned int cache_level;
>> };
>>
>>
>> @@ -2510,6 +2520,27 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr
>> monitor,
>>
>>
>> int
>> +virResctrlMonitorSetID(virResctrlMonitorPtr monitor,
>> + const char *id)
>> +{
>> +if (!id) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Resctrl monitor 'id' cannot be NULL"));
>> +return -1;
>> +}
>> +
>> +return VIR_STRDUP(monitor->id, id);
>> +}
>> +
>> +
>> +const char *
>> +virResctrlMonitorGetID(virResctrlMonitorPtr monitor)
>> +{
>> +return monitor->id;
>> +}
>> +
>> +
>> +int
>> 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.
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.
>
>
>> +}
>> +
>> +return ret;
>> +}
>> +
>> +
>> +int
>> +virResctrlMonitorSetCacheLevel(virResctrlMonitorPtr monitor,
>> +
Re: [libvirt] [PATCHv5 09/19] util: Add more interfaces for resctrl monitor
On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> Add interfaces monitor group to support operations such
> as add PID, set ID, remove group ... etc.
>
> The interface for getting cache occupancy information
> from the monitor is also added.
>
> Signed-off-by: Wang Huaqiang
> ---
> src/libvirt_private.syms | 6 ++
> src/util/virresctrl.c| 209
> ++-
> src/util/virresctrl.h| 23 ++
> 3 files changed, 236 insertions(+), 2 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a878083..c93d19f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2683,7 +2683,13 @@ virResctrlInfoNew;
> virResctrlMonitorAddPID;
> virResctrlMonitorCreate;
> virResctrlMonitorDeterminePath;
> +virResctrlMonitorGetCacheLevel;
> +virResctrlMonitorGetCacheOccupancy;
> +virResctrlMonitorGetID;
> virResctrlMonitorNew;
> +virResctrlMonitorRemove;
> +virResctrlMonitorSetCacheLevel;
> +virResctrlMonitorSetID;
>
>
> # util/virrotatingfile.h
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index b3d20cc..fca1f6f 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -225,11 +225,19 @@ virResctrlInfoMonFree(virResctrlInfoMonPtr mon)
> }
>
>
> +
> +/*
> + * virResctrlAlloc and virResctrlMonitor are representing a resource control
> + * group (in XML under cputune/cachetune and consequently a directory under
> + * /sys/fs/resctrl). virResctrlAlloc is the data structure for resource
> + * allocation, while the virResctrlMonitor represents the resource monitoring
> + * part.
> + */
> +
> /* virResctrlAlloc */
>
> /*
> - * virResctrlAlloc represents one allocation (in XML under cputune/cachetune
> and
> - * consequently a directory under /sys/fs/resctrl). Since it can have
> multiple
> + * virResctrlAlloc represents one allocation. Since it can have multiple
I would think that perhaps the comments changing here would go earlier
when virResctrlMonitor was introduced. The comment with the single
virResctrlAlloc could be changed to "virResctrlAlloc and
virResctrlMonitor", then merge in what you've typed above.
> * parts of multiple caches allocated it is represented as bunch of nested
> * sparse arrays (by sparse I mean array of pointers so that each might be
> NULL
> * in case there is no allocation for that particular cache allocation
> (level,
> @@ -347,6 +355,8 @@ struct _virResctrlMonitor {
> /* libvirt-generated path in /sys/fs/resctrl for this particular
> * monitor */
> char *path;
> +/* The cache 'level', special for cache monitor */
> +unsigned int cache_level;
> };
>
>
> @@ -2510,6 +2520,27 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr
> monitor,
>
>
> int
> +virResctrlMonitorSetID(virResctrlMonitorPtr monitor,
> + const char *id)
> +{
> +if (!id) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Resctrl monitor 'id' cannot be NULL"));
> +return -1;
> +}
> +
> +return VIR_STRDUP(monitor->id, id);
> +}
> +
> +
> +const char *
> +virResctrlMonitorGetID(virResctrlMonitorPtr monitor)
> +{
> +return monitor->id;
> +}
> +
> +
> +int
> 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...
> +virResctrlMonitorRemove(virResctrlMonitorPtr monitor)
> +{
> +int ret = 0;
> +
So unlike Alloc, @monitor cannot be NULL...
> +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.
> +}
> +
> +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
[libvirt] [PATCHv5 09/19] util: Add more interfaces for resctrl monitor
Add interfaces monitor group to support operations such
as add PID, set ID, remove group ... etc.
The interface for getting cache occupancy information
from the monitor is also added.
Signed-off-by: Wang Huaqiang
---
src/libvirt_private.syms | 6 ++
src/util/virresctrl.c| 209 ++-
src/util/virresctrl.h| 23 ++
3 files changed, 236 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a878083..c93d19f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2683,7 +2683,13 @@ virResctrlInfoNew;
virResctrlMonitorAddPID;
virResctrlMonitorCreate;
virResctrlMonitorDeterminePath;
+virResctrlMonitorGetCacheLevel;
+virResctrlMonitorGetCacheOccupancy;
+virResctrlMonitorGetID;
virResctrlMonitorNew;
+virResctrlMonitorRemove;
+virResctrlMonitorSetCacheLevel;
+virResctrlMonitorSetID;
# util/virrotatingfile.h
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index b3d20cc..fca1f6f 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -225,11 +225,19 @@ virResctrlInfoMonFree(virResctrlInfoMonPtr mon)
}
+
+/*
+ * virResctrlAlloc and virResctrlMonitor are representing a resource control
+ * group (in XML under cputune/cachetune and consequently a directory under
+ * /sys/fs/resctrl). virResctrlAlloc is the data structure for resource
+ * allocation, while the virResctrlMonitor represents the resource monitoring
+ * part.
+ */
+
/* virResctrlAlloc */
/*
- * virResctrlAlloc represents one allocation (in XML under cputune/cachetune
and
- * consequently a directory under /sys/fs/resctrl). Since it can have multiple
+ * virResctrlAlloc represents one allocation. Since it can have multiple
* parts of multiple caches allocated it is represented as bunch of nested
* sparse arrays (by sparse I mean array of pointers so that each might be NULL
* in case there is no allocation for that particular cache allocation (level,
@@ -347,6 +355,8 @@ struct _virResctrlMonitor {
/* libvirt-generated path in /sys/fs/resctrl for this particular
* monitor */
char *path;
+/* The cache 'level', special for cache monitor */
+unsigned int cache_level;
};
@@ -2510,6 +2520,27 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr
monitor,
int
+virResctrlMonitorSetID(virResctrlMonitorPtr monitor,
+ const char *id)
+{
+if (!id) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Resctrl monitor 'id' cannot be NULL"));
+return -1;
+}
+
+return VIR_STRDUP(monitor->id, id);
+}
+
+
+const char *
+virResctrlMonitorGetID(virResctrlMonitorPtr monitor)
+{
+return monitor->id;
+}
+
+
+int
virResctrlMonitorCreate(virResctrlAllocPtr alloc,
virResctrlMonitorPtr monitor,
const char *machinename)
@@ -2534,3 +2565,177 @@ virResctrlMonitorCreate(virResctrlAllocPtr alloc,
virResctrlUnlock(lockfd);
return ret;
}
+
+
+int
+virResctrlMonitorRemove(virResctrlMonitorPtr monitor)
+{
+int ret = 0;
+
+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);
+}
+
+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;
+}
+
+
+/*
+ * virResctrlMonitorGetStatistic
+ *
+ * @monitor: The monitor that the statistic data will be retrieved from.
+ * @resource: The name for resource name. 'llc_occpancy' for cache resource.
+ * "mbm_totol_bytes" and "mbm_local_bytes" for memory bandwidth resource.
+ * @len: The array length for @ids, and @vals
+ * @ids: The id array for resource statistic information, ids[0]
+ * stores the first node id value, ids[1] stores the second node id value,
+ * ... and so on.
+ * @vals: The resource resource utilization information array. vals[0]
+ * stores the cache or memory bandwidth utilization value for first node,
+ * vals[1] stores the second value ... and so on.
+ *
+ * Get cache or memory bandwidth utilization information from monitor that
+ * specified by @id.
+ *
+ * Returns 0 for success, -1 for error.
+ */
+static int
+virResctrlMonitorGetStatistic(virResctrlMonitorPtr monitor,
+
