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

[John Ferlan]

On 07/25/2018 12:28 AM, bing.niu wrote:
> 
> 
> On 2018年07月25日 06:08, John Ferlan wrote:

[...]

>>
>>> +         * fatal in this case (errors out in next condition) - the
>>> +         * kernel interface might've changed too much or something
>>> else is
>>> +         * wrong. */
>>
>> "kernel" can fit on previous line meaning "wrong." can fit on previous
>> line.  Also, no need for blank line between here and virReportError
> 
> Sorry I am a bit lost. Do you mean put "kernel" in previous line? right?
> I have a try in Thunderbird. Looks like put “kernel” in previous line
> beyond max length of line and Thunderbird give me automatically split. :(

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.

> Above hunk is used to calculate the number of memory bandwidth
> allocation controllers in system. The number of memory bandwidth
> allocation controllers is same as last level cache. This number is
> calculate by traversing cache hierarchy of
> host(/sys/bus/cpu/devices/cpuX/cache/).
> So above hunk is used to collect that information, to sanitize domain
> XML about memory bandwidth allocation part. And the number of memory
> bandwidth allocation controllers will not change, it just cannot query
> directly from RDT kernel module.

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 at redhat.com>
>>
>> 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>