[v5,1/3] eal/linux: make hugetlbfs analysis reusable

Message ID 20210921081632.858873-2-dkozlyuk@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series eal: add memory pre-allocation from existing files |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Dmitry Kozlyuk Sept. 21, 2021, 8:16 a.m. UTC
  From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>

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>
---
 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

John Levon Sept. 22, 2021, 1:52 p.m. UTC | #1
On Tue, Sep 21, 2021 at 11:16:30AM +0300, dkozlyuk@nvidia.com wrote:

> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> 
> 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>

regards
john
  
Thomas Monjalon Oct. 5, 2021, 5:36 p.m. UTC | #2
21/09/2021 10:16, dkozlyuk@oss.nvidia.com:
> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> 
> 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>

First version was sent in July.
Anatoly, please are you available to review?

> +++ b/lib/eal/linux/eal_hugepage_info.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 NVIDIA CORPORATION & AFFILIATES.

Please use this exact format:

Copyright (c) 2021 NVIDIA Corporation & Affiliates
  
John Levon Oct. 8, 2021, 3:33 p.m. UTC | #3
On Tue, Oct 05, 2021 at 07:36:21PM +0200, Thomas Monjalon wrote:

> 21/09/2021 10:16, dkozlyuk@oss.nvidia.com:
> > From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> > 
> > 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>
> 
> First version was sent in July.
> Anatoly, please are you available to review?

Any progress on these? Since now my original patch ("eal: allow hugetlbfs
sub-directories") is going to have to wait behind this series, since nobody
responded to review of the last version.

Is it usual in DPDK to have to wait months?

thanks
john
  
Dmitry Kozlyuk Oct. 8, 2021, 3:50 p.m. UTC | #4
Hello John,

> Any progress on these? Since now my original patch ("eal: allow hugetlbfs
> sub-directories") is going to have to wait behind this series, since nobody
> responded to review of the last version.

Your patch does not directly depend on this one and can be merged earlier.
Maybe my previous comment wasn't clear enough. If your patch is merged first,
I will rebase mine, updating your added code to use the new functions.
If my patch is merged first, it's vice versa. For EAL part it has my ack since v3.

> Is it usual in DPDK to have to wait months?

It is not normal, but admittedly happens :(
  

Patch

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";
+
+	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;
 }
 
 /*
diff --git a/lib/eal/linux/eal_hugepage_info.h b/lib/eal/linux/eal_hugepage_info.h
new file mode 100644
index 0000000000..c7efa37c66
--- /dev/null
+++ b/lib/eal/linux/eal_hugepage_info.h
@@ -0,0 +1,39 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 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_ */