Re: [dpdk-dev] [PATCH v6 1/3] eal/linux: make hugetlbfs analysis reusable

[David Marchand <david.marchand@redhat.com>]

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


-- 
David Marchand