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

[bing.niu]

On 2018年07月26日 06:31, John Ferlan wrote:
> 
> 
> 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.

Thanks for this!
> 
>>>
> 
> [...]
> 
>>>
>>>> +        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/).

Above summary is clear and informative. :)
> 
> 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
>>>