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 A704EA0547; Mon, 9 Aug 2021 15:45:41 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6727F4068A; Mon, 9 Aug 2021 15:45:41 +0200 (CEST) Received: from mail-io1-f42.google.com (mail-io1-f42.google.com [209.85.166.42]) by mails.dpdk.org (Postfix) with ESMTP id C8AE44003C for ; Mon, 9 Aug 2021 15:45:40 +0200 (CEST) Received: by mail-io1-f42.google.com with SMTP id f11so27472197ioj.3 for ; Mon, 09 Aug 2021 06:45:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=bPQlQfVWygzyKE0xdgHQIZaZ5zRh/FmT81lpXq/Rpww=; b=PoJEKYDTf9DGQwyFP9SAhw7EIPi4244IqI2pRoNtayy8YHehjGfRZ5xots6TwqBcPY QfoVhtBIFipwQn6jblSiZyPF/RVjZjDf3kpkdA5MHhSiHR1haK4HC+jyHYFPU4QnQVmq POtiao3DDMRTNarM5MNIQmjFmaYd+RYi9R9/qPSNWS/zZpfAhd+XccDOjmk53E1c7zzH c1js75QBlpwHhpzLSyH+EN4G2oZymwr3K0TllvY1AbPY81za1LlsbRmFodiMGlMDguWQ nA+mkl5Y9LCotLhKCzjXRffDblPLp6FrxnduUoj4RgOuoEWc8p1t5GFb43yQYr1Vp4xR gy8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=bPQlQfVWygzyKE0xdgHQIZaZ5zRh/FmT81lpXq/Rpww=; b=PBIrJQoydJp1LWVh3sbwQJt4QS4rrFU+OFKPokMb9J6+1a2gWdZNJV5oqd3S3qhcHy Zb3PtmOL0Ew6dG3r+VkhD15PFlCJS95JBFnJQBZ68jMu1zhARE+SRRnbE/2xqFOZevNw cLOZU+dKYl0dMbc4qRCnxQj5fv8vDiYNIXwyhBq56yvEpp/j6u8JQaABKGSZnwGh5jCX cgRgkhIy25MY6lY20AhliRMa0Xi3++07gEcNYmuibe1yaRwFsORXdIrwiUlmiKgjbJgW 4vSoFoEKsklOLqjrppULCdwmmyzgUhVTmR5hPc2T6ZW6phW/3QrASwtD42RL60F4jRaJ E9nQ== X-Gm-Message-State: AOAM5339hdUvCS272wjpQM1EQzl8CH9oW62Yj38DRgsr8XJxhr2U4bro /rwJVQ9fNDeG6rDcNpudaOuWaVKGpXmxP9J5Of0= X-Google-Smtp-Source: ABdhPJxu6URAYy92LF9+9T4qY4Nd3awhc7eljIIg0Lk0zDZaNL6zWgxMK5VAMFWtAlo4besC6xwL9RcoSzT5u/vvaO8= X-Received: by 2002:a5d:93d1:: with SMTP id j17mr96181ioo.123.1628516740076; Mon, 09 Aug 2021 06:45:40 -0700 (PDT) MIME-Version: 1.0 References: <1705686.Ar61aiJpx4@thomas> <20210809112434.383123-1-john.levon@nutanix.com> In-Reply-To: <20210809112434.383123-1-john.levon@nutanix.com> From: Jerin Jacob Date: Mon, 9 Aug 2021 19:15:13 +0530 Message-ID: To: John Levon Cc: dpdk-dev , Anatoly Burakov , Dmitry Kozlyuk , Thomas Monjalon , David Marchand Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH] eal: allow hugetlbfs sub-directories 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" On Mon, Aug 9, 2021 at 4:54 PM John Levon wrote: > > get_hugepage_dir() was implemented in such a way that a --huge-dir > option had to exactly match the mountpoint, but there's no reason for > this restriction: DPDK might not be the only user of hugepages, and > shouldn't assume it owns an entire mountpoint. For example, if I have > /dev/hugepages/myapp, and /dev/hugepages/dpdk, I should be able to > specify: > > --huge-dir=/dev/hugepages/dpdk/ > > and have DPDK only use that sub-directory. > > Fix the implementation to allow a sub-directory within a suitable > hugetlbfs mountpoint to be specified, preferring the closest match. > > Signed-off-by: John Levon Cool feature. Could you add to the release note as well when this patch gets merged https://patches.dpdk.org/project/dpdk/patch/20210808192658.284547-1-thomas@monjalon.net/ > --- > v2: prefer closer matches > v3: checkpatch fixes > v4: fix docs, added tests > > app/test/test_eal_flags.c | 116 ++++++++++++------ > doc/guides/linux_gsg/linux_eal_parameters.rst | 3 +- > lib/eal/linux/eal_hugepage_info.c | 83 +++++++++---- > 3 files changed, 138 insertions(+), 64 deletions(-) > > diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c > index b4880ee802..1d18a0ba8f 100644 > --- a/app/test/test_eal_flags.c > +++ b/app/test/test_eal_flags.c > @@ -14,8 +14,9 @@ > #include > #include > #include > -#include > #include > +#include > +#include > #include > #include > > @@ -800,6 +801,9 @@ static int > test_misc_flags(void) > { > char hugepath[PATH_MAX] = {0}; > + char hugepath_dir[PATH_MAX] = {0}; > + char hugepath_dir2[PATH_MAX] = {0}; > + char hugepath_dir3[PATH_MAX] = {0}; > #ifdef RTE_EXEC_ENV_FREEBSD > /* BSD target doesn't support prefixes at this point */ > const char * prefix = ""; > @@ -810,6 +814,7 @@ test_misc_flags(void) > FILE * hugedir_handle = NULL; > char line[PATH_MAX] = {0}; > unsigned i, isempty = 1; > + > if (get_current_prefix(tmp, sizeof(tmp)) == NULL) { > printf("Error - unable to get current prefix!\n"); > return -1; > @@ -849,6 +854,20 @@ test_misc_flags(void) > } > #endif > > + snprintf(hugepath_dir, sizeof(hugepath_dir), "%s/dpdk.missing", hugepath); > + snprintf(hugepath_dir2, sizeof(hugepath_dir2), "%s/dpdk.dir", hugepath); > + > + if (mkdir(hugepath_dir2, 0700) != 0 && errno != EEXIST) { > + printf("Error - failed to mkdir(%s)\n", hugepath_dir2); > + return -1; > + } > + > + snprintf(hugepath_dir3, sizeof(hugepath_dir3), "%s/dpdk.dir/sub", hugepath); > + > + if (mkdir(hugepath_dir3, 0700) != 0 && errno != EEXIST) { > + printf("Error - failed to mkdir(%s)\n", hugepath_dir3); > + goto fail; > + } > > /* check that some general flags don't prevent things from working. > * All cases, apart from the first, app should run. > @@ -881,60 +900,66 @@ test_misc_flags(void) > /* With invalid --huge-dir */ > const char *argv9[] = {prgname, "-m", DEFAULT_MEM_SIZE, > "--file-prefix=hugedir", "--huge-dir", "invalid"}; > + /* With invalid --huge-dir sub-directory */ > + const char *argv10[] = {prgname, "-m", DEFAULT_MEM_SIZE, > + "--file-prefix=hugedir", "--huge-dir", hugepath_dir}; > + /* With valid --huge-dir sub-directory */ > + const char *argv11[] = {prgname, "-m", DEFAULT_MEM_SIZE, > + "--file-prefix=hugedir", "--huge-dir", hugepath_dir2}; > /* Secondary process with invalid --huge-dir (should run as flag has no > * effect on secondary processes) */ > - const char *argv10[] = {prgname, prefix, mp_flag, > + const char *argv12[] = {prgname, prefix, mp_flag, > "--huge-dir", "invalid"}; > > /* try running with base-virtaddr param */ > - const char *argv11[] = {prgname, "--file-prefix=virtaddr", > + const char *argv13[] = {prgname, "--file-prefix=virtaddr", > "--base-virtaddr=0x12345678"}; > > /* try running with --vfio-intr INTx flag */ > - const char *argv12[] = {prgname, "--file-prefix=intr", > + const char *argv14[] = {prgname, "--file-prefix=intr", > "--vfio-intr=legacy"}; > > /* try running with --vfio-intr MSI flag */ > - const char *argv13[] = {prgname, "--file-prefix=intr", > + const char *argv15[] = {prgname, "--file-prefix=intr", > "--vfio-intr=msi"}; > > /* try running with --vfio-intr MSI-X flag */ > - const char *argv14[] = {prgname, "--file-prefix=intr", > + const char *argv16[] = {prgname, "--file-prefix=intr", > "--vfio-intr=msix"}; > > /* try running with --vfio-intr invalid flag */ > - const char *argv15[] = {prgname, "--file-prefix=intr", > + const char *argv17[] = {prgname, "--file-prefix=intr", > "--vfio-intr=invalid"}; > > /* With process type as auto-detect */ > - const char * const argv16[] = {prgname, "--file-prefix=auto", > + const char * const argv18[] = {prgname, "--file-prefix=auto", > "--proc-type=auto"}; > > /* With process type as auto-detect with no-shconf */ > - const char * const argv17[] = {prgname, "--proc-type=auto", > + const char * const argv19[] = {prgname, "--proc-type=auto", > no_shconf, nosh_prefix, no_huge}; > > /* With process type as --create-uio-dev flag */ > - const char * const argv18[] = {prgname, "--file-prefix=uiodev", > + const char * const argv20[] = {prgname, "--file-prefix=uiodev", > "--create-uio-dev"}; > > /* run all tests also applicable to FreeBSD first */ > > if (launch_proc(argv0) == 0) { > printf("Error - process ran ok with invalid flag\n"); > - return -1; > + goto fail; > } > if (launch_proc(argv1) != 0) { > printf("Error - process did not run ok with --no-pci flag\n"); > - return -1; > + goto fail; > } > if (launch_proc(argv2) != 0) { > printf("Error - process did not run ok with -v flag\n"); > - return -1; > + goto fail; > } > if (launch_proc(argv6) != 0) { > printf("Error - process did not run ok with --no-shconf flag\n"); > - return -1; > + goto fail; > } > > #ifdef RTE_EXEC_ENV_FREEBSD > @@ -944,73 +969,88 @@ test_misc_flags(void) > > if (launch_proc(argv3) != 0) { > printf("Error - process did not run ok with --syslog flag\n"); > - return -1; > + goto fail; > } > if (launch_proc(argv4) == 0) { > printf("Error - process run ok with empty --syslog flag\n"); > - return -1; > + goto fail; > } > if (launch_proc(argv5) == 0) { > printf("Error - process run ok with invalid --syslog flag\n"); > - return -1; > + goto fail; > } > if (launch_proc(argv7) != 0) { > printf("Error - process did not run ok with --huge-dir flag\n"); > - return -1; > + goto fail; > } > if (launch_proc(argv8) == 0) { > printf("Error - process run ok with empty --huge-dir flag\n"); > - return -1; > + goto fail; > } > if (launch_proc(argv9) == 0) { > printf("Error - process run ok with invalid --huge-dir flag\n"); > - return -1; > + goto fail; > } > - if (launch_proc(argv10) != 0) { > - printf("Error - secondary process did not run ok with invalid --huge-dir flag\n"); > - return -1; > + if (launch_proc(argv10) == 0) { > + printf("Error - process run ok with invalid --huge-dir sub-dir flag\n"); > + goto fail; > } > if (launch_proc(argv11) != 0) { > - printf("Error - process did not run ok with --base-virtaddr parameter\n"); > - return -1; > + printf("Error - process did not run ok with --huge-dir subdir flag\n"); > + goto fail; > } > if (launch_proc(argv12) != 0) { > + printf("Error - secondary process did not run ok with invalid --huge-dir flag\n"); > + goto fail; > + } > + if (launch_proc(argv13) != 0) { > + printf("Error - process did not run ok with --base-virtaddr parameter\n"); > + goto fail; > + } > + if (launch_proc(argv14) != 0) { > printf("Error - process did not run ok with " > "--vfio-intr INTx parameter\n"); > - return -1; > + goto fail; > } > - if (launch_proc(argv13) != 0) { > + if (launch_proc(argv15) != 0) { > printf("Error - process did not run ok with " > "--vfio-intr MSI parameter\n"); > - return -1; > + goto fail; > } > - if (launch_proc(argv14) != 0) { > + if (launch_proc(argv16) != 0) { > printf("Error - process did not run ok with " > "--vfio-intr MSI-X parameter\n"); > - return -1; > + goto fail; > } > - if (launch_proc(argv15) == 0) { > + if (launch_proc(argv17) == 0) { > printf("Error - process run ok with " > "--vfio-intr invalid parameter\n"); > - return -1; > + goto fail; > } > - if (launch_proc(argv16) != 0) { > + if (launch_proc(argv18) != 0) { > printf("Error - process did not run ok with " > "--proc-type as auto parameter\n"); > - return -1; > + goto fail; > } > - if (launch_proc(argv17) != 0) { > + if (launch_proc(argv19) != 0) { > printf("Error - process did not run ok with " > "--proc-type and --no-shconf parameter\n"); > - return -1; > + goto fail; > } > - if (launch_proc(argv18) != 0) { > + if (launch_proc(argv20) != 0) { > printf("Error - process did not run ok with " > "--create-uio-dev parameter\n"); > - return -1; > + goto fail; > } > > + rmdir(hugepath_dir3); > + rmdir(hugepath_dir2); > return 0; > + > +fail: > + rmdir(hugepath_dir3); > + rmdir(hugepath_dir2); > + return -1; > } > > static int > diff --git a/doc/guides/linux_gsg/linux_eal_parameters.rst b/doc/guides/linux_gsg/linux_eal_parameters.rst > index bd3977cb3d..74df2611b5 100644 > --- a/doc/guides/linux_gsg/linux_eal_parameters.rst > +++ b/doc/guides/linux_gsg/linux_eal_parameters.rst > @@ -81,7 +81,8 @@ Memory-related options > > * ``--huge-dir `` > > - Use specified hugetlbfs directory instead of autodetected ones. > + Use specified hugetlbfs directory instead of autodetected ones. This can be > + a sub-directory within a hugetlbfs mountpoint. > > * ``--huge-unlink`` > > diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage_info.c > index d97792cade..9fb0e968db 100644 > --- a/lib/eal/linux/eal_hugepage_info.c > +++ b/lib/eal/linux/eal_hugepage_info.c > @@ -213,10 +213,19 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len) > const size_t pagesize_opt_len = sizeof(pagesize_opt) - 1; > const char split_tok = ' '; > char *splitstr[_FIELDNAME_MAX]; > + char found[PATH_MAX] = ""; > char buf[BUFSIZ]; > - int retval = -1; > const struct internal_config *internal_conf = > eal_get_internal_configuration(); > + struct stat st; > + > + /* > + * If the specified dir doesn't exist, we can't match it. > + */ > + if (internal_conf->hugepage_dir != NULL && > + stat(internal_conf->hugepage_dir, &st) != 0) { > + return -1; > + } > > FILE *fd = fopen(proc_mounts, "r"); > if (fd == NULL) > @@ -226,42 +235,66 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len) > default_size = get_default_hp_size(); > > while (fgets(buf, sizeof(buf), fd)){ > + const char *pagesz_str; > + > 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 */ > } > > - /* 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) > + if (strncmp(splitstr[FSTYPE], hugetlbfs_str, htlbfs_str_len) != 0) > continue; > > - if (strncmp(splitstr[FSTYPE], hugetlbfs_str, htlbfs_str_len) == 0){ > - const char *pagesz_str = strstr(splitstr[OPTIONS], pagesize_opt); > + pagesz_str = strstr(splitstr[OPTIONS], pagesize_opt); > > - /* 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 */ > + /* if no explicit page size, the default page size is compared */ > + if (pagesz_str == NULL) { > + if (hugepage_sz != default_size) > + continue; > + } > + /* 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) > + continue; > + } > + > + /* > + * If no --huge-dir option has been given, we're done. > + */ > + if (internal_conf->hugepage_dir == NULL) { > + strlcpy(found, splitstr[MOUNTPT], len); > + break; > + } > + > + /* > + * Ignore any mount that doesn't contain the --huge-dir > + * directory. > + */ > + if (strncmp(internal_conf->hugepage_dir, splitstr[MOUNTPT], > + strlen(splitstr[MOUNTPT])) != 0) { > + continue; > + } > + > + /* > + * We found a match, but only prefer it if it's a longer match > + * (so /mnt/1 is preferred over /mnt for matching /mnt/1/2)). > + */ > + if (strlen(splitstr[MOUNTPT]) > strlen(found)) > + strlcpy(found, splitstr[MOUNTPT], len); > } /* end while fgets */ > > fclose(fd); > - return retval; > + > + if (found[0] != '\0') { > + /* If needed, return the requested dir, not the mount point. */ > + strlcpy(hugedir, internal_conf->hugepage_dir != NULL ? > + internal_conf->hugepage_dir : found, len); > + return 0; > + } > + > + return -1; > } > > /* > -- > 2.30.2 >