Export virResctrlMonitorGetStats and make
virResctrlMonitorGetCacheOccupancy obsoleted.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++--------
src/util/virresctrl.c | 46 +++++++++++++++++++++++++++-------------------
src/util/virresctrl.h | 6 ++++++
4 files changed, 59 insertions(+), 27 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9099757..2e3d48c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2771,6 +2771,7 @@ virResctrlMonitorDeterminePath;
virResctrlMonitorFreeStats;
virResctrlMonitorGetCacheOccupancy;
virResctrlMonitorGetID;
+virResctrlMonitorGetStats;
virResctrlMonitorNew;
virResctrlMonitorRemove;
virResctrlMonitorSetAlloc;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4ea346c..bc19171 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19987,6 +19987,7 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
/**
* qemuDomainGetResctrlMonData:
+ * @driver: Pointer to qemu driver
* @dom: Pointer for the domain that the resctrl monitors reside in
* @resdata: Pointer of virQEMUResctrlMonDataPtr pointer for receiving the
* virQEMUResctrlMonDataPtr array. Caller is responsible for
@@ -20005,16 +20006,29 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
* Returns -1 on failure, or 0 on success.
*/
static int
-qemuDomainGetResctrlMonData(virDomainObjPtr dom,
+qemuDomainGetResctrlMonData(virQEMUDriverPtr driver,
+ virDomainObjPtr dom,
virQEMUResctrlMonDataPtr **resdata,
size_t *nresdata,
virResctrlMonitorType tag)
{
virDomainResctrlDefPtr resctrl = NULL;
virQEMUResctrlMonDataPtr res = NULL;
+ char **features = NULL;
size_t i = 0;
size_t j = 0;
+ if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
+ features = driver->caps->host.cache.monitor->features;
+ } else {
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unsupported resctrl monitor type"));
+ return -1;
+ }
+
+ if (virStringListLength((const char * const *)features) == 0)
+ return 0;
+
for (i = 0; i < dom->def->nresctrls; i++) {
resctrl = dom->def->resctrls[i];
@@ -20041,9 +20055,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom,
if (VIR_STRDUP(res->name, virResctrlMonitorGetID(monitor)) < 0)
goto error;
- if (virResctrlMonitorGetCacheOccupancy(monitor,
- &res->stats,
- &res->nstats) < 0)
+ if (virResctrlMonitorGetStats(monitor, (const char **)features,
+ &res->stats, &res->nstats) < 0)
goto error;
if (VIR_APPEND_ELEMENT(*resdata, *nresdata, res) < 0)
@@ -20060,7 +20073,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom,
static int
-qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
+qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver,
+ virDomainObjPtr dom,
virDomainStatsRecordPtr record,
int *maxparams)
{
@@ -20074,10 +20088,13 @@ qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
if (!virDomainObjIsActive(dom))
return 0;
- if (qemuDomainGetResctrlMonData(dom, &resdata, &nresdata,
+ if (qemuDomainGetResctrlMonData(driver, dom, &resdata, &nresdata,
VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0)
goto cleanup;
+ if (nresdata == 0)
+ return 0;
+
snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
"cpu.cache.monitor.count");
if (virTypedParamsAddUInt(&record->params, &record->nparams,
@@ -20175,7 +20192,7 @@ qemuDomainGetStatsCpuCgroup(virDomainObjPtr dom,
static int
-qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+qemuDomainGetStatsCpu(virQEMUDriverPtr driver,
virDomainObjPtr dom,
virDomainStatsRecordPtr record,
int *maxparams,
@@ -20184,7 +20201,7 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
if (qemuDomainGetStatsCpuCgroup(dom, record, maxparams) < 0)
return -1;
- if (qemuDomainGetStatsCpuCache(dom, record, maxparams) < 0)
+ if (qemuDomainGetStatsCpuCache(driver, dom, record, maxparams) < 0)
return -1;
return 0;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index c2fe2ed..76a8d02 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2668,8 +2668,7 @@ virResctrlMonitorStatsSorter(const void *a,
* virResctrlMonitorGetStats
*
* @monitor: The monitor that the statistic data will be retrieved from.
- * @resource: The name for resource name. 'llc_occupancy' for cache resource.
- * "mbm_total_bytes" and "mbm_local_bytes" for memory bandwidth resource.
+ * @resources: A string list for the monitor feature names.
* @stats: Pointer of of virResctrlMonitorStatsPtr array for holding cache or
* memory bandwidth usage data.
* @nstats: A size_t pointer to hold the returned array length of @stats
@@ -2678,14 +2677,15 @@ virResctrlMonitorStatsSorter(const void *a,
*
* Returns 0 on success, -1 on error.
*/
-static int
+int
virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
- const char *resource,
+ const char **resources,
virResctrlMonitorStatsPtr **stats,
size_t *nstats)
{
int rv = -1;
int ret = -1;
+ size_t i = 0;
unsigned int val = 0;
DIR *dirp = NULL;
char *datapath = NULL;
@@ -2743,21 +2743,23 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
if (virStrToLong_uip(node_id, NULL, 0, &stat->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;
+ for (i = 0; resources[i]; i++) {
+ rv = virFileReadValueUint(&val, "%s/%s/%s", datapath,
+ ent->d_name, resources[i]);
+ if (rv == -2) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("File '%s/%s/%s' does not exist."),
+ datapath, ent->d_name, resources[i]);
+ }
+ if (rv < 0)
+ goto cleanup;
- if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0)
- goto cleanup;
+ if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0)
+ goto cleanup;
- if (virStringListAdd(&stat->features, resource) < 0)
- goto cleanup;
+ if (virStringListAdd(&stat->features, resources[i]) < 0)
+ goto cleanup;
+ }
if (VIR_APPEND_ELEMENT(*stats, *nstats, stat) < 0)
goto cleanup;
@@ -2813,6 +2815,12 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
virResctrlMonitorStatsPtr **stats,
size_t *nstats)
{
- return virResctrlMonitorGetStats(monitor, "llc_occupancy",
- stats, nstats);
+ char **features = NULL;
+ int ret = -1;
+
+ virStringListAdd(&features, "llc_occupancy");
+ ret = virResctrlMonitorGetStats(monitor, (const char**)features, stats, nstats);
+ virStringListFree(features);
+
+ return ret;
}
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index 97205bc..d1905b0 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -236,6 +236,12 @@ int
virResctrlMonitorRemove(virResctrlMonitorPtr monitor);
int
+virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
+ const char **resources,
+ virResctrlMonitorStatsPtr **stats,
+ size_t *nstats);
+
+int
virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
virResctrlMonitorStatsPtr **stats,
size_t *nstats);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
> Export virResctrlMonitorGetStats and make
> virResctrlMonitorGetCacheOccupancy obsoleted.
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
> ---
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++--------
> src/util/virresctrl.c | 46 +++++++++++++++++++++++++++-------------------
> src/util/virresctrl.h | 6 ++++++
> 4 files changed, 59 insertions(+), 27 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 9099757..2e3d48c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2771,6 +2771,7 @@ virResctrlMonitorDeterminePath;
> virResctrlMonitorFreeStats;
> virResctrlMonitorGetCacheOccupancy;
> virResctrlMonitorGetID;
> +virResctrlMonitorGetStats;
> virResctrlMonitorNew;
> virResctrlMonitorRemove;
> virResctrlMonitorSetAlloc;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4ea346c..bc19171 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19987,6 +19987,7 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
>
> /**
> * qemuDomainGetResctrlMonData:
> + * @driver: Pointer to qemu driver
> * @dom: Pointer for the domain that the resctrl monitors reside in
> * @resdata: Pointer of virQEMUResctrlMonDataPtr pointer for receiving the
> * virQEMUResctrlMonDataPtr array. Caller is responsible for
> @@ -20005,16 +20006,29 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
> * Returns -1 on failure, or 0 on success.
> */
> static int
> -qemuDomainGetResctrlMonData(virDomainObjPtr dom,
> +qemuDomainGetResctrlMonData(virQEMUDriverPtr driver,
> + virDomainObjPtr dom,
> virQEMUResctrlMonDataPtr **resdata,
> size_t *nresdata,
> virResctrlMonitorType tag)
> {
> virDomainResctrlDefPtr resctrl = NULL;
> virQEMUResctrlMonDataPtr res = NULL;
> + char **features = NULL;
> size_t i = 0;
> size_t j = 0;
>
> + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
> + features = driver->caps->host.cache.monitor->features;
No, we have virQEMUDriverGetCapabilities() which ensures that
driver->caps object doesn't go away while accessing this.
> + } else {
> + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> + _("Unsupported resctrl monitor type"));
> + return -1;
> + }
> +
> + if (virStringListLength((const char * const *)features) == 0)
> + return 0;
> +
> for (i = 0; i < dom->def->nresctrls; i++) {
> resctrl = dom->def->resctrls[i];
>
> @@ -20041,9 +20055,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom,
> if (VIR_STRDUP(res->name, virResctrlMonitorGetID(monitor)) < 0)
> goto error;
>
> - if (virResctrlMonitorGetCacheOccupancy(monitor,
> - &res->stats,
> - &res->nstats) < 0)
> + if (virResctrlMonitorGetStats(monitor, (const char **)features,
> + &res->stats, &res->nstats) < 0)
> goto error;
>
> if (VIR_APPEND_ELEMENT(*resdata, *nresdata, res) < 0)
> @@ -20060,7 +20073,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom,
>
>
> static int
> -qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
> +qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver,
> + virDomainObjPtr dom,
> virDomainStatsRecordPtr record,
> int *maxparams)
> {
> @@ -20074,10 +20088,13 @@ qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
> if (!virDomainObjIsActive(dom))
> return 0;
>
> - if (qemuDomainGetResctrlMonData(dom, &resdata, &nresdata,
> + if (qemuDomainGetResctrlMonData(driver, dom, &resdata, &nresdata,
> VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0)
> goto cleanup;
>
> + if (nresdata == 0)
> + return 0;
> +
> snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> "cpu.cache.monitor.count");
> if (virTypedParamsAddUInt(&record->params, &record->nparams,
> @@ -20175,7 +20192,7 @@ qemuDomainGetStatsCpuCgroup(virDomainObjPtr dom,
>
>
> static int
> -qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> +qemuDomainGetStatsCpu(virQEMUDriverPtr driver,
> virDomainObjPtr dom,
> virDomainStatsRecordPtr record,
> int *maxparams,
> @@ -20184,7 +20201,7 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> if (qemuDomainGetStatsCpuCgroup(dom, record, maxparams) < 0)
> return -1;
>
> - if (qemuDomainGetStatsCpuCache(dom, record, maxparams) < 0)
> + if (qemuDomainGetStatsCpuCache(driver, dom, record, maxparams) < 0)
> return -1;
>
> return 0;
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index c2fe2ed..76a8d02 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -2668,8 +2668,7 @@ virResctrlMonitorStatsSorter(const void *a,
> * virResctrlMonitorGetStats
> *
> * @monitor: The monitor that the statistic data will be retrieved from.
> - * @resource: The name for resource name. 'llc_occupancy' for cache resource.
> - * "mbm_total_bytes" and "mbm_local_bytes" for memory bandwidth resource.
> + * @resources: A string list for the monitor feature names.
> * @stats: Pointer of of virResctrlMonitorStatsPtr array for holding cache or
> * memory bandwidth usage data.
> * @nstats: A size_t pointer to hold the returned array length of @stats
> @@ -2678,14 +2677,15 @@ virResctrlMonitorStatsSorter(const void *a,
> *
> * Returns 0 on success, -1 on error.
> */
> -static int
> +int
> virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
> - const char *resource,
> + const char **resources,
> virResctrlMonitorStatsPtr **stats,
> size_t *nstats)
> {
> int rv = -1;
> int ret = -1;
> + size_t i = 0;
> unsigned int val = 0;
> DIR *dirp = NULL;
> char *datapath = NULL;
> @@ -2743,21 +2743,23 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
> if (virStrToLong_uip(node_id, NULL, 0, &stat->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;
> + for (i = 0; resources[i]; i++) {
> + rv = virFileReadValueUint(&val, "%s/%s/%s", datapath,
> + ent->d_name, resources[i]);
> + if (rv == -2) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("File '%s/%s/%s' does not exist."),
> + datapath, ent->d_name, resources[i]);
> + }
> + if (rv < 0)
> + goto cleanup;
>
> - if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0)
> - goto cleanup;
> + if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0)
> + goto cleanup;
>
> - if (virStringListAdd(&stat->features, resource) < 0)
> - goto cleanup;
> + if (virStringListAdd(&stat->features, resources[i]) < 0)
> + goto cleanup;
> + }
>
> if (VIR_APPEND_ELEMENT(*stats, *nstats, stat) < 0)
> goto cleanup;
> @@ -2813,6 +2815,12 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
> virResctrlMonitorStatsPtr **stats,
> size_t *nstats)
> {
> - return virResctrlMonitorGetStats(monitor, "llc_occupancy",
> - stats, nstats);
> + char **features = NULL;
> + int ret = -1;
> +
> + virStringListAdd(&features, "llc_occupancy");
> + ret = virResctrlMonitorGetStats(monitor, (const char**)features, stats, nstats);
> + virStringListFree(features);
No need to dynamically create the string list.
const char *features = {"llc_occupancy", NULL};
is just fine. Also, this way you don't need to check if
virStringListAdd() succeeded ;-)
I know you're removing this function in next commit, but it still makes
sense todo things right.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2019年05月27日 23:26, Michal Privoznik wrote:
> On 5/23/19 11:34 AM, Wang Huaqiang wrote:
>> Export virResctrlMonitorGetStats and make
>> virResctrlMonitorGetCacheOccupancy obsoleted.
>>
>> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
>> ---
>> src/libvirt_private.syms | 1 +
>> src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++--------
>> src/util/virresctrl.c | 46
>> +++++++++++++++++++++++++++-------------------
>> src/util/virresctrl.h | 6 ++++++
>> 4 files changed, 59 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 9099757..2e3d48c 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2771,6 +2771,7 @@ virResctrlMonitorDeterminePath;
>> virResctrlMonitorFreeStats;
>> virResctrlMonitorGetCacheOccupancy;
>> virResctrlMonitorGetID;
>> +virResctrlMonitorGetStats;
>> virResctrlMonitorNew;
>> virResctrlMonitorRemove;
>> virResctrlMonitorSetAlloc;
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 4ea346c..bc19171 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -19987,6 +19987,7 @@
>> qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
>> /**
>> * qemuDomainGetResctrlMonData:
>> + * @driver: Pointer to qemu driver
>> * @dom: Pointer for the domain that the resctrl monitors reside in
>> * @resdata: Pointer of virQEMUResctrlMonDataPtr pointer for
>> receiving the
>> * virQEMUResctrlMonDataPtr array. Caller is responsible
>> for
>> @@ -20005,16 +20006,29 @@
>> qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
>> * Returns -1 on failure, or 0 on success.
>> */
>> static int
>> -qemuDomainGetResctrlMonData(virDomainObjPtr dom,
>> +qemuDomainGetResctrlMonData(virQEMUDriverPtr driver,
>> + virDomainObjPtr dom,
>> virQEMUResctrlMonDataPtr **resdata,
>> size_t *nresdata,
>> virResctrlMonitorType tag)
>> {
>> virDomainResctrlDefPtr resctrl = NULL;
>> virQEMUResctrlMonDataPtr res = NULL;
>> + char **features = NULL;
>> size_t i = 0;
>> size_t j = 0;
>> + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
>> + features = driver->caps->host.cache.monitor->features;
>
>
> No, we have virQEMUDriverGetCapabilities() which ensures that
> driver->caps object doesn't go away while accessing this.
>
I can't refer 'driver->caps->host.cache.monitor->features' directly
because this function (qemuDomainGetResctrlMonData)
will be used for 'memory bandwidth' monitors also.
at that time that the piece of code looks like:
+ if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
+ if (driver->caps->host.cache.monitor)
+ features = driver->caps->host.cache.monitor->features;
+ if (tag == VIR_RESCTRL_MONITOR_TYPE_MEMBW) {
+ if(driver->caps->host.membw)
+ features = driver->caps->host.membw.monitor->features;
Hope I understand your question correctly.
>> + } else {
>> + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> + _("Unsupported resctrl monitor type"));
>> + return -1;
>> + }
>> +
>> + if (virStringListLength((const char * const *)features) == 0)
>> + return 0;
>> +
>> for (i = 0; i < dom->def->nresctrls; i++) {
>> resctrl = dom->def->resctrls[i];
>> @@ -20041,9 +20055,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr
>> dom,
>> if (VIR_STRDUP(res->name,
>> virResctrlMonitorGetID(monitor)) < 0)
>> goto error;
>> - if (virResctrlMonitorGetCacheOccupancy(monitor,
>> - &res->stats,
>> - &res->nstats) < 0)
>> + if (virResctrlMonitorGetStats(monitor, (const char
>> **)features,
>> + &res->stats, &res->nstats)
>> < 0)
>> goto error;
>> if (VIR_APPEND_ELEMENT(*resdata, *nresdata, res) < 0)
>> @@ -20060,7 +20073,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom,
>> static int
>> -qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
>> +qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver,
>> + virDomainObjPtr dom,
>> virDomainStatsRecordPtr record,
>> int *maxparams)
>> {
>> @@ -20074,10 +20088,13 @@ qemuDomainGetStatsCpuCache(virDomainObjPtr
>> dom,
>> if (!virDomainObjIsActive(dom))
>> return 0;
>> - if (qemuDomainGetResctrlMonData(dom, &resdata, &nresdata,
>> + if (qemuDomainGetResctrlMonData(driver, dom, &resdata, &nresdata,
>> VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0)
>> goto cleanup;
>> + if (nresdata == 0)
>> + return 0;
>> +
>> snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>> "cpu.cache.monitor.count");
>> if (virTypedParamsAddUInt(&record->params, &record->nparams,
>> @@ -20175,7 +20192,7 @@ qemuDomainGetStatsCpuCgroup(virDomainObjPtr dom,
>> static int
>> -qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>> +qemuDomainGetStatsCpu(virQEMUDriverPtr driver,
>> virDomainObjPtr dom,
>> virDomainStatsRecordPtr record,
>> int *maxparams,
>> @@ -20184,7 +20201,7 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver
>> ATTRIBUTE_UNUSED,
>> if (qemuDomainGetStatsCpuCgroup(dom, record, maxparams) < 0)
>> return -1;
>> - if (qemuDomainGetStatsCpuCache(dom, record, maxparams) < 0)
>> + if (qemuDomainGetStatsCpuCache(driver, dom, record, maxparams) < 0)
>> return -1;
>> return 0;
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index c2fe2ed..76a8d02 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -2668,8 +2668,7 @@ virResctrlMonitorStatsSorter(const void *a,
>> * virResctrlMonitorGetStats
>> *
>> * @monitor: The monitor that the statistic data will be retrieved
>> from.
>> - * @resource: The name for resource name. 'llc_occupancy' for cache
>> resource.
>> - * "mbm_total_bytes" and "mbm_local_bytes" for memory bandwidth
>> resource.
>> + * @resources: A string list for the monitor feature names.
>> * @stats: Pointer of of virResctrlMonitorStatsPtr array for
>> holding cache or
>> * memory bandwidth usage data.
>> * @nstats: A size_t pointer to hold the returned array length of
>> @stats
>> @@ -2678,14 +2677,15 @@ virResctrlMonitorStatsSorter(const void *a,
>> *
>> * Returns 0 on success, -1 on error.
>> */
>> -static int
>> +int
>> virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
>> - const char *resource,
>> + const char **resources,
>> virResctrlMonitorStatsPtr **stats,
>> size_t *nstats)
>> {
>> int rv = -1;
>> int ret = -1;
>> + size_t i = 0;
>> unsigned int val = 0;
>> DIR *dirp = NULL;
>> char *datapath = NULL;
>> @@ -2743,21 +2743,23 @@
>> virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
>> if (virStrToLong_uip(node_id, NULL, 0, &stat->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;
>> + for (i = 0; resources[i]; i++) {
>> + rv = virFileReadValueUint(&val, "%s/%s/%s", datapath,
>> + ent->d_name, resources[i]);
>> + if (rv == -2) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("File '%s/%s/%s' does not exist."),
>> + datapath, ent->d_name, resources[i]);
>> + }
>> + if (rv < 0)
>> + goto cleanup;
>> - if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0)
>> - goto cleanup;
>> + if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0)
>> + goto cleanup;
>> - if (virStringListAdd(&stat->features, resource) < 0)
>> - goto cleanup;
>> + if (virStringListAdd(&stat->features, resources[i]) < 0)
>> + goto cleanup;
>> + }
>> if (VIR_APPEND_ELEMENT(*stats, *nstats, stat) < 0)
>> goto cleanup;
>> @@ -2813,6 +2815,12 @@
>> virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
>> virResctrlMonitorStatsPtr **stats,
>> size_t *nstats)
>> {
>> - return virResctrlMonitorGetStats(monitor, "llc_occupancy",
>> - stats, nstats);
>> + char **features = NULL;
>> + int ret = -1;
>> +
>> + virStringListAdd(&features, "llc_occupancy");
>> + ret = virResctrlMonitorGetStats(monitor, (const char**)features,
>> stats, nstats);
>> + virStringListFree(features);
>
> No need to dynamically create the string list.
>
> const char *features = {"llc_occupancy", NULL};
>
> is just fine. Also, this way you don't need to check if
> virStringListAdd() succeeded ;-)
> I know you're removing this function in next commit, but it still
> makes sense todo things right.
>
Agree. will be refined.
> Michal
Thanks for review.
Huaqiang
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 5/28/19 10:32 AM, Huaqiang,Wang wrote:
>
>
> On 2019年05月27日 23:26, Michal Privoznik wrote:
>> On 5/23/19 11:34 AM, Wang Huaqiang wrote:
>>> Export virResctrlMonitorGetStats and make
>>> virResctrlMonitorGetCacheOccupancy obsoleted.
>>>
>>> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
>>> ---
>>> src/libvirt_private.syms | 1 +
>>> src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++--------
>>> src/util/virresctrl.c | 46
>>> +++++++++++++++++++++++++++-------------------
>>> src/util/virresctrl.h | 6 ++++++
>>> 4 files changed, 59 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index 9099757..2e3d48c 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -2771,6 +2771,7 @@ virResctrlMonitorDeterminePath;
>>> virResctrlMonitorFreeStats;
>>> virResctrlMonitorGetCacheOccupancy;
>>> virResctrlMonitorGetID;
>>> +virResctrlMonitorGetStats;
>>> virResctrlMonitorNew;
>>> virResctrlMonitorRemove;
>>> virResctrlMonitorSetAlloc;
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 4ea346c..bc19171 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -19987,6 +19987,7 @@
>>> qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
>>> /**
>>> * qemuDomainGetResctrlMonData:
>>> + * @driver: Pointer to qemu driver
>>> * @dom: Pointer for the domain that the resctrl monitors reside in
>>> * @resdata: Pointer of virQEMUResctrlMonDataPtr pointer for
>>> receiving the
>>> * virQEMUResctrlMonDataPtr array. Caller is responsible
>>> for
>>> @@ -20005,16 +20006,29 @@
>>> qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
>>> * Returns -1 on failure, or 0 on success.
>>> */
>>> static int
>>> -qemuDomainGetResctrlMonData(virDomainObjPtr dom,
>>> +qemuDomainGetResctrlMonData(virQEMUDriverPtr driver,
>>> + virDomainObjPtr dom,
>>> virQEMUResctrlMonDataPtr **resdata,
>>> size_t *nresdata,
>>> virResctrlMonitorType tag)
>>> {
>>> virDomainResctrlDefPtr resctrl = NULL;
>>> virQEMUResctrlMonDataPtr res = NULL;
>>> + char **features = NULL;
>>> size_t i = 0;
>>> size_t j = 0;
>>> + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
>>> + features = driver->caps->host.cache.monitor->features;
>>
>>
>> No, we have virQEMUDriverGetCapabilities() which ensures that
>> driver->caps object doesn't go away while accessing this.
>>
>
> I can't refer 'driver->caps->host.cache.monitor->features' directly
> because this function (qemuDomainGetResctrlMonData)
> will be used for 'memory bandwidth' monitors also.
> at that time that the piece of code looks like:
> + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
> + if (driver->caps->host.cache.monitor)
> + features = driver->caps->host.cache.monitor->features;
> + if (tag == VIR_RESCTRL_MONITOR_TYPE_MEMBW) {
> + if(driver->caps->host.membw)
> + features = driver->caps->host.membw.monitor->features;
What I meant is that you can obtain virCaps object and then access it
instead:
virCapsPtr caps = virQEMUDriverGetCapabilities(driver, false);
if (tag == VIR_RES..)
features = caps->host.cache.monitor->features;
It's not safe to access driver->caps directly because another thread
might refresh those meanwhile. What may happen for instance is the
following:
Thread1:
fetches driver pointer
adds ->caps displacement
fetches value from that address (which is a long way of saying 'fetch
driver->caps')
Thread2:
Executes virQEMUDriverGetCapabilities(), which boils down to:
virObjectUnref(driver->caps);
driver->caps = caps;
Thread1:
resumes execution and tries to access ->host member.
This is where Thread1 would be accessing invalid memory because the
memory it's trying to access was freed meanwhile by Thread2.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.