From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A8954A0C4D; Wed, 13 Oct 2021 10:16:50 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 96B4541180; Wed, 13 Oct 2021 10:16:50 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 6EE6841162 for ; Wed, 13 Oct 2021 10:16:48 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1634113008; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=6cUgCCL7iU/jIYVfcWhk+kTdnMrL7EkrzOJYOBaxqGU=; b=DDBd6WPWmb0drGrVSakt96jEEEMX2eUTgrN/B8ahCDyr3ZbLXshz79FOMP5d56pddHzed+ jkoIfGqu51Xjk0SYyLvz3g6ckc9lx76IKiw6+zAqRcIjqmHrPi59p0iJJI0FHhqWiJ1DI1 Hv/vrBWmK8Yyd6u5dkTt8eOu7LDsNDo= Received: from mail-lf1-f70.google.com (mail-lf1-f70.google.com [209.85.167.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-502-dPFBxhLONxqv3QpzH9n7Kg-1; Wed, 13 Oct 2021 04:16:46 -0400 X-MC-Unique: dPFBxhLONxqv3QpzH9n7Kg-1 Received: by mail-lf1-f70.google.com with SMTP id s8-20020ac25c48000000b003faf62e104eso1352298lfp.22 for ; Wed, 13 Oct 2021 01:16:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=6cUgCCL7iU/jIYVfcWhk+kTdnMrL7EkrzOJYOBaxqGU=; b=ltOmRoFN+Wvai8d5dCICDuztGf6Je9wBKakHUO7NOPBtNs6vvjqYRLaFDxkWTcKlmD 6WcxvAlcMKfftE3OMpBwVeezxeJmOzzq15+XvtY3+3fT41vax03V4o+TO4/L2SMhkFLW T9MSIrQNXOz97whdZPxBK5tvO20Og3wRw5hBvs7rXTWndXIlGIHRhY0LaST7KYnfUwvE agUlWbLMzsRHqFDuZZ4l529vhlrx1jaIuDZNNTd4wL62O9cwTDAU2J2DB3Q1PsF6xv4T Rr1lEaxFsJx+w2MaXikWU/JEeEMHCSfY1IMOXbMZAaBhsj+rfYAJZE3+g9GreZ+eWzTY BYaQ== X-Gm-Message-State: AOAM5331p+0XBtrO24sTNqliTxN0XHZC/jGBPfUZ21yLSXkxzQN/4lsf ADbICgtNOdUUX+083T78s5FTtF6KB53CS9f7csHtb5QS0tpXBjf09QAHxvxsfWRLAWUPGjSPDuf KEwU9un6f8oUT9LBm9HE= X-Received: by 2002:a2e:8584:: with SMTP id b4mr36071351lji.477.1634113005134; Wed, 13 Oct 2021 01:16:45 -0700 (PDT) X-Google-Smtp-Source: ABdhPJy6MV8E9FxxUZpESJoeyVgLmzr2oqWaEQhGjzI/gzYT1xJkLDswXGybXYs1rnpe+nY7ch/4QsEb9najyo3CwBI= X-Received: by 2002:a2e:8584:: with SMTP id b4mr36071324lji.477.1634113004886; Wed, 13 Oct 2021 01:16:44 -0700 (PDT) MIME-Version: 1.0 References: <20210921081632.858873-1-dkozlyuk@nvidia.com> <20211011085644.2716490-1-dkozlyuk@nvidia.com> <20211011085644.2716490-2-dkozlyuk@nvidia.com> In-Reply-To: <20211011085644.2716490-2-dkozlyuk@nvidia.com> From: David Marchand Date: Wed, 13 Oct 2021 10:16:33 +0200 Message-ID: To: Dmitry Kozlyuk Cc: dev , Viacheslav Ovsiienko , John Levon Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v6 1/3] eal/linux: make hugetlbfs analysis reusable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello, On Mon, Oct 11, 2021 at 10:57 AM Dmitry Kozlyuk 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 > Reviewed-by: Viacheslav Ovsiienko > Reviewed-by: John Levon 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 > #include > #include > +#include > #include > #include > #include > @@ -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