Re: [libvirt] [PATCH 1/5] util, resctrl: using 64bit interface instead of 32bit for counters
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(a)redhat.com>
Sent: Friday, December 13, 2019 11:01 PM
To: Wang, Huaqiang <huaqiang.wang(a)intel.com>
Cc: libvir-list(a)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(a)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(a)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