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

On 08/27/2018 07:23 AM, Wang Huaqiang wrote: Do you mean you're setting the basis for future patches to provide the capability data for monitor info? Caveat - I didn't go back and read all the previous history on this. Sorry there's just too much. I hope that mkletzan will also take a look at the series since he was involved previously. There's two things going on in this patch: 1. The actual fetch of the data into resctrl structures 2. The movement/copy of some of that data into @bank Splitting the patches such that item 1 is separate and then item 2 is combined with patches 3 and 4 along with some doc adjustments to describe the output. Of course I just peeked looking for "cache" and "bank" in docs/*.in and found nothing /-|... Looks like docs/formatcaps.html.in needs some love to describe <cache> and <memory_bandwidth> (how come this stuff comes to me afterwards...) I wonder if perhaps if it'd be better to have virResctrlInfoGetCache just take @bank as a parameter instead of continually adding more... I'm also not convinced @bank is the best place considering it's the same data that repeated gets fetched/stored. This structure notes usage for specific @level; however, from how I read the path to the data, the data is only provided in L3. Since on output <bank> can specify a specific level, I assume that L1 or L2 would be possible for it; however, given how the code is written wouldn't that mean @monitor data is included as well? Additionally from how I read things it seems the same data is repeated for each bank id='#' found. So is the data unique to a bank by id or is unique to the level regardless of the bank? If the former, then the data needs to be properly split so it can be shown to be different for each id. If the latter, then <monitor> wouldn't seem to need to be a child of <bank>. It's not clear whether it's then a child or peer of <cache>. Could add a few comments here to describe what's being provided here and how it fits (regardless of where in the schema of things it ends up). Consider using STRPREFIX. So we get a list filtered by prefixes "llc_" and "mbm_" Eventually this list gets pared down again to just "llc_". None of the subsequent patches do anything with "mbm_" other than list it in capabilities XML output. Sure "mbm_" could be used in the future, but the question that comes to mind is why are initially filtering at all? That is, why not just replace lines/nlines with info->features and info->nfeatures? That then provides "everything" supported in the tree, right? I would figure this code would be just mirroring what's available. It's then up to the upper layers or other code to decide what to do with the list. consider using cleanup as part of the processing and thus removing the need to have multiple VIR_FREE(featurestr) and virStringListFree(lines). If you add a char **features = NULL; which you use to perform the virStringListAdd calls and then VIR_STEAL_PTR(info->features, features); VIR_STEAL_PTR(resctrl->monitor_info, info); ret = 0; and fall through allowing the featurestr, lines, and features to be used below. Of course that all assumes it's really necessary to do the filtering. There's also the VIR_AUTOPTR stuff added which I'm not exactly sure how to use yet as I wasn't part of that effort. Again use STRPREFIX instead. This code further filters our "llc_" and "mbm_" list into just "llc_". Not sure I understand why we only "care about last level cache" values just yet. Of course I see that is all that gets added the subsequent patch capabilities output, but is that really what's wanted. Are we going to start seeing patches that start expanding this list? Why limit/filter now? if (!cachemon->features), then @cachemon is leaked, consider using: VIR_STEAL_PTR(*monitor, cachemon); in the if condition, then VIR_FREE(cachemon); or just the VIR_FREE(cachemon); as an else. IDC either way. Of course, it's still not quite clear what's going on. Perhaps, you should have an API that gets all the names of the values prefixed by some string, IOW: virResctrlInfoGetMonitorPrefix(resctrl, filter) where filter is a "const char *filter" it would return that cachemon list whether it's NULL, empty, or full anything. Let the caller decide what to do with it. I haven't looked beyond the first 4 patches, so how things are used later on may determine what API's you could need. The relationship to <cache> and <bank> isn't clear. What kind of allocations? From your cover you state maximum number of monitoring groups that could be created, but it's impossible to know how this value is expected to be used by what's provided as a comment here. The code shows this is the value from /info/L3_MON/num_rmids - I don't see the correlation in FS name to structure name. Since you later print a unit of 'B' I assume it's bytes of something, but the comment seems to imply a maximum simultaneous number of something. Perhaps if documentation was added I would have had my answer without needing to go research that is. Again, alone this comment is difficult to decipher as it relates to the structure field name. The code shows the value read is "/info/L3_MON/max_threshold_occupancy". It's not clear what "occupancy" means. Is there something related to this number that some consumer could change that would improve some performance? John