From: Huaqiang <huaqiang.wang@intel.com>
The underlying resctrl monitoring is actually using 64 bit counters,
not the 32bit one. Correct this by using 64bit interfaces.
Signed-off-by: Huaqiang <huaqiang.wang@intel.com>
---
src/qemu/qemu_driver.c | 4 ++--
src/util/virfile.c | 40 ++++++++++++++++++++++++++++++++++++++++
src/util/virfile.h | 2 ++
src/util/virresctrl.c | 6 +++---
src/util/virresctrl.h | 2 +-
5 files changed, 48 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f4ff2ba292..e396358871 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20587,8 +20587,8 @@ qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver,
"cpu.cache.monitor.%zu.bank.%zu.id", i, j) < 0)
goto cleanup;
- if (virTypedParamListAddUInt(params, resdata[i]->stats[j]->vals[0],
- "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j) < 0)
+ if (virTypedParamListAddULLong(params, resdata[i]->stats[j]->vals[0],
+ "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j) < 0)
goto cleanup;
}
}
diff --git a/src/util/virfile.c b/src/util/virfile.c
index ced0ea70b7..372498e69e 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -4204,6 +4204,46 @@ virFileReadValueUint(unsigned int *value, const char *format, ...)
}
+/**
+ * virFileReadValueUllong:
+ * @value: pointer to unsigned long long to be filled in with the value
+ * @format, ...: file to read from
+ *
+ * Read unsigned int from @format and put it into @value.
+ *
+ * Return -2 for non-existing file, -1 on other errors and 0 if everything went
+ * fine.
+ */
+int
+virFileReadValueUllong(unsigned long long *value, const char *format, ...)
+{
+ g_autofree char *str = NULL;
+ g_autofree char *path = NULL;
+ va_list ap;
+
+ va_start(ap, format);
+ path = g_strdup_vprintf(format, ap);
+ va_end(ap);
+
+ if (!virFileExists(path))
+ return -2;
+
+ if (virFileReadAll(path, INT_BUFSIZE_BOUND(*value), &str) < 0)
+ return -1;
+
+ virStringTrimOptionalNewline(str);
+
+ if (virStrToLong_ullp(str, NULL, 10, value) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid unsigned long long value '%s' in file '%s'"),
+ str, path);
+ return -1;
+ }
+
+ return 0;
+}
+
+
/**
* virFileReadValueScaledInt:
* @value: pointer to unsigned long long int to be filled in with the value
diff --git a/src/util/virfile.h b/src/util/virfile.h
index a570529330..f77d8501c3 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -363,6 +363,8 @@ int virFileReadValueInt(int *value, const char *format, ...)
G_GNUC_PRINTF(2, 3);
int virFileReadValueUint(unsigned int *value, const char *format, ...)
G_GNUC_PRINTF(2, 3);
+int virFileReadValueUllong(unsigned long long *value, const char *format, ...)
+ G_GNUC_PRINTF(2, 3);
int virFileReadValueBitmap(virBitmapPtr *value, const char *format, ...)
G_GNUC_PRINTF(2, 3);
int virFileReadValueScaledInt(unsigned long long *value, const char *format, ...)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index b78fded026..684d2ce398 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2678,7 +2678,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
int rv = -1;
int ret = -1;
size_t i = 0;
- unsigned int val = 0;
+ unsigned long long val = 0;
DIR *dirp = NULL;
char *datapath = NULL;
char *filepath = NULL;
@@ -2734,8 +2734,8 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
goto cleanup;
for (i = 0; resources[i]; i++) {
- rv = virFileReadValueUint(&val, "%s/%s/%s", datapath,
- ent->d_name, resources[i]);
+ rv = virFileReadValueUllong(&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."),
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index 3dd7c96348..11f275acf4 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -204,7 +204,7 @@ struct _virResctrlMonitorStats {
char **features;
/* @vals store the statistical record values and @val[0] is the value for
* @features[0], @val[1] for@features[1] ... respectively */
- unsigned int *vals;
+ unsigned long long *vals;
/* The length of @vals array */
size_t nvals;
};
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Nov 14, 2019 at 01:08:19AM +0800, Wang Huaqiang wrote: > From: Huaqiang <huaqiang.wang@intel.com> > > The underlying resctrl monitoring is actually using 64 bit counters, > not the 32bit one. Correct this by using 64bit interfaces. > > Signed-off-by: Huaqiang <huaqiang.wang@intel.com> > --- > src/qemu/qemu_driver.c | 4 ++-- > src/util/virfile.c | 40 ++++++++++++++++++++++++++++++++++++++++ > src/util/virfile.h | 2 ++ > src/util/virresctrl.c | 6 +++--- > src/util/virresctrl.h | 2 +- > 5 files changed, 48 insertions(+), 6 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index f4ff2ba292..e396358871 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -20587,8 +20587,8 @@ qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver, > "cpu.cache.monitor.%zu.bank.%zu.id", i, j) < 0) > goto cleanup; > > - if (virTypedParamListAddUInt(params, resdata[i]->stats[j]->vals[0], > - "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j) < 0) > + if (virTypedParamListAddULLong(params, resdata[i]->stats[j]->vals[0], > + "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j) < 0) > goto cleanup; > } > } Urgh, we cannot do this, as it changes API semantics for applications. Apps are expecting this field to be encoded as UInt & so the change will break their decoding. Is this 32 vs 64-bit difference actually a problem in the real world. eg can the 32-bit value genuinely overflow in real deployments of this feature ? If not, then we should not change this at all. If we do need to change this though, the only option is to leave the current field unchanged, and document that it can be truncated. Then introduce a new field with a different name eg cpu.cache.monitor.%zu.bank.%zu.bytes64 Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi Daniel, Thanks for your review. About patch 1/5, I understand we should be very cautious when we changes the determined interface. I'd like to reserved the old 32bit interface and propose a new 64bit interface just as you suggested if you still think so after you got my intention. Here actually I want to fix a bug, maybe I should title this patch as *bug fixing*. Reason is the underlying hardware, the cache monitor and memory bandwidth counters, are 64bit width. Using 32bit interface to access these counters are problematic. This bug is not found because this interface is only used for tracking the amount of cache that used before this patch, normally the occupied cache will not exceed 4GB range. (32bit counter can counter value up to 4GB). But for memory, this counter records the data passing through the memory controller issued by this CPU in bytes and accumulatively, this value can easily exceed the 4GB bound, so I don’t want to reserve the old 32 bit interface and let user use it, because it will report incorrect value. > -----Original Message----- > From: Daniel P. Berrangé <berrange@redhat.com> > Sent: Friday, December 13, 2019 11:01 PM > To: Wang, Huaqiang <huaqiang.wang@intel.com> > Cc: libvir-list@redhat.com > Subject: Re: [libvirt] [PATCH 1/5] util, resctrl: using 64bit interface instead of > 32bit for counters > > On Thu, Nov 14, 2019 at 01:08:19AM +0800, Wang Huaqiang wrote: > > From: Huaqiang <huaqiang.wang@intel.com> > > > > The underlying resctrl monitoring is actually using 64 bit counters, > > not the 32bit one. Correct this by using 64bit interfaces. > > > > Signed-off-by: Huaqiang <huaqiang.wang@intel.com> > > --- > > src/qemu/qemu_driver.c | 4 ++-- > > src/util/virfile.c | 40 ++++++++++++++++++++++++++++++++++++++++ > > src/util/virfile.h | 2 ++ > > src/util/virresctrl.c | 6 +++--- > > src/util/virresctrl.h | 2 +- > > 5 files changed, 48 insertions(+), 6 deletions(-) > > > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index > > f4ff2ba292..e396358871 100644 > > --- a/src/qemu/qemu_driver.c > > +++ b/src/qemu/qemu_driver.c > > @@ -20587,8 +20587,8 @@ > qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver, > > "cpu.cache.monitor.%zu.bank.%zu.id", i, j) < 0) > > goto cleanup; > > > > - if (virTypedParamListAddUInt(params, resdata[i]->stats[j]->vals[0], > > - "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j) < 0) > > + if (virTypedParamListAddULLong(params, resdata[i]->stats[j]- > >vals[0], > > + > > + "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j) < 0) > > goto cleanup; > > } > > } > > Urgh, we cannot do this, as it changes API semantics for applications. > > Apps are expecting this field to be encoded as UInt & so the change will break > their decoding. > > Is this 32 vs 64-bit difference actually a problem in the real world. > > eg can the 32-bit value genuinely overflow in real deployments of this > feature ? Yes. The underlying interface is 64bit, and, for memory monitor, it tracks and adds up the data passing through the memory controller belong to the monitor in Bytes, the counter can easily overflow in a very short of time, for example just for one second. This bug/issue is introduced in 3f2214c2cdd9751df92ec9aa801e6161fa1a7009. > > > If not, then we should not change this at all. > > > > If we do need to change this though, the only option is to leave the current > field unchanged, and document that it can be truncated. > > Then introduce a new field with a different name > > eg cpu.cache.monitor.%zu.bank.%zu.bytes64 > > Regards, > Daniel > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- > https://www.instagram.com/dberrange :| Thanks Huaqiang -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Dec 15, 2019 at 02:39:20AM +0000, Wang, Huaqiang wrote: > Hi Daniel, > > Thanks for your review. > > About patch 1/5, I understand we should be very cautious when we changes the determined > interface. > > I'd like to reserved the old 32bit interface and propose a new 64bit interface just as you > suggested if you still think so after you got my intention. > Here actually I want to fix a bug, maybe I should title this patch as *bug fixing*. Reason is the > underlying hardware, the cache monitor and memory bandwidth counters, are 64bit width. > Using 32bit interface to access these counters are problematic. This bug is not found because > this interface is only used for tracking the amount of cache that used before this patch, normally > the occupied cache will not exceed 4GB range. (32bit counter can counter value up to 4GB). > But for memory, this counter records the data passing through the memory controller issued > by this CPU in bytes and accumulatively, this value can easily exceed the 4GB bound, so I > don’t want to reserve the old 32 bit interface and let user use it, because it will report incorrect value. We simply have to document the limitati onof the old interface. We can *NOT* change it, because it WILL break API compatibility for apps that deserailize the current data. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 2019/12/16 下午6:12, Daniel P. Berrangé wrote: > On Sun, Dec 15, 2019 at 02:39:20AM +0000, Wang, Huaqiang wrote: >> Hi Daniel, >> >> Thanks for your review. >> >> About patch 1/5, I understand we should be very cautious when we changes the determined >> interface. >> >> I'd like to reserved the old 32bit interface and propose a new 64bit interface just as you >> suggested if you still think so after you got my intention. >> Here actually I want to fix a bug, maybe I should title this patch as *bug fixing*. Reason is the >> underlying hardware, the cache monitor and memory bandwidth counters, are 64bit width. >> Using 32bit interface to access these counters are problematic. This bug is not found because >> this interface is only used for tracking the amount of cache that used before this patch, normally >> the occupied cache will not exceed 4GB range. (32bit counter can counter value up to 4GB). >> But for memory, this counter records the data passing through the memory controller issued >> by this CPU in bytes and accumulatively, this value can easily exceed the 4GB bound, so I >> don’t want to reserve the old 32 bit interface and let user use it, because it will report incorrect value. > We simply have to document the limitati onof the old interface. We can > *NOT* change it, because it WILL break API compatibility for apps that > deserailize the current data. Got. I'll resend new patches for review. Thanks Huaqiang > > > Regards, > Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.