[v3] eal: allow hugetlbfs sub-directories
Checks
Commit Message
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(-)
Comments
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>
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.
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
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
@@ -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;
}
/*