[v6,1/3] eal/linux: make hugetlbfs analysis reusable
Checks
Commit Message
get_hugepage_dir() searched for a hugetlbfs mount with a given page size
using handcraft parsing of /proc/mounts and mixing traversal logic with
selecting the needed entry. Separate code to enumerate hugetlbfs mounts
to eal_hugepage_mount_walk() taking a callback that can inspect already
parsed entries. Use mntent(3) API for parsing. This allows to reuse
enumeration logic in subsequent patches.
Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
Reviewed-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Reviewed-by: John Levon <john.levon@nutanix.com>
---
lib/eal/linux/eal_hugepage_info.c | 153 +++++++++++++++++++-----------
lib/eal/linux/eal_hugepage_info.h | 39 ++++++++
2 files changed, 135 insertions(+), 57 deletions(-)
create mode 100644 lib/eal/linux/eal_hugepage_info.h
Comments
Hello,
On Mon, Oct 11, 2021 at 10:57 AM Dmitry Kozlyuk <dkozlyuk@oss.nvidia.com> wrote:
>
> get_hugepage_dir() searched for a hugetlbfs mount with a given page size
> using handcraft parsing of /proc/mounts and mixing traversal logic with
> selecting the needed entry. Separate code to enumerate hugetlbfs mounts
> to eal_hugepage_mount_walk() taking a callback that can inspect already
> parsed entries. Use mntent(3) API for parsing. This allows to reuse
> enumeration logic in subsequent patches.
>
> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Reviewed-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> Reviewed-by: John Levon <john.levon@nutanix.com>
As you probably noticed, I merged John patch.
Could you rebase this series on the main branch please?
Two minor comments below:
> ---
> lib/eal/linux/eal_hugepage_info.c | 153 +++++++++++++++++++-----------
> lib/eal/linux/eal_hugepage_info.h | 39 ++++++++
> 2 files changed, 135 insertions(+), 57 deletions(-)
> create mode 100644 lib/eal/linux/eal_hugepage_info.h
>
> diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage_info.c
> index d97792cade..193282e779 100644
> --- a/lib/eal/linux/eal_hugepage_info.c
> +++ b/lib/eal/linux/eal_hugepage_info.c
> @@ -12,6 +12,7 @@
> #include <stdio.h>
> #include <fnmatch.h>
> #include <inttypes.h>
> +#include <mntent.h>
> #include <stdarg.h>
> #include <unistd.h>
> #include <errno.h>
> @@ -34,6 +35,7 @@
> #include "eal_private.h"
> #include "eal_internal_cfg.h"
> #include "eal_hugepages.h"
> +#include "eal_hugepage_info.h"
> #include "eal_filesystem.h"
>
> static const char sys_dir_path[] = "/sys/kernel/mm/hugepages";
> @@ -195,73 +197,110 @@ get_default_hp_size(void)
> return size;
> }
>
> -static int
> -get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
> +int
> +eal_hugepage_mount_walk(eal_hugepage_mount_walk_cb *cb, void *cb_arg)
> {
> - enum proc_mount_fieldnames {
> - DEVICE = 0,
> - MOUNTPT,
> - FSTYPE,
> - OPTIONS,
> - _FIELDNAME_MAX
> - };
> - static uint64_t default_size = 0;
> - const char proc_mounts[] = "/proc/mounts";
> - const char hugetlbfs_str[] = "hugetlbfs";
> - const size_t htlbfs_str_len = sizeof(hugetlbfs_str) - 1;
> - const char pagesize_opt[] = "pagesize=";
> - const size_t pagesize_opt_len = sizeof(pagesize_opt) - 1;
> - const char split_tok = ' ';
> - char *splitstr[_FIELDNAME_MAX];
> - char buf[BUFSIZ];
> - int retval = -1;
> - const struct internal_config *internal_conf =
> - eal_get_internal_configuration();
> -
> - FILE *fd = fopen(proc_mounts, "r");
> - if (fd == NULL)
> - rte_panic("Cannot open %s\n", proc_mounts);
> + static const char PATH[] = "/proc/mounts";
> + static const char OPTION[] = "pagesize";
Nit: please avoid PATH and OPTION as variable names.
All-uppercase words are usually for macros/defines in dpdk.
Plus, in PATH case, this is a well known shell variable.
> +
> + static uint64_t default_size;
> +
> + FILE *f = NULL;
> + struct mntent mntent;
> + char strings[PATH_MAX];
> + char *hugepage_sz_str;
> + uint64_t hugepage_sz;
> + bool stopped = false;
> + int ret = -1;
> +
> + f = setmntent(PATH, "r");
> + if (f == NULL) {
> + RTE_LOG(ERR, EAL, "%s(): setmntent(%s): %s\n",
> + __func__, PATH, strerror(errno));
> + goto exit;
We are in a rather generic helper function.
Error messages should be logged by callers of this helper, because the
caller knows better what the impact of failing to list mountpoints is.
In the helper itself, this log should probably be info or debug level.
If you think this error-level log should be kept in the helper, can
you make it a bit higher level so that users understand what is wrong
and what actions should be done to fix the situation?
> + }
>
> if (default_size == 0)
> default_size = get_default_hp_size();
>
> - while (fgets(buf, sizeof(buf), fd)){
> - if (rte_strsplit(buf, sizeof(buf), splitstr, _FIELDNAME_MAX,
> - split_tok) != _FIELDNAME_MAX) {
> - RTE_LOG(ERR, EAL, "Error parsing %s\n", proc_mounts);
> - break; /* return NULL */
> + ret = 0;
> + while (getmntent_r(f, &mntent, strings, sizeof(strings)) != NULL) {
> + if (strcmp(mntent.mnt_type, "hugetlbfs") != 0)
> + continue;
> +
> + hugepage_sz_str = hasmntopt(&mntent, OPTION);
> + if (hugepage_sz_str != NULL) {
> + hugepage_sz_str += strlen(OPTION) + 1; /* +1 for '=' */
> + hugepage_sz = rte_str_to_size(hugepage_sz_str);
> + if (hugepage_sz == 0) {
> + RTE_LOG(DEBUG, EAL, "Cannot parse hugepage size from '%s' for %s\n",
> + mntent.mnt_opts, mntent.mnt_dir);
> + continue;
> + }
> + } else {
> + RTE_LOG(DEBUG, EAL, "Hugepage filesystem at %s without %s option\n",
> + mntent.mnt_dir, OPTION);
> + hugepage_sz = default_size;
> }
>
> - /* we have a specified --huge-dir option, only examine that dir */
> - if (internal_conf->hugepage_dir != NULL &&
> - strcmp(splitstr[MOUNTPT], internal_conf->hugepage_dir) != 0)
> - continue;
> + if (cb(mntent.mnt_dir, hugepage_sz, cb_arg) != 0) {
> + stopped = true;
> + break;
> + }
> + }
>
> - if (strncmp(splitstr[FSTYPE], hugetlbfs_str, htlbfs_str_len) == 0){
> - const char *pagesz_str = strstr(splitstr[OPTIONS], pagesize_opt);
> + if (ferror(f) || (!stopped && !feof(f))) {
> + RTE_LOG(ERR, EAL, "%s(): getmntent_r(): %s\n",
> + __func__, strerror(errno));
Idem.
> + ret = -1;
> + goto exit;
> + }
Hello,
> [...]
> As you probably noticed, I merged John patch.
> Could you rebase this series on the main branch please?
Of course. Only would you accept that for now I'll just keep the tests John has added? With the new helper, directory selection logic can be tested isolated from parsing and mkdir(), but we have little time until RC1. Tests can be improved anytime.
> [...]
> We are in a rather generic helper function.
> Error messages should be logged by callers of this helper, because the
> caller knows better what the impact of failing to list mountpoints is.
> In the helper itself, this log should probably be info or debug level.
>
> If you think this error-level log should be kept in the helper, can you
> make it a bit higher level so that users understand what is wrong and what
> actions should be done to fix the situation?
No, I agree that DEBUG is better for system errors.
I don't often add generic code, but for Windos
we log all system errors as DEBUG and higher-level as ERR.
Will send v7, thanks.
@@ -12,6 +12,7 @@
#include <stdio.h>
#include <fnmatch.h>
#include <inttypes.h>
+#include <mntent.h>
#include <stdarg.h>
#include <unistd.h>
#include <errno.h>
@@ -34,6 +35,7 @@
#include "eal_private.h"
#include "eal_internal_cfg.h"
#include "eal_hugepages.h"
+#include "eal_hugepage_info.h"
#include "eal_filesystem.h"
static const char sys_dir_path[] = "/sys/kernel/mm/hugepages";
@@ -195,73 +197,110 @@ get_default_hp_size(void)
return size;
}
-static int
-get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
+int
+eal_hugepage_mount_walk(eal_hugepage_mount_walk_cb *cb, void *cb_arg)
{
- enum proc_mount_fieldnames {
- DEVICE = 0,
- MOUNTPT,
- FSTYPE,
- OPTIONS,
- _FIELDNAME_MAX
- };
- static uint64_t default_size = 0;
- const char proc_mounts[] = "/proc/mounts";
- const char hugetlbfs_str[] = "hugetlbfs";
- const size_t htlbfs_str_len = sizeof(hugetlbfs_str) - 1;
- const char pagesize_opt[] = "pagesize=";
- const size_t pagesize_opt_len = sizeof(pagesize_opt) - 1;
- const char split_tok = ' ';
- char *splitstr[_FIELDNAME_MAX];
- char buf[BUFSIZ];
- int retval = -1;
- const struct internal_config *internal_conf =
- eal_get_internal_configuration();
-
- FILE *fd = fopen(proc_mounts, "r");
- if (fd == NULL)
- rte_panic("Cannot open %s\n", proc_mounts);
+ static const char PATH[] = "/proc/mounts";
+ static const char OPTION[] = "pagesize";
+
+ static uint64_t default_size;
+
+ FILE *f = NULL;
+ struct mntent mntent;
+ char strings[PATH_MAX];
+ char *hugepage_sz_str;
+ uint64_t hugepage_sz;
+ bool stopped = false;
+ int ret = -1;
+
+ f = setmntent(PATH, "r");
+ if (f == NULL) {
+ RTE_LOG(ERR, EAL, "%s(): setmntent(%s): %s\n",
+ __func__, PATH, strerror(errno));
+ goto exit;
+ }
if (default_size == 0)
default_size = get_default_hp_size();
- while (fgets(buf, sizeof(buf), fd)){
- if (rte_strsplit(buf, sizeof(buf), splitstr, _FIELDNAME_MAX,
- split_tok) != _FIELDNAME_MAX) {
- RTE_LOG(ERR, EAL, "Error parsing %s\n", proc_mounts);
- break; /* return NULL */
+ ret = 0;
+ while (getmntent_r(f, &mntent, strings, sizeof(strings)) != NULL) {
+ if (strcmp(mntent.mnt_type, "hugetlbfs") != 0)
+ continue;
+
+ hugepage_sz_str = hasmntopt(&mntent, OPTION);
+ if (hugepage_sz_str != NULL) {
+ hugepage_sz_str += strlen(OPTION) + 1; /* +1 for '=' */
+ hugepage_sz = rte_str_to_size(hugepage_sz_str);
+ if (hugepage_sz == 0) {
+ RTE_LOG(DEBUG, EAL, "Cannot parse hugepage size from '%s' for %s\n",
+ mntent.mnt_opts, mntent.mnt_dir);
+ continue;
+ }
+ } else {
+ RTE_LOG(DEBUG, EAL, "Hugepage filesystem at %s without %s option\n",
+ mntent.mnt_dir, OPTION);
+ hugepage_sz = default_size;
}
- /* we have a specified --huge-dir option, only examine that dir */
- if (internal_conf->hugepage_dir != NULL &&
- strcmp(splitstr[MOUNTPT], internal_conf->hugepage_dir) != 0)
- continue;
+ if (cb(mntent.mnt_dir, hugepage_sz, cb_arg) != 0) {
+ stopped = true;
+ break;
+ }
+ }
- if (strncmp(splitstr[FSTYPE], hugetlbfs_str, htlbfs_str_len) == 0){
- const char *pagesz_str = strstr(splitstr[OPTIONS], pagesize_opt);
+ if (ferror(f) || (!stopped && !feof(f))) {
+ RTE_LOG(ERR, EAL, "%s(): getmntent_r(): %s\n",
+ __func__, strerror(errno));
+ ret = -1;
+ goto exit;
+ }
- /* if no explicit page size, the default page size is compared */
- if (pagesz_str == NULL){
- if (hugepage_sz == default_size){
- strlcpy(hugedir, splitstr[MOUNTPT], len);
- retval = 0;
- break;
- }
- }
- /* there is an explicit page size, so check it */
- else {
- uint64_t pagesz = rte_str_to_size(&pagesz_str[pagesize_opt_len]);
- if (pagesz == hugepage_sz) {
- strlcpy(hugedir, splitstr[MOUNTPT], len);
- retval = 0;
- break;
- }
- }
- } /* end if strncmp hugetlbfs */
- } /* end while fgets */
+exit:
+ if (f != NULL)
+ endmntent(f);
+ return ret;
+}
- fclose(fd);
- return retval;
+struct match_hugepage_mount_arg {
+ uint64_t hugepage_sz;
+ char *hugedir;
+ int hugedir_len;
+ bool done;
+};
+
+static int
+match_hugepage_mount(const char *path, uint64_t hugepage_sz, void *cb_arg)
+{
+ const struct internal_config *internal_conf =
+ eal_get_internal_configuration();
+ struct match_hugepage_mount_arg *arg = cb_arg;
+
+ /* we have a specified --huge-dir option, only examine that dir */
+ if (internal_conf->hugepage_dir != NULL &&
+ strcmp(path, internal_conf->hugepage_dir) != 0)
+ return 0;
+
+ if (hugepage_sz == arg->hugepage_sz) {
+ strlcpy(arg->hugedir, path, arg->hugedir_len);
+ arg->done = true;
+ return 1;
+ }
+
+ return 0;
+}
+
+static int
+get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
+{
+ struct match_hugepage_mount_arg arg = {
+ .hugepage_sz = hugepage_sz,
+ .hugedir = hugedir,
+ .hugedir_len = len,
+ .done = false,
+ };
+ int ret = eal_hugepage_mount_walk(match_hugepage_mount, &arg);
+ return ret == 0 && arg.done ? 0 : -1;
}
/*
new file mode 100644
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2021 NVIDIA Corporation & Affiliates
+ */
+
+#ifndef _EAL_HUGEPAGE_INFO_
+#define _EAL_HUGEPAGE_INFO_
+
+#include <stdint.h>
+
+/**
+ * Function called for each hugetlbfs mount point.
+ *
+ * @param path
+ * Mount point directory.
+ * @param hugepage_sz
+ * Hugepage size for the mount or default system hugepage size.
+ * @param arg
+ * User data.
+ *
+ * @return
+ * 0 to continue walking, 1 to stop.
+ */
+typedef int (eal_hugepage_mount_walk_cb)(const char *path, uint64_t hugepage_sz,
+ void *arg);
+
+/**
+ * Enumerate hugetlbfs mount points.
+ *
+ * @param cb
+ * Function called for each mount point.
+ * @param cb_arg
+ * User data passed to the callback.
+ *
+ * @return
+ * 0 on success, negative on failure.
+ */
+int eal_hugepage_mount_walk(eal_hugepage_mount_walk_cb *cb, void *cb_arg);
+
+#endif /* _EAL_HUGEPAGE_INFO_ */