Hello,

On Mon, Oct 11, 2021 at 10:57 AM Dmitry Kozlyuk [off-list ref] 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 <redacted>
Reviewed-by: Viacheslav Ovsiienko <redacted>
Reviewed-by: John Levon <redacted>

As you probably noticed, I merged John patch.
Could you rebase this series on the main branch please?

Two minor comments below:

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;
+       }

-- 
David Marchand