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