Re: [libvirt] [PATCH 4/9] util: Add MBA capability information query to resctrl

On 07/25/2018 12:28 AM, bing.niu wrote: [...] Yes moving kernel up a line is what I meant. It's taken care of in my branch already. [...] >>> +        membw_info = resctrl->membw_info; >>> +        if (level > membw_info->last_level_cache) { >>> +            membw_info->last_level_cache = level; >>> +            membw_info->max_id = 0; >>> +        } else if (membw_info->last_level_cache == level) { >>> +            membw_info->max_id++; >>> +        } >>> +    } >>> + >> This last hunk should be it's own patch. I can split it out for you. >> The rest of the patch introduces the concept, does the query, and saves >> the data in the "right place". >> This hunk says, hey we have some membw_info data that can change our >> perception of reality, so we need to adjust. Although nothing yet has >> set last_level_cache or max_id...  I'll assume that's comeing. >> I'll also make membw_info "local" to the if {}. >> The only hmm... I have is this last hunk, I've already forgotten what we >> discussed the previous series. But my hmm is related to why is it here >> and what impact can it (eventually) have if the values are changed in >> this method while perhaps also being changed in a different method.  I'm >> sure I'll learn more of that as I move forward. > > Thanks for that! Let me help to recall the discuss. :) > RDT kernel module will report some parameters for MBA. They are : > "min_bandwidth":        The minimum memory bandwidth percentage which >                          user can request. > > "bandwidth_gran":       The granularity in which the memory bandwidth >                           percentage is allocated. The allocated >                           b/w percentage is rounded off to the next >                           control step available on the hardware. The >                           available bandwidth control steps are: >                           min_bandwidth + N * bandwidth_gran. > > "num_closids":          The number mba group can exist simultaneous. > > Those information is query from kernel interface /sys/fs/resctrl/info/MB > Right this I understand the other fields are fetch-able. Setting last_level_cache and max_id is only ever done in virResctrlInfoGetCache. I have to remind myself what ends up calling into here and the order of processing for all this code. So does the following seem like a good summary for the split out patch?: util: Add MBA check to virResctrlInfoGetCache If we have some membw_info data, then we need to calculate the number of MBA controllers on the system. The value cannot be obtained from a direct query to the RDT kernel module, but it is the same as the last level cache value which is calculated by traversing the cache hierarchy of host(/sys/bus/cpu/devices/cpuX/cache/). John >> Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; >> John >> BTW: I'm stopping here for the evening, I'll work through the rest >> hopefully tomorrow depending on interruptions. > > Good evening :). > The rest are about 1. allocate memory bandwidth 2. domain XML and 3. > host capability XML. >>>       if (level >= resctrl->nlevels) >>>           return 0; >>>   >> -- >> libvir-list mailing list >> libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list