[libvirt] [PATCH 0/4] Tiny updates for virresctrl
The first patch is the only interesting one. Martin Kletzander (4): util: Check for empty allocation instead of just NULL pointer util: Use "resctrl" instead of "resctrlfs" spelling util: Make it possible for virResctrlAllocSetMask to replace existing mask util: Remove unused variable in virResctrlGetInfo src/qemu/qemu_process.c | 2 +- src/util/virresctrl.c | 19 +++++++++---------- 2 files changed, 10 insertions(+), 11 deletions(-) -- 2.16.1
When working on the CAT series one of the changes was that the pointer got allocated in another part of the code, even when resctrl was not available on the host system. However this one particular place neglected that so it needs to be fixed in order to get the proper error message when requesting <cachetune/> on HW with no support for it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 754820ee463e..03218a481dc0 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1472,7 +1472,7 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (!alloc) return 0; - if (!resctrl) { + if (virResctrlInfoIsEmpty(resctrl)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Resource control is not supported on this host")); return -1; -- 2.16.1
Pointed out during review on one or two places, but it actually appears in lot more places. So let's be consistent. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_process.c | 2 +- src/util/virresctrl.c | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3a697de037e1..0577f4c35d08 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5936,7 +5936,7 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessSetupEmulator(vm) < 0) goto cleanup; - VIR_DEBUG("Setting up resctrlfs"); + VIR_DEBUG("Setting up resctrl"); if (qemuProcessResctrlCreate(driver, vm) < 0) goto cleanup; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 03218a481dc0..a1d09c547e45 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -76,7 +76,7 @@ struct _virResctrlInfoPerType { unsigned int max_cache_id; /* In order to be self-sufficient we need size information per cache. - * Funnily enough, one of the outcomes of the resctrlfs design is that it + * Funnily enough, one of the outcomes of the resctrl design is that it * does not account for different sizes per cache on the same level. So * for the sake of easiness, let's copy that, for now. */ unsigned long long size; @@ -302,12 +302,12 @@ virResctrlLockInternal(int op) int fd = open(SYSFS_RESCTRL_PATH, O_DIRECTORY | O_CLOEXEC); if (fd < 0) { - virReportSystemError(errno, "%s", _("Cannot open resctrlfs")); + virReportSystemError(errno, "%s", _("Cannot open resctrl")); return -1; } if (flock(fd, op) < 0) { - virReportSystemError(errno, "%s", _("Cannot lock resctrlfs")); + virReportSystemError(errno, "%s", _("Cannot lock resctrl")); VIR_FORCE_CLOSE(fd); return -1; } @@ -328,7 +328,7 @@ static inline int virResctrlLockWrite(void) { virReportSystemError(ENOSYS, "%s", - _("resctrlfs not supported on this platform")); + _("resctrl not supported on this platform")); return -1; } @@ -347,11 +347,11 @@ virResctrlUnlock(int fd) /* The lock gets unlocked by closing the fd, which we need to do anyway in * order to clean up properly */ if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, "%s", _("Cannot close resctrlfs")); + virReportSystemError(errno, "%s", _("Cannot close resctrl")); /* Trying to save the already broken */ if (flock(fd, LOCK_UN) < 0) - virReportSystemError(errno, "%s", _("Cannot unlock resctrlfs")); + virReportSystemError(errno, "%s", _("Cannot unlock resctrl")); return -1; } #endif /* ! __linux__ */ @@ -486,7 +486,7 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) if (i_level->types[type]) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Duplicate cache type in resctrlfs for level %u"), + _("Duplicate cache type in resctrl for level %u"), level); goto cleanup; } -- 2.16.1
This wil be used in the future, but it makes sense for now as well. It makes sure there is no mask leftover that would leak. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index a1d09c547e45..89b1382b6857 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1239,6 +1239,7 @@ virResctrlAllocSetMask(virResctrlAllocPerTypePtr a_type, cache - a_type->nmasks + 1) < 0) return -1; + virBitmapFree(a_type->masks[cache]); a_type->masks[cache] = mask; return 0; -- 2.16.1
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 89b1382b6857..684cb22fd8a2 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -392,7 +392,6 @@ int virResctrlGetInfo(virResctrlInfoPtr resctrl) { DIR *dirp = NULL; - char *info_path = NULL; char *endptr = NULL; char *tmp_str = NULL; int ret = -1; @@ -507,7 +506,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) ret = 0; cleanup: VIR_DIR_CLOSE(dirp); - VIR_FREE(info_path); if (i_type) VIR_FREE(i_type->cbm_mask); VIR_FREE(i_type); -- 2.16.1
We are skipping non-directories under /sys/fs/resctrl/info/ since those are not interesting for us. However in tests it can sometimes happen that ent->d_type is 0 instead of 4 (DT_DIR) for directories. I've seen it fail on two machines. Different machines, different systems, I cannot reproduce it even using the same setup. So one of the ways how to work around this is call stat() on it. The other one is not checking if it is a directory since we'll find out eventually when we want to read some files underneath it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 684cb22fd8a2..0e09c67802a6 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -410,10 +410,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { VIR_DEBUG("Parsing info type '%s'", ent->d_name); - - if (ent->d_type != DT_DIR) - continue; - if (ent->d_name[0] != 'L') continue; @@ -436,19 +432,28 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) rv = virFileReadValueUint(&i_type->control.max_allocation, SYSFS_RESCTRL_PATH "/info/%s/num_closids", ent->d_name); - if (rv == -2) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot get num_closids from resctrl cache info")); - if (rv < 0) + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, + * but we can scan further */ + VIR_WARN("The path '" SYSFS_RESCTRL_PATH "/info/%s/num_closids' " + "does not exist", + ent->d_name); + } else if (rv < 0) { + /* Other failures are fatal, so just quit */ goto cleanup; + } rv = virFileReadValueString(&i_type->cbm_mask, SYSFS_RESCTRL_PATH "/info/%s/cbm_mask", ent->d_name); - if (rv == -2) + if (rv == -2) { + /* If the previous file exists, so should this one. Hence -2 is + * fatal in this case as well - the kernel interface might've + * changed too much or something else is wrong. */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot get cbm_mask from resctrl cache info")); + } if (rv < 0) goto cleanup; -- 2.16.1
We are skipping non-directories under /sys/fs/resctrl/info/ since those are not interesting for us. However in tests it can sometimes happen that ent->d_type is 0 instead of 4 (DT_DIR) for directories. I've seen it fail on two machines. Different machines, different systems, I cannot reproduce it even using the same setup. So one of the ways how to work around this is call stat() on it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 684cb22fd8a2..12dda76f9d33 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -409,11 +409,32 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) } while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { + struct stat st; + char *path = NULL; + VIR_DEBUG("Parsing info type '%s'", ent->d_name); + if (virAsprintf(&path, SYSFS_RESCTRL_PATH "/info/%s", ent->d_name) < 0) + continue; + + if (stat(path, &st) < 0) { + virReportSystemError(errno, _("Cannot stat '%s'"), path); + VIR_FREE(path); + continue; + } + VIR_FREE(path); + + /* + * So this doesn't work on some machines when we're mocking syscalls in tests + if (ent->d_type != DT_DIR) continue; + But the following does, for some reason. + */ + if ((st.st_mode & S_IFMT) != S_IFDIR) + continue; + if (ent->d_name[0] != 'L') continue; -- 2.16.1
We are skipping non-directories under /sys/fs/resctrl/(info/) since those are not interesting for us. However in tests it can sometimes happen that ent->d_type is 0 instead of 4 (DT_DIR) for directories. I've seen it fail on two machines. Different machines, different systems, I cannot reproduce it even using the same setup. So one of the ways how to work around this is call stat() on it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 684cb22fd8a2..2c1d07ff3a25 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -409,9 +409,30 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) } while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { + struct stat st; + char *path = NULL; + VIR_DEBUG("Parsing info type '%s'", ent->d_name); - if (ent->d_type != DT_DIR) + if (virAsprintf(&path, SYSFS_RESCTRL_PATH "/info/%s", ent->d_name) < 0) + continue; + + if (stat(path, &st) < 0) { + virReportSystemError(errno, _("Cannot stat '%s'"), path); + VIR_FREE(path); + continue; + } + VIR_FREE(path); + + /* + * So this doesn't work on some machines when we're mocking syscalls in tests + * + * if (ent->d_type != DT_DIR) + * continue; + * + * But the following does, for some reason. + */ + if ((st.st_mode & S_IFMT) != S_IFDIR) continue; if (ent->d_name[0] != 'L') @@ -1169,6 +1190,32 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) goto error; while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH)) > 0) { + struct stat st; + char *path = NULL; + + VIR_DEBUG("Parsing info type '%s'", ent->d_name); + + if (virAsprintf(&path, SYSFS_RESCTRL_PATH "/%s", ent->d_name) < 0) + continue; + + if (stat(path, &st) < 0) { + virReportSystemError(errno, _("Cannot stat '%s'"), path); + VIR_FREE(path); + continue; + } + VIR_FREE(path); + + /* + * So this doesn't work on some machines when we're mocking syscalls in tests + * + * if (ent->d_type != DT_DIR) + * continue; + * + * But the following does, for some reason. + */ + if ((st.st_mode & S_IFMT) != S_IFDIR) + continue; + if (ent->d_type != DT_DIR) continue; -- 2.16.1
We are skipping non-directories under /sys/fs/resctrl/(info/) since those are not interesting for us. However in tests it can sometimes happen that ent->d_type is 0 instead of 4 (DT_DIR) for directories. I've seen it fail on two machines. Different machines, different systems, I cannot reproduce it even using the same setup. So one of the ways how to work around this is call stat() on it. The other one is not checking if it is a directory since we'll find out eventually when we want to read some files underneath it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 684cb22fd8a2..ad4294c26b1e 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -410,10 +410,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { VIR_DEBUG("Parsing info type '%s'", ent->d_name); - - if (ent->d_type != DT_DIR) - continue; - if (ent->d_name[0] != 'L') continue; @@ -436,19 +432,28 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) rv = virFileReadValueUint(&i_type->control.max_allocation, SYSFS_RESCTRL_PATH "/info/%s/num_closids", ent->d_name); - if (rv == -2) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot get num_closids from resctrl cache info")); - if (rv < 0) + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, + * but we can scan further */ + VIR_WARN("The path '" SYSFS_RESCTRL_PATH "/info/%s/num_closids' " + "does not exist", + ent->d_name); + } else if (rv < 0) { + /* Other failures are fatal, so just quit */ goto cleanup; + } rv = virFileReadValueString(&i_type->cbm_mask, SYSFS_RESCTRL_PATH "/info/%s/cbm_mask", ent->d_name); - if (rv == -2) + if (rv == -2) { + /* If the previous file exists, so should this one. Hence -2 is + * fatal in this case as well - the kernel interface might've + * changed too much or something else is wrong. */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot get cbm_mask from resctrl cache info")); + } if (rv < 0) goto cleanup; @@ -1169,9 +1174,6 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) goto error; while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH)) > 0) { - if (ent->d_type != DT_DIR) - continue; - if (STREQ(ent->d_name, "info")) continue; -- 2.16.1
On Mon, Jan 29, 2018 at 05:45:44PM +0100, Martin Kletzander wrote:
We are skipping non-directories under /sys/fs/resctrl/(info/) since those are not interesting for us. However in tests it can sometimes happen that ent->d_type is 0 instead of 4 (DT_DIR) for directories.
I've seen it fail on two machines. Different machines, different systems, I cannot reproduce it even using the same setup. So one of the ways how to work around this is call stat() on it. The other one is not checking if it is a directory since we'll find out eventually when we want to read some files underneath it.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 684cb22fd8a2..ad4294c26b1e 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -410,10 +410,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { VIR_DEBUG("Parsing info type '%s'", ent->d_name); - - if (ent->d_type != DT_DIR) - continue; - if (ent->d_name[0] != 'L') continue;
@@ -436,19 +432,28 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) rv = virFileReadValueUint(&i_type->control.max_allocation, SYSFS_RESCTRL_PATH "/info/%s/num_closids", ent->d_name); - if (rv == -2) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot get num_closids from resctrl cache info")); - if (rv < 0) + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, + * but we can scan further */ + VIR_WARN("The path '" SYSFS_RESCTRL_PATH "/info/%s/num_closids' " + "does not exist", + ent->d_name); + } else if (rv < 0) { + /* Other failures are fatal, so just quit */ goto cleanup; + }
rv = virFileReadValueString(&i_type->cbm_mask, SYSFS_RESCTRL_PATH "/info/%s/cbm_mask", ent->d_name); - if (rv == -2) + if (rv == -2) { + /* If the previous file exists, so should this one. Hence -2 is + * fatal in this case as well - the kernel interface might've
non-fatal, I assume
+ * changed too much or something else is wrong. */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot get cbm_mask from resctrl cache info")); + } if (rv < 0) goto cleanup;
ACK Jan
On Mon, Jan 29, 2018 at 05:54:03PM +0100, Ján Tomko wrote:
On Mon, Jan 29, 2018 at 05:45:44PM +0100, Martin Kletzander wrote:
We are skipping non-directories under /sys/fs/resctrl/(info/) since those are not interesting for us. However in tests it can sometimes happen that ent->d_type is 0 instead of 4 (DT_DIR) for directories.
I've seen it fail on two machines. Different machines, different systems, I cannot reproduce it even using the same setup. So one of the ways how to work around this is call stat() on it. The other one is not checking if it is a directory since we'll find out eventually when we want to read some files underneath it.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 684cb22fd8a2..ad4294c26b1e 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -410,10 +410,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { VIR_DEBUG("Parsing info type '%s'", ent->d_name); - - if (ent->d_type != DT_DIR) - continue; - if (ent->d_name[0] != 'L') continue;
@@ -436,19 +432,28 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) rv = virFileReadValueUint(&i_type->control.max_allocation, SYSFS_RESCTRL_PATH "/info/%s/num_closids", ent->d_name); - if (rv == -2) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot get num_closids from resctrl cache info")); - if (rv < 0) + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, + * but we can scan further */ + VIR_WARN("The path '" SYSFS_RESCTRL_PATH "/info/%s/num_closids' " + "does not exist", + ent->d_name); + } else if (rv < 0) { + /* Other failures are fatal, so just quit */ goto cleanup; + }
rv = virFileReadValueString(&i_type->cbm_mask, SYSFS_RESCTRL_PATH "/info/%s/cbm_mask", ent->d_name); - if (rv == -2) + if (rv == -2) { + /* If the previous file exists, so should this one. Hence -2 is + * fatal in this case as well - the kernel interface might've
non-fatal, I assume
No, it really is fatal, it's just that it errors out in the next condition, which looks ugly, I'll fix the comment at least, since changing the code would need changes in later parts as well in order to stay consistent. Thanks for the review.
+ * changed too much or something else is wrong. */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot get cbm_mask from resctrl cache info")); + } if (rv < 0) goto cleanup;
ACK
Jan
On Mon, Jan 29, 2018 at 04:31:40PM +0100, Martin Kletzander wrote:
The first patch is the only interesting one.
Martin Kletzander (4): util: Check for empty allocation instead of just NULL pointer util: Use "resctrl" instead of "resctrlfs" spelling util: Make it possible for virResctrlAllocSetMask to replace existing mask util: Remove unused variable in virResctrlGetInfo
src/qemu/qemu_process.c | 2 +- src/util/virresctrl.c | 19 +++++++++---------- 2 files changed, 10 insertions(+), 11 deletions(-)
ACK to the original four. Jan
participants (2)
-
Ján Tomko -
Martin Kletzander