[libvirt] [PATCH 1/5] util, resctrl: using 64bit interface instead of 32bit for counters

[Wang, Huaqiang]

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 at redhat.com>
> Sent: Friday, December 13, 2019 11:01 PM
> To: Wang, Huaqiang <huaqiang.wang at intel.com>
> Cc: libvir-list at 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 at 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 at 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