Re: [libvirt] [PATCHv2 00/11] util/resctrl cleanups and refactors

-----Original Message----- From: Cole Robinson [mailto:crobinso@redhat.com] Sent: Tuesday, August 20, 2019 6:30 AM To: Michal Privoznik <mprivozn(a)redhat.com>; Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com Cc: Su, Tao <tao.su(a)intel.com&gt; Subject: Re: [libvirt] [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(a)redhat.com&gt; > > I'll push these once the freeze is over. > - Cole