[libvirt] [PATCH 4/9] util: Add MBA capability information query to resctrl
bing.niu
bing.niu at intel.com
Wed Jul 25 04:28:16 UTC 2018
On 2018年07月25日 06:08, John Ferlan wrote:
>
>
> On 07/18/2018 03:57 AM, bing.niu at intel.com wrote:
>> From: Bing Niu <bing.niu at intel.com>
>>
>> Introducing virResctrlInfoMemBW for the information memory bandwidth
>> allocation information. Those information is used for memory
>> bandwidth allocating.
>
> Last sentence is duplicitous.
>
>>
>> Signed-off-by: Bing Niu <bing.niu at intel.com>
>> ---
>> src/util/virresctrl.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 104 insertions(+)
>>
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index 1515de2..06e2702 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -80,6 +80,9 @@ typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr;
>> typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel;
>> typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;
>>
>> +typedef struct _virResctrlInfoMemBW virResctrlInfoMemBW;
>> +typedef virResctrlInfoMemBW *virResctrlInfoMemBWPtr;
>> +
>> typedef struct _virResctrlAllocPerType virResctrlAllocPerType;
>> typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
>>
>> @@ -116,11 +119,31 @@ struct _virResctrlInfoPerLevel {
>> virResctrlInfoPerTypePtr *types;
>> };
>>
>> +/* Information about memory bandwidth allocation */
>> +struct _virResctrlInfoMemBW {
>> + /* minimum memory bandwidth allowed */
>> + unsigned int min_bandwidth;
>> + /* bandwidth granularity */
>> + unsigned int bandwidth_granularity;
>> + /* Maximum number of simultaneous allocations */
>> + unsigned int max_allocation;
>> + /* level number of last level cache */
>> + unsigned int last_level_cache;
>> + /* max id of last level cache, this is used to track
>> + * how many last level cache available in host system,
>> + * the number of memory bandwidth allocation controller
>> + * is identical with last level cache.
>> + */
>
> The closing */ can be on the previous line.
>
>> + unsigned int max_id;
>> +};
>> +
>> struct _virResctrlInfo {
>> virObject parent;
>>
>> virResctrlInfoPerLevelPtr *levels;
>> size_t nlevels;
>> +
>> + virResctrlInfoMemBWPtr membw_info;
>> };
>>
>>
>> @@ -146,6 +169,7 @@ virResctrlInfoDispose(void *obj)
>> VIR_FREE(level);
>> }
>>
>> + VIR_FREE(resctrl->membw_info);
>> VIR_FREE(resctrl->levels);
>> }
>>
>> @@ -442,6 +466,65 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp)
>>
>>
>> static int
>> +virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl)
>> +{
>> + int ret = -1;
>> + int rv = -1;
>> + virResctrlInfoMemBWPtr i_membw = NULL;
>> +
>> + /* query memory bandwidth allocation info */
>> + if (VIR_ALLOC(i_membw) < 0)
>> + goto cleanup;
>> + rv = virFileReadValueUint(&i_membw->bandwidth_granularity,
>> + SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran");
>> + if (rv == -2) {
>> + /* The file doesn't exist, so it's unusable for us,
>> + * properly mba unsupported */
>
> s/ properly/probably/
>
> ?? Is that what you meant?
>
Yes! probably. :)
> s/mba/memory bandwidth/
s/mba/memory bandwidth allocation/
>
> right?
>
>> + VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran'"
>> + "does not exist");
>> + ret = 0;
>> + goto cleanup;
>> + } else if (rv < 0) {
>> + /* Other failures are fatal, so just quit */
>> + goto cleanup;
>> + }
>> +
>> + rv = virFileReadValueUint(&i_membw->min_bandwidth,
>> + SYSFS_RESCTRL_PATH "/info/MB/min_bandwidth");
>> + if (rv == -2) {
>> + /* If the previous file exists, so should this one. Hence -2 is
>
> s/ Hence/ Hence/
>
> It's a raging debate at times on this list whether to go with 1 space or
> 2. I think 1 space usually wins, but I use 2 many times as well. I'll
> adjust this to 1 space though ;-)
Good to know this history :). Above 2 space is just following the
existing virresctrl code.
>
>> + * 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. :(
>
>> +
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot get min bandwidth from resctrl memory info"));
>> + }
>> + if (rv < 0)
>> + goto cleanup;
>> +
>> + rv = virFileReadValueUint(&i_membw->max_allocation,
>> + SYSFS_RESCTRL_PATH "/info/MB/num_closids");
>> + if (rv == -2) {
>> + /* If the previous file exists, so should this one. Hence -2 is
>> + * fatal in this case as well (errors out in next condition) - the
>> + * kernel interface might've changed too much or something else is
>> + * wrong. */
>
> I'd just note /* Similar reasoning as min_bandwidth above */
Short and clear!
>
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot get max allocation from resctrl memory info"));
>> + }
>> + if (rv < 0)
>> + goto cleanup;
>> +
>> + VIR_STEAL_PTR(resctrl->membw_info, i_membw);
>> + ret = 0;
>> + cleanup:
>> + VIR_FREE(i_membw);
>> + return ret;
>> +}
>> +
>> +
>> +static int
>> virResctrlGetInfo(virResctrlInfoPtr resctrl)
>> {
>> DIR *dirp = NULL;
>> @@ -451,6 +534,10 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
>> if (ret <= 0)
>> goto cleanup;
>>
>> + ret = virResctrlGetMemoryBandwidthInfo(resctrl);
>> + if (ret < 0)
>> + goto cleanup;
>> +
>> ret = virResctrlGetCacheInfo(resctrl, dirp);
>> if (ret < 0)
>> goto cleanup;
>> @@ -492,6 +579,9 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
>> if (!resctrl)
>> return true;
>>
>> + if (resctrl->membw_info)
>> + return false;
>> +
>> for (i = 0; i < resctrl->nlevels; i++) {
>> virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];
>>
>> @@ -517,12 +607,26 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
>> {
>> virResctrlInfoPerLevelPtr i_level = NULL;
>> virResctrlInfoPerTypePtr i_type = NULL;
>> + virResctrlInfoMemBWPtr membw_info = NULL;
>
> This can be local to the if...
>
>> size_t i = 0;
>> int ret = -1;
>>
>> if (virResctrlInfoIsEmpty(resctrl))
>> return 0;
>>
>> + /* Let's take the opportunity to update the number of last level
>> + * cache. This number of memory bandwidth controller is same with
>> + * last level cache */
>> + if (resctrl->membw_info) {
>
> virResctrlInfoMemBWPtr membw_info = resctrl->membw_info;
Yes. Should put to here. :). I remember some compile flags will complain
if local variable not at the begin of a function.
>
>> + 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
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.
>
> 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
>
More information about the libvir-list
mailing list