Hi Cole, Good catch, I'll fix and make test soon. Thanks > -----Original Message----- > From: Cole Robinson [mailto:crobinso@xxxxxxxxxx] > Sent: Tuesday, August 20, 2019 6:30 AM > To: Michal Privoznik <mprivozn@xxxxxxxxxx>; Wang, Huaqiang > <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx > Cc: Su, Tao <tao.su@xxxxxxxxx> > Subject: Re: [PATCHv2 00/11] util/resctrl cleanups and refactors > > On 7/31/19 8:37 AM, Michal Privoznik wrote: > > On 6/11/19 5:31 AM, Wang Huaqiang wrote: > >> Patches submitted for purpose of refactoring existing 'resctrl' > >> related source code, including some code cleanups as well as some > >> fixes. This is also a preparation for memory bandwidth monitor codes. > >> > >> Plan to support Resctrl Control Monitors, which is a feature > >> introduced by kernel 'resctrl' sub-model. Submit some cleanup and > >> refactoring patches for upcoming memory bandwidth resource > monitoring > >> (MBM) monitors. > >> > >> Related MBM RFC is > >> https://www.redhat.com/archives/libvir-list/2019-April/msg01409.html. > >> This RFC is not actively discussed since libvirt already implemented > >> similar resctrl cache monitoring (CMT), and lots details have been > >> discussed and implemented during the work of CMT. > >> > >> The cleanups and refactoring includes: > >> v2 changes: > >> 1. Addressed comments of v1. > >> 2. Introduce a new algorithm for verifying new monitor vcpus and > >> existing monitors and allocations. > >> 3. Fixes for creating default-allocation-monitor in 'resctrl' file > >> system.(patch 0001). > >> > >> v1 changes: > >> 1. Removing some reluctant lines and white spaces that is existing in > >> current code and not meet the libvirt coding style. > >> 2. Replace 'virResctrlAllocIsEmpty' with @n==0 for indicating no > >> resctrl allocation in configuration file. > >> 3. Private API changes, removed 'virResctrlMonitorGetCacheOccupancy' > >> and exported a new API named 'virResctrlMonitorGetStats' with similar > >> functionality, but with capability to be used for retrieving MBM > >> statistical information. > >> 4. Refactoring 'virResctrlMonitorFreeStats' for more reusing in code. > >> 5. Extend data structure 'virResctrlMonitorStats' with the capability > >> to carry multiple statistical information from monitor. > >> > >> Wang Huaqiang (11): > >> util,conf: Handle default monitor group of an allocation properly > >> conf: code cleanup, remove empty line and one space > >> conf: code cleanup for return error code directly > >> conf: some code cleanup > >> conf: refactor 'virDomainResctrlVcpuMatch' and some code cleanup > > I think this patch introduced an issue. resctrl returned from VcpuMatch > should not be free'd, but that can happen in > virDomainMemorytuneDefParse > > Use the attached XML and do: > > ./tools/virsh --connect test:///default define bad.xml > > >> conf: Append 'resctrl' object according to number of monitor group > >> directly > >> util: Refactor and rename 'virResctrlMonitorFreeStats' > >> util: Refactor 'virResctrlMonitorStats' > >> util: Extend virresctl API to retrieve multiple monitor statistics > >> util: Remove unused virResctrlMonitorGetCacheOccupancy > >> conf: Refactor and rename the function to validate a new resctrl > >> monitor > >> > >> src/conf/domain_conf.c | 145 > >> ++++++++++++++++++++++++----------------------- > >> src/libvirt_private.syms | 5 +- > >> src/qemu/qemu_driver.c | 41 ++++++++++---- > >> src/qemu/qemu_process.c | 3 +- > >> src/util/virresctrl.c | 75 ++++++++++-------------- > >> src/util/virresctrl.h | 23 +++++--- > >> 6 files changed, 156 insertions(+), 136 deletions(-) > >> > > > > Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > > > > I'll push these once the freeze is over. > > > > > > > - Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
