[dpdk-dev] [PATCH] eal: allow hugetlbfs sub-directories

[dpdk-dev] [PATCH] eal: allow hugetlbfs sub-directories

[John Levon]

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. Fix the implementation to allow a sub-directory within a
suitable hugetlbfs mountpoint to be specified.

Signed-off-by: John Levon <john.levon@nutanix.com>
---
 lib/eal/linux/eal_hugepage_info.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage_info.c
index d97792cad..d7e9918f8 100644
--- a/lib/eal/linux/eal_hugepage_info.c
+++ b/lib/eal/linux/eal_hugepage_info.c
@@ -226,16 +226,29 @@ 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 *dir;
+
 		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)
-			continue;
+		dir = splitstr[MOUNTPT];
+
+		/*
+		 * If a --huge-dir option has been specified, only examine
+		 * mounts that contain that directory, and make sure to return
+		 * the directory, not the mount.
+		 */
+		if (internal_conf->hugepage_dir != NULL) {
+			if (strncmp(internal_conf->hugepage_dir,
+				splitstr[MOUNTPT],
+				strlen(splitstr[MOUNTPT])) != 0)
+				continue;
+
+			dir = internal_conf->hugepage_dir;
+		}
 
 		if (strncmp(splitstr[FSTYPE], hugetlbfs_str, htlbfs_str_len) == 0){
 			const char *pagesz_str = strstr(splitstr[OPTIONS], pagesize_opt);
@@ -243,7 +256,7 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
 			/* 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);
+					strlcpy(hugedir, dir, len);
 					retval = 0;
 					break;
 				}
@@ -252,7 +265,7 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
 			else {
 				uint64_t pagesz = rte_str_to_size(&pagesz_str[pagesize_opt_len]);
 				if (pagesz == hugepage_sz) {
-					strlcpy(hugedir, splitstr[MOUNTPT], len);
+					strlcpy(hugedir, dir, len);
 					retval = 0;
 					break;
 				}
-- 
2.25.1

[dpdk-dev] [PATCH RESEND] eal: allow hugetlbfs sub-directories

[John Levon]

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. Fix the implementation to allow a sub-directory within a
suitable hugetlbfs mountpoint to be specified.

Signed-off-by: John Levon <john.levon@nutanix.com>
---
 lib/eal/linux/eal_hugepage_info.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage_info.c
index d97792cad..d7e9918f8 100644
--- a/lib/eal/linux/eal_hugepage_info.c
+++ b/lib/eal/linux/eal_hugepage_info.c
@@ -226,16 +226,29 @@ 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 *dir;
+
 		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)
-			continue;
+		dir = splitstr[MOUNTPT];
+
+		/*
+		 * If a --huge-dir option has been specified, only examine
+		 * mounts that contain that directory, and make sure to return
+		 * the directory, not the mount.
+		 */
+		if (internal_conf->hugepage_dir != NULL) {
+			if (strncmp(internal_conf->hugepage_dir,
+				splitstr[MOUNTPT],
+				strlen(splitstr[MOUNTPT])) != 0)
+				continue;
+
+			dir = internal_conf->hugepage_dir;
+		}
 
 		if (strncmp(splitstr[FSTYPE], hugetlbfs_str, htlbfs_str_len) == 0){
 			const char *pagesz_str = strstr(splitstr[OPTIONS], pagesize_opt);
@@ -243,7 +256,7 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
 			/* 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);
+					strlcpy(hugedir, dir, len);
 					retval = 0;
 					break;
 				}
@@ -252,7 +265,7 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
 			else {
 				uint64_t pagesz = rte_str_to_size(&pagesz_str[pagesize_opt_len]);
 				if (pagesz == hugepage_sz) {
-					strlcpy(hugedir, splitstr[MOUNTPT], len);
+					strlcpy(hugedir, dir, len);
 					retval = 0;
 					break;
 				}
-- 
2.25.1

Re: [dpdk-dev] [PATCH RESEND] eal: allow hugetlbfs sub-directories

[David Marchand]

Hello Anatoly,

On Fri, Jun 25, 2021 at 12:46 PM John Levon <john.levon@nutanix.com> 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. Fix the implementation to allow a sub-directory within a
> suitable hugetlbfs mountpoint to be specified.

Review please.


>
> Signed-off-by: John Levon <john.levon@nutanix.com>


-- 
David Marchand

Re: [dpdk-dev] [PATCH RESEND] eal: allow hugetlbfs sub-directories

[Dmitry Kozlyuk]

2021-06-25 11:44 (UTC+0100), John Levon:
> 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. Fix the implementation to allow a sub-directory within a
> suitable hugetlbfs mountpoint to be specified.
> 
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
>  lib/eal/linux/eal_hugepage_info.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage_info.c
> index d97792cad..d7e9918f8 100644
> --- a/lib/eal/linux/eal_hugepage_info.c
> +++ b/lib/eal/linux/eal_hugepage_info.c
> @@ -226,16 +226,29 @@ 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 *dir;
> +
>  		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)
> -			continue;
> +		dir = splitstr[MOUNTPT];
> +
> +		/*
> +		 * If a --huge-dir option has been specified, only examine
> +		 * mounts that contain that directory, and make sure to return
> +		 * the directory, not the mount.
> +		 */
> +		if (internal_conf->hugepage_dir != NULL) {
> +			if (strncmp(internal_conf->hugepage_dir,
> +				splitstr[MOUNTPT],
> +				strlen(splitstr[MOUNTPT])) != 0)
> +				continue;
> +
> +			dir = internal_conf->hugepage_dir;
> +		}

Suppose there are /mnt/huge and /mnt/huge-2M mounted with -o pagesize=2M.
If --huge-dir=/mnt/huge-2M is specified, this code will select /mnt/huge.
The code would be shorter with `splitstr[MOUNTPT]` -> `dir`.

Re: [dpdk-dev] [PATCH RESEND] eal: allow hugetlbfs sub-directories

[John Levon]

On Wed, Jul 07, 2021 at 11:06:05PM +0300, Dmitry Kozlyuk 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. Fix the implementation to allow a sub-directory within a
> > suitable hugetlbfs mountpoint to be specified.
> > 
> > Signed-off-by: John Levon <john.levon@nutanix.com>
> > ---
> >  lib/eal/linux/eal_hugepage_info.c | 25 +++++++++++++++++++------
> >  1 file changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage_info.c
> > index d97792cad..d7e9918f8 100644
> > --- a/lib/eal/linux/eal_hugepage_info.c
> > +++ b/lib/eal/linux/eal_hugepage_info.c
> > @@ -226,16 +226,29 @@ 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 *dir;
> > +
> >  		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)
> > -			continue;
> > +		dir = splitstr[MOUNTPT];
> > +
> > +		/*
> > +		 * If a --huge-dir option has been specified, only examine
> > +		 * mounts that contain that directory, and make sure to return
> > +		 * the directory, not the mount.
> > +		 */
> > +		if (internal_conf->hugepage_dir != NULL) {
> > +			if (strncmp(internal_conf->hugepage_dir,
> > +				splitstr[MOUNTPT],
> > +				strlen(splitstr[MOUNTPT])) != 0)
> > +				continue;
> > +
> > +			dir = internal_conf->hugepage_dir;
> > +		}
> 
> Suppose there are /mnt/huge and /mnt/huge-2M mounted with -o pagesize=2M.
> If --huge-dir=/mnt/huge-2M is specified, this code will select /mnt/huge.

Yeah, good spot. I'll change it to prefer closer matches or something.

> The code would be shorter with `splitstr[MOUNTPT]` -> `dir`.

Sure.

regards
john

[dpdk-dev] [PATCH] eal: allow hugetlbfs sub-directories

[John Levon]

v2: fix to prefer closest match

[dpdk-dev] [PATCH] eal: allow hugetlbfs sub-directories

[John Levon]

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. 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 <john.levon@nutanix.com>
---
 lib/eal/linux/eal_hugepage_info.c | 74 +++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 23 deletions(-)

diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage_info.c
index d97792cad..b5f08e94d 100644
--- a/lib/eal/linux/eal_hugepage_info.c
+++ b/lib/eal/linux/eal_hugepage_info.c
@@ -213,8 +213,8 @@ 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();
 
@@ -226,42 +226,70 @@ 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;
-				}
+		/* 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) {
-					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) {
+				continue;
 			}
-		} /* end if strncmp hugetlbfs */
+		}
+
+		/*
+		 * 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.25.1

[dpdk-dev] [PATCH v3] eal: allow hugetlbfs sub-directories

[John Levon]

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. 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 <john.levon@nutanix.com>
---
v2: prefer closer matches
v3: checkpatch fixes

 lib/eal/linux/eal_hugepage_info.c | 74 ++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage_info.c
index d97792cad..f78347617 100644
--- a/lib/eal/linux/eal_hugepage_info.c
+++ b/lib/eal/linux/eal_hugepage_info.c
@@ -213,8 +213,8 @@ 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();
 
@@ -226,42 +226,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.25.1

Re: [dpdk-dev] [PATCH v3] eal: allow hugetlbfs sub-directories

[Dmitry Kozlyuk]

2021-07-08 11:59 (UTC+0100), John Levon:
> 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. 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 <john.levon@nutanix.com>

Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Re: [dpdk-dev] [PATCH v3] eal: allow hugetlbfs sub-directories

[David Marchand]

On Thu, Jul 8, 2021 at 1:00 PM John Levon <john.levon@nutanix.com> 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. 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 <john.levon@nutanix.com>

This change in EAL hugetlbfs discovery is too dangerous to be taken after -rc1.

Could you give some usecases/examples on why this change is needed?
Updating the documentation and the unit test also seem necessary.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v3] eal: allow hugetlbfs sub-directories

[John Levon]

On Thu, Jul 22, 2021 at 10:29:45PM +0200, David Marchand wrote:

> On Thu, Jul 8, 2021 at 1:00 PM John Levon <john.levon@nutanix.com> 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. 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 <john.levon@nutanix.com>
> 
> This change in EAL hugetlbfs discovery is too dangerous to be taken after -rc1.

Sure.

> Could you give some usecases/examples on why this change is needed?

Would you like me to expand the commit message? I had hoped it was clear enough,
but I suppose not. Simply put, DPDK above is assuming its the only user of
hugepages on the system - including clear_hugedir(). That is certainly not the
case for our use cases.

> Updating the documentation

https://doc.dpdk.org/guides/linux_gsg/linux_eal_parameters.html

"""
--huge-dir <path to hugetlbfs directory>

Use specified hugetlbfs directory instead of autodetected ones.
"""

That is, it already says "directory", not "mount". You'd like something
additional saying it can be below a mount point?

> and the unit test also seem necessary.

You're talking about app/test/test_eal_flags.c or something else?

thanks,
john

Re: [dpdk-dev] [PATCH v3] eal: allow hugetlbfs sub-directories

[Thomas Monjalon]

22/07/2021 23:06, John Levon:
> On Thu, Jul 22, 2021 at 10:29:45PM +0200, David Marchand wrote:
> 
> > On Thu, Jul 8, 2021 at 1:00 PM John Levon <john.levon@nutanix.com> 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. 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 <john.levon@nutanix.com>
> > 
> > This change in EAL hugetlbfs discovery is too dangerous to be taken after -rc1.
> 
> Sure.
> 
> > Could you give some usecases/examples on why this change is needed?
> 
> Would you like me to expand the commit message?

Yes please add some examples of directories explaining the issue you hit.

> I had hoped it was clear enough,
> but I suppose not. Simply put, DPDK above is assuming its the only user of
> hugepages on the system - including clear_hugedir(). That is certainly not the
> case for our use cases.
> 
> > Updating the documentation
> 
> https://doc.dpdk.org/guides/linux_gsg/linux_eal_parameters.html
> 
> """
> --huge-dir <path to hugetlbfs directory>
> 
> Use specified hugetlbfs directory instead of autodetected ones.
> """
> 
> That is, it already says "directory", not "mount". You'd like something
> additional saying it can be below a mount point?

Yes

> > and the unit test also seem necessary.
> 
> You're talking about app/test/test_eal_flags.c or something else?

Yes

Thanks

[dpdk-dev] [PATCH] eal: allow hugetlbfs sub-directories

[John Levon]

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 <john.levon@nutanix.com>
---
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 <errno.h>
 #include <unistd.h>
 #include <dirent.h>
-#include <sys/wait.h>
 #include <sys/file.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
 #include <limits.h>
 #include <fcntl.h>
 
@@ -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 <path to hugetlbfs directory>``
 
-    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

Re: [dpdk-dev] [PATCH] eal: allow hugetlbfs sub-directories

[Jerin Jacob]

On Mon, Aug 9, 2021 at 4:54 PM John Levon <john.levon@nutanix.com> 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 <john.levon@nutanix.com>

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 <errno.h>
>  #include <unistd.h>
>  #include <dirent.h>
> -#include <sys/wait.h>
>  #include <sys/file.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
>  #include <limits.h>
>  #include <fcntl.h>
>
> @@ -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 <path to hugetlbfs directory>``
>
> -    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
>

Re: [dpdk-dev] [PATCH] eal: allow hugetlbfs sub-directories

[John Levon]

On Mon, Aug 09, 2021 at 12:24:34PM +0100, 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 <john.levon@nutanix.com>
> ---
> v2: prefer closer matches
> v3: checkpatch fixes
> v4: fix docs, added tests

Anyone like to (re) review?

thanks
john

> 
>  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 <errno.h>
>  #include <unistd.h>
>  #include <dirent.h>
> -#include <sys/wait.h>
>  #include <sys/file.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
>  #include <limits.h>
>  #include <fcntl.h>
>  
> @@ -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 <path to hugetlbfs directory>``
>  
> -    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;
>  }
>  
>  /*

Re: [dpdk-dev] [PATCH] eal: allow hugetlbfs sub-directories

[David Marchand]

On Tue, Aug 17, 2021 at 10:05 PM John Levon <john.levon@nutanix.com> wrote:
> On Mon, Aug 09, 2021 at 12:24:34PM +0100, 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 <john.levon@nutanix.com>
> > ---
> > v2: prefer closer matches
> > v3: checkpatch fixes
> > v4: fix docs, added tests
>
> Anyone like to (re) review?

Your patch lgtm, I tested a few scenarios I had in mind and it looks ok.
Dmitry offered to rebase after your patch, can you send a new revision
with following updates?
- This patch only concerns Linux, so the title prefix should be eal/linux,
- Jerin asked for a release notes update, a quick description is fine,
- Dmitry mentionned he acked your v3, so you can add it.


Thanks.

-- 
David Marchand

[dpdk-dev] [PATCH] eal/linux: allow hugetlbfs sub-directories

[John Levon]

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 <john.levon@nutanix.com>
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
v2: prefer closer matches
v3: checkpatch fixes
v4: fix docs, added tests
v5: added release note

 app/test/test_eal_flags.c                     | 116 ++++++++++++------
 doc/guides/linux_gsg/linux_eal_parameters.rst |   3 +-
 doc/guides/rel_notes/release_21_11.rst        |   6 +-
 lib/eal/linux/eal_hugepage_info.c             |  83 +++++++++----
 4 files changed, 143 insertions(+), 65 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 <errno.h>
 #include <unistd.h>
 #include <dirent.h>
-#include <sys/wait.h>
 #include <sys/file.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
 #include <limits.h>
 #include <fcntl.h>
 
@@ -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 <path to hugetlbfs directory>``
 
-    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/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index 54718ff367..9fe689356b 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -62,6 +62,11 @@ New Features
   * Added bus-level parsing of the devargs syntax.
   * Kept compatibility with the legacy syntax as parsing fallback.
 
+* **Updated EAL hugetlbfs mount handling.**
+
+  * Modified to allow ``--huge-dir`` option to specify a sub-directory
+    within a hugetlbfs mountpoint.
+
 * **Added new RSS offload types for IPv4/L4 checksum in RSS flow.**
 
   Added macros ETH_RSS_IPV4_CHKSUM and ETH_RSS_L4_CHKSUM, now IPv4 and
@@ -154,7 +159,6 @@ New Features
   * Added tests to verify tunnel header verification in IPsec inbound.
   * Added tests to verify inner checksum.
 
-
 Removed Items
 -------------
 
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

Re: [dpdk-dev] [PATCH] eal/linux: allow hugetlbfs sub-directories

[David Marchand]

On Tue, Oct 12, 2021 at 6:06 PM John Levon <john.levon@nutanix.com> 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 <john.levon@nutanix.com>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>

Applied, thanks.


> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index 54718ff367..9fe689356b 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -154,7 +159,6 @@ New Features
>    * Added tests to verify tunnel header verification in IPsec inbound.
>    * Added tests to verify inner checksum.
>
> -

Restored this empty line: sections in doc are separated with two empty lines.

>  Removed Items
>  -------------
>

-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal/linux: allow hugetlbfs sub-directories

[John Levon]

On Tue, Oct 12, 2021 at 09:03:09PM +0200, David Marchand wrote:

> > diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> > index 54718ff367..9fe689356b 100644
> > --- a/doc/guides/rel_notes/release_21_11.rst
> > +++ b/doc/guides/rel_notes/release_21_11.rst
> > @@ -154,7 +159,6 @@ New Features
> >    * Added tests to verify tunnel header verification in IPsec inbound.
> >    * Added tests to verify inner checksum.
> >
> > -
> 
> Restored this empty line: sections in doc are separated with two empty lines.

Apologies, and thanks for fixing.

regards
john