Re: [libvirt] [PATCH 02/10] util: add interface retrieving CMT capability

Yes, patches 1 to 4 are adding host capability for cache monitor. This patch introduces two structures, adding 'virResctrlInfoMonPtr' to @resctrl (type 'virResctrlInfoPtr') to keep the resctrl monitoring group capabilities, and another is adding 'virResctrlInfoMonPtr' for each cache @bank (type 'virCapsHostCacheBankPtr') to store capabilites of cache monitor. A subsequent patch 3 posts cache monitor capability information to host capability query request. This is accomplished by filling information to @virResctrlInfo->monitor_info. This is accomplished by filling information to @virCapsHostCacheBankPtr->monitor Make sense. Will split this patch as you suggested, and add doc content. But patch 4 is a test patch for the newly involved cache monitor capability, do you think it is should be merged with patch 3 and second half of patch 2? I'll provide the descriptions for cache monitor. This should be OK, since 'virCapsHostCacheBankPtr' structure is fully exposed in 'capabilities.h' file. The suggestion will be followed. @level could be 1,2 or 3 for a 3-level cache system. @monitor will be involved in structure of each level cache. @monitor will be set to NULL if hardware does not support monitoring. For example any L1D cache does not support resource monitoring, then the corresponding 'virCapsHostCacheBank' 's @monitor will be set to NULL. Data is only kept in cache 'bank' that supports cache monitor feature, for cache that does not support monitoring feature, the @monitor will be set to 'NULL'. And, yes, it might be duplicated for serval times for a multi-node system, because multiple cache 'bank', which has feature of cache monitoring, were found. Seems you have a big concern for my arrangement of putting @monitor inside 'virCapsHostCacheBankPtr' structure. Before introducing my considerations for this arrangement, let me clarify the definition of 'cache bank' and the cache topologoy, I may make mistakes here, if does, please correct me. 'cache bank' has a group of attributes, including 'id', 'level', 'type', 'size' and 'cpus', there attributes are defined by Linux kernel. You can find the values of some specific CPU cache block from this directory: /sys/devices/system/cpu/cpu*/cache/* My understanding to libvirt cache 'bank' (or 'cache bank'): 'virCapsHostCacheBankPtr' represents the 'cache bank'. The 'virCapsHostCacheBankEquals', listed as below, tell us: for two 'cache bank', if they have exactly the same attributes values, means, they are the same 'cache bank', otherwise, they are different 'cache bank's. bool virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a, virCapsHostCacheBankPtr b) { return (a->id == b->id && a->level == b->level && a->type == b->type && a->size == b->size && virBitmapEqual(a->cpus, b->cpus)); } How many 'cache bank's in system? and what is the relationship for 'cache bank' and hardware cache block? For convenience, take 2-socket E5-2699v4 system for example, there are two CPU nodes in system, each CPU(or node) has 22 hardware CPU cores, each core is equipped with two private L1 caches and one private L2 data cache, each node has a L3(or LLC) cache shared among cores. Let's use the concept of 'cache bank' to describe the cache of this 2-socket system, there are two L3 'cache bank's (assuming CDP is disabled), one for each socket and shared by associated CPU cores; There are totally 44 CPU cores, and each CPU core has three private 'cache bank's, the private L1D 'cache bank', L1I 'cache bank' and L2 'cache bank'. In total, 44x3+2=134 'cache bank's exist in the system. Here are my considerations for add @monitor for each 'cache bank': 1. This leverages the design of @controls/@ncontrols. a.) @controls/@ncontrols is designed for introducing cache allocation(CAT) feature, only last level cache of particular CPU supports cache allocation, private 'cache bank' does not support cache allocation. b.) @monitor follows the same policy of @controls/@ncontrols does. For 'cache bank' supports cache monitor, populates appropriate cache information through 'virCapsHostCacheBankPtr'->monitor; for 'cache bank' does not support cache monitor feature, just leave @monitor to empty. 2. Cache ‘monitor’ is designed as an accompany concept to cache ‘control’: cache ‘control’ mostly covers CAT technology, and cache ‘monitor’ now refers to CMT technology. You also mentioned "it seems the same data is repeated". Yes, it does for some system, for example, a 2-socket E5-2600v4 system. This is also confusing me a lot when I began to write the POC code. As I said, the 'cache monitor' leverages the 'cache allocation' design, the capability of cache allocation is stored in each 'cache bank' through @ncontrols and @controls also, @controls is empty and @ncontrols is 0, mean that there is no capability of cache allocation for this 'cache bank', otherwise, the @controls/@ncontrols points to an array of 'virResctrlInfoPerCachePtr' pointers, where it indicates the capabilities of cache allocation (CAT) feature. The content of @controls/@ncontrols may also duplicated. So libvirt has the capability (or designed) to keep 'cache bank' unique information, but resctrl could not provide such kind of 'cache bank' unique information, even you have a multiple socket system each socket populates a different CPU. The reality is resctrl only reports system wide CAT, CMT, MBA, MBM capabilities, it does not report CAT,CMT,MBA,MBM capabilities based on 'cache bank'. I think there should have an explanation for the necessity of keeping the cache allocation capability based on 'cache bank', despite the fact that resctrl only provide a copy of cache allocation capability data. Maybe, it is not wise to leverage the design of @bank/@controls here. or maybe I totally misunderstood the design of @bank/@controls. how about adding these as function comments: /* * Retrieve the capability of resource monitoring group by checking the kernel * resource control interface. * the capability information includes: * max_allocation: the maximum number of monitoring groups could be created. * mon_features: the monitoring features supported, which could be * 'llc_occupancy' for the feature of reporting how much last level cache using. * 'mbm_total_bytes' for the feature of reporting total memory bandwidth using. * 'mbm_local_bytes' for the feature of reporting local memory bandwidth using. * cache_threshold: this affects the actual destroy of resource monitoring * group, mainly affects the hardware resource reclaim, a greater value of this, in * bytes, will make the request of creating new resource monitoring group more * likely to fail if the existing number of monitoring groups reaches up to * 'max_allocation'. */ OK. To be fixed. Agree. Will remove the filter, just report the content from resctrl. Agree. Let upper layer make decision. Thanks for advice, really helpful. Will pay attention to 'cleanup'/'error' label and its backend logic. Will be fixed. Hope I addressed your concerns: In resctrl, there are three resource monitoring features: 'llc_occupancy' for cache monitor. 'mbm_total_bytes' and 'mbm_local_bytes' for memory bandwidth monitor. In this series patches I only introduced the cache monitor, and memory bandwidth monitor is planned to be introduced in a series of patches afterwards. All resctrl features list above ('llc_'s and 'mbm_'s) are kept in @resctrl->monitor_info, then, when libvirt wants to get the feature list for, just for, cache monitor, function virResctrlGetCacheInfo will be invoked, only 'llc_' are the interested feature name. this is the reason why limit/filter applied here. (will using full name filter instead of the form such as 'llc_*') The virResctrlInfoGetCache will be called for any 'cache bank' that supported 'llc_occupancy' feature, the result will be stored in 'cache bank' private data area (with data type virCapsHostCacheBankPtr). Of course, as you mentioned, the data may be duplicated. The monitor feature list will be expanded when formatting cache monitor capabilities string in task of reporting host capabilities, as illustrated in following code: function virCapabilitiesFormatCaches(a)capabilities.c ... if (bank->monitor && bank->monitor->nfeatures) { virBufferAsprintf(&childrenBuf, "<monitor threshold='%u' unit='B' " "maxAllocs='%u'>\n", bank->monitor->cache_threshold, bank->monitor->max_allocation); /* expanding cache monitor feature list */ for (j = 0; j < bank->monitor->nfeatures; j++) { virBufferAdjustIndent(&childrenBuf, 2); virBufferAsprintf(&childrenBuf, "<feature name='%s'/>\n", bank->monitor->features[j]); virBufferAdjustIndent(&childrenBuf, -2); } virBufferAddLit(&childrenBuf, "</monitor>\n"); } ... You catched my bug. Thanks. Looks reasonable and make code more concise, especially when adding memory bandwidth monitor. Will be implemented in next version patch. "The relationship to <cache> and <bank> isn't clear." -- Not catching your idea too much, do you mean the relationship to 'physical CPU cache' and the software scope 'cache bank' is not clear? If yes, please refer to my upper replies, the clarification paragraph of 'cache bank' with an example of 2S E5-2699v4 system. Changing the comment from /* Maximum number of simultaneous allocations */ to /* Maximum number of monitoring groups that could be created */ Looks the capability string make you confused. The XML output of cache monitor capability looks like: (leveraging the format of 'cache control' capability) <monitor threshold='270336' unit='B' maxAllocs='176'> <feature name='llc_occupancy'/> </monitor> 'B' is the unit for 'threshold', 'maxAllocs' is for max_allocation (parsed through /info/L3_MON/num_rmids). The 'unit' are not limited to 'B', could be 'KiB' ... This tells docs are necessary for interpreting these settings. "monitor": describes cache monitor capability. "threshold": This is cache occupancy threshold value used in kernel resource control system, and affects the actual release of hardware resource, the RMID (resource monitoring ID). A greater value of this will make the request of creating a new resource monitoring group more likely to fail if the existing number of monitoring groups reaches up to 'maxAlloc'. "unit": This is the unit of "threashold", could be 'B', 'KiB', 'MiB' or 'GiB'. "maxAllocs": This is a number that maximum number of monitoring groups could be created. "feature": describes the feature name supported by 'monitor'. Hope this documentation clears your confusion. See docs above. "max_threshold_occupancy" is a concept involved by kernel resctrl. It is a cache value, in bytes, affects the release of hardware 'RMID', thus, affects the maximum number of monitor group could be created. Get more information from the cache monitor attribute 'threshold''s description. Thanks for your efforts of the review.