6:13 a.m.
v1:
https://www.redhat.com/archives/libvir-list/2015-September/msg01054.html
v2:
-improve the error message
-improve the code struct
-add a new patch for rework the exist way to check node and page size
Luyao Huang (3):
util: split the virNumaGetHugePageInfoPath into separate function
util: move the pagesize and node check and error report to one place
util: Produce friendlier error message to user
src/util/virnuma.c | 113 ++++++++++++++++++++++++++++++++++-------------------
1 file changed, 72 insertions(+), 41 deletions(-)
--
1.8.3.1
6:13 a.m.
New subject: [libvirt] [PATCHv2 1/3] util: split the virNumaGetHugePageInfoPath into separate function
https://bugzilla.redhat.com/show_bug.cgi?id=1265114
When pass 0 page_size to virNumaGetHugePageInfoPath function, we will
get fail like this:
error : virFileReadAll:1358 : Failed to read file
'/sys/devices/system/node/node0/hugepages/': Is a directory
Because when the page_size is 0 the virNumaGetHugePageInfoPath will
build the directory of system path, but we don't want that.
Introduce a new helper to build the dir path could avoid this issue.
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
src/util/virnuma.c | 51 +++++++++++++++++++--------------------------------
1 file changed, 19 insertions(+), 32 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 1a62d62..cb80972 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -493,44 +493,31 @@ virNumaGetHugePageInfoPath(char **path,
unsigned int page_size,
const char *suffix)
{
-
- int ret = -1;
-
if (node == -1) {
/* We are aiming at overall system info */
- if (page_size) {
- /* And even on specific huge page size */
- if (virAsprintf(path,
- HUGEPAGES_SYSTEM_PREFIX HUGEPAGES_PREFIX
"%ukB/%s",
- page_size, suffix ? suffix : "") < 0)
- goto cleanup;
- } else {
- if (VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX) < 0)
- goto cleanup;
- }
-
+ return virAsprintf(path,
+ HUGEPAGES_SYSTEM_PREFIX HUGEPAGES_PREFIX "%ukB/%s",
+ page_size, suffix ? suffix : "");
} else {
/* We are aiming on specific NUMA node */
- if (page_size) {
- /* And even on specific huge page size */
- if (virAsprintf(path,
- HUGEPAGES_NUMA_PREFIX "node%d/hugepages/"
- HUGEPAGES_PREFIX "%ukB/%s",
- node, page_size, suffix ? suffix : "") < 0)
- goto cleanup;
- } else {
- if (virAsprintf(path,
- HUGEPAGES_NUMA_PREFIX "node%d/hugepages/",
- node) < 0)
- goto cleanup;
- }
+ return virAsprintf(path,
+ HUGEPAGES_NUMA_PREFIX "node%d/hugepages/"
+ HUGEPAGES_PREFIX "%ukB/%s",
+ node, page_size, suffix ? suffix : "");
}
-
- ret = 0;
- cleanup:
- return ret;
}
+static int
+virNumaGetHugePageInfoDir(char **path, int node)
+{
+ if (node == -1) {
+ return VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX);
+ } else {
+ return virAsprintf(path,
+ HUGEPAGES_NUMA_PREFIX "node%d/hugepages/",
+ node);
+ }
+}
/**
* virNumaGetHugePageInfo:
@@ -724,7 +711,7 @@ virNumaGetPages(int node,
* is always shown as used memory. Here, however, we want to report
* slightly different information. So we take the total memory on a node
* and subtract memory taken by the huge pages. */
- if (virNumaGetHugePageInfoPath(&path, node, 0, NULL) < 0)
+ if (virNumaGetHugePageInfoDir(&path, node) < 0)
goto cleanup;
if (!(dir = opendir(path))) {
--
1.8.3.1
6:13 a.m.
New subject: [libvirt] [PATCH 2/3] util: move the pagesize and node check and error report to one place
Just like 1c24cfe9, check the pagesize and numa node if we cannot find
the hugepage path. And improve the error message again.
After this patch:
# virsh allocpages --pagesize 2047 --pagecount 1 --cellno 0
error: operation failed: page size 2047 is not available on node 0
# virsh allocpages --pagesize 2047 --pagecount 1
error: operation failed: page size 2047 is not available
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
src/util/virnuma.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index cb80972..815cbc1 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -836,19 +836,25 @@ virNumaSetPagePoolSize(int node,
goto cleanup;
}
- if (node != -1 && !virNumaNodeIsAvailable(node)) {
- virReportError(VIR_ERR_OPERATION_FAILED,
- _("NUMA node %d is not available"),
- node);
- goto cleanup;
- }
-
if (virNumaGetHugePageInfoPath(&nr_path, node, page_size,
"nr_hugepages") < 0)
goto cleanup;
if (!virFileExists(nr_path)) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("page size or NUMA node not available"));
+ if (node != -1) {
+ if (!virNumaNodeIsAvailable(node)) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("NUMA node %d is not available"),
+ node);
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("page size %u is not available on node %d"),
+ page_size, node);
+ }
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("page size %u is not available"),
+ page_size);
+ }
goto cleanup;
}
--
1.8.3.1
6:13 a.m.
New subject: [libvirt] [PATCHv2 3/3] util: Produce friendlier error message to user
Commit 1c24cfe9 fix the problem in virNumaSetPagePoolSize,
this patch just like it and fix the issue in another function.
when user use freepages and specify a invalid node or page size,
will get error like this:
# virsh freepages 0 1
error: Failed to open file
'/sys/devices/system/node/node0/hugepages/hugepages-1kB/free_hugepages':No such
file or directory
Add two checks to catch this and therefore produce much more
friendlier error messages.
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
src/util/virnuma.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 815cbc1..cd000f9 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -553,6 +553,25 @@ virNumaGetHugePageInfo(int node,
page_size, "nr_hugepages") < 0)
goto cleanup;
+ if (!virFileExists(path)) {
+ if (node != -1) {
+ if (!virNumaNodeIsAvailable(node)) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("NUMA node %d is not available"),
+ node);
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("page size %u is not available on node
%d"),
+ page_size, node);
+ }
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("page size %u is not available"),
+ page_size);
+ }
+ goto cleanup;
+ }
+
if (virFileReadAll(path, 1024, &buf) < 0)
goto cleanup;
@@ -572,6 +591,25 @@ virNumaGetHugePageInfo(int node,
page_size, "free_hugepages") < 0)
goto cleanup;
+ if (!virFileExists(path)) {
+ if (node != -1) {
+ if (!virNumaNodeIsAvailable(node)) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("NUMA node %d is not available"),
+ node);
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("page size %u is not available on node
%d"),
+ page_size, node);
+ }
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("page size %u is not available"),
+ page_size);
+ }
+ goto cleanup;
+ }
+
if (virFileReadAll(path, 1024, &buf) < 0)
goto cleanup;
--
1.8.3.1
3:58 p.m.
New subject: [libvirt] [PATCHv2 3/3] util: Produce friendlier error message to user
On 10/21/2015 12:13 AM, Luyao Huang wrote:
Commit 1c24cfe9 fix the problem in virNumaSetPagePoolSize,
this patch just like it and fix the issue in another function.
when user use freepages and specify a invalid node or page size,
will get error like this:
# virsh freepages 0 1
error: Failed to open file
'/sys/devices/system/node/node0/hugepages/hugepages-1kB/free_hugepages':No such
file or directory
Add two checks to catch this and therefore produce much more
friendlier error messages.
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
src/util/virnuma.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
Now that I see things in their final form, one more suggestion which I
can take care of... You've repeated the same "if (!virFileExists)"
check and error messages for all 3 callers to virNumaGetHugePageInfo.
So I think moving that check into virNumaGetHugePageInfo would now be
appropriate (see the attached patch which I'll squash)
I'll also fix up the commit messages and push some time later today
John
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 815cbc1..cd000f9 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -553,6 +553,25 @@ virNumaGetHugePageInfo(int node,
page_size, "nr_hugepages") < 0)
goto cleanup;
+ if (!virFileExists(path)) {
+ if (node != -1) {
+ if (!virNumaNodeIsAvailable(node)) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("NUMA node %d is not available"),
+ node);
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("page size %u is not available on node
%d"),
+ page_size, node);
+ }
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("page size %u is not available"),
+ page_size);
+ }
+ goto cleanup;
+ }
+
if (virFileReadAll(path, 1024, &buf) < 0)
goto cleanup;
@@ -572,6 +591,25 @@ virNumaGetHugePageInfo(int node,
page_size, "free_hugepages") < 0)
goto cleanup;
+ if (!virFileExists(path)) {
+ if (node != -1) {
+ if (!virNumaNodeIsAvailable(node)) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("NUMA node %d is not available"),
+ node);
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("page size %u is not available on node
%d"),
+ page_size, node);
+ }
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("page size %u is not available"),
+ page_size);
+ }
+ goto cleanup;
+ }
+
if (virFileReadAll(path, 1024, &buf) < 0)
goto cleanup;
5:41 a.m.
New subject: [libvirt] [PATCHv2 3/3] util: Produce friendlier error message to user
On 10/21/2015 09:58 PM, John Ferlan wrote:
On 10/21/2015 12:13 AM, Luyao Huang wrote:
> Commit 1c24cfe9 fix the problem in virNumaSetPagePoolSize,
> this patch just like it and fix the issue in another function.
>
> when user use freepages and specify a invalid node or page size,
> will get error like this:
>
> # virsh freepages 0 1
> error: Failed to open file
'/sys/devices/system/node/node0/hugepages/hugepages-1kB/free_hugepages':No such
file or directory
>
> Add two checks to catch this and therefore produce much more
> friendlier error messages.
>
> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
> ---
> src/util/virnuma.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
Now that I see things in their final form, one more suggestion which I
can take care of... You've repeated the same "if (!virFileExists)"
check and error messages for all 3 callers to virNumaGetHugePageInfo.
So I think moving that check into virNumaGetHugePageInfo would now be
appropriate (see the attached patch which I'll squash)
I'll also fix up the commit messages and push some time later today
Oh, right, i should move the check in virNumaGetHugePageInfo to avoid
code duplicated. Your patch looks good to me. And thanks a lot for your
quick review and lots of help!
John
Luyao
> diff --git a/src/util/virnuma.c b/src/util/virnuma.c
> index 815cbc1..cd000f9 100644
> --- a/src/util/virnuma.c
> +++ b/src/util/virnuma.c
> @@ -553,6 +553,25 @@ virNumaGetHugePageInfo(int node,
> page_size, "nr_hugepages") <
0)
> goto cleanup;
>
> + if (!virFileExists(path)) {
> + if (node != -1) {
> + if (!virNumaNodeIsAvailable(node)) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("NUMA node %d is not available"),
> + node);
> + } else {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("page size %u is not available on node
%d"),
> + page_size, node);
> + }
> + } else {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("page size %u is not available"),
> + page_size);
> + }
> + goto cleanup;
> + }
> +
> if (virFileReadAll(path, 1024, &buf) < 0)
> goto cleanup;
>
> @@ -572,6 +591,25 @@ virNumaGetHugePageInfo(int node,
> page_size, "free_hugepages") <
0)
> goto cleanup;
>
> + if (!virFileExists(path)) {
> + if (node != -1) {
> + if (!virNumaNodeIsAvailable(node)) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("NUMA node %d is not available"),
> + node);
> + } else {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("page size %u is not available on node
%d"),
> + page_size, node);
> + }
> + } else {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("page size %u is not available"),
> + page_size);
> + }
> + goto cleanup;
> + }
> +
> if (virFileReadAll(path, 1024, &buf) < 0)
> goto cleanup;
>
>
3254
days inactive
3255
days old
5 comments
3 participants
participants (3)
-
John Ferlan -
lhuang -
Luyao Huang