Re: [libvirt] [PATCH 4/9] util: Add MBA capability information query to resctrl
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
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/9] util: Add MBA capability information query to resctrl
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
>>
>> 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@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/9] util: Add MBA capability information query to resctrl
On 2018年07月25日 06:08, John Ferlan wrote:
On 07/18/2018 03:57 AM, bing@intel.com wrote:
From: Bing Niu
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
---
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_INTERN
Re: [libvirt] [PATCH 4/9] util: Add MBA capability information query to resctrl
On 07/18/2018 03:57 AM, bing@intel.com wrote:
> From: Bing Niu
>
> 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
> ---
> 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?
s/mba/memory bandwidth/
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 ;-)
> + * 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
> +
> +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 */
> +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
