Uh oh!
There was an error while loading. Please reload this page.
linux, checking THP current system settings. - #494
linux, checking THP current system settings.#494David CARLIER (devnexen) wants to merge 1 commit into
Conversation
Daan (daanx)
left a comment
There was a problem hiding this comment.
Thanks for adding this (the current pipeline failure is due to an earlier commit of mine I think for Windows). I added some inline comments. Also, as I understand, the madvise call is only needed if the setting is madvise ?
| static void os_detect_transparent_huge_pages(void) { | ||
| #if defined(MADV_HUGEPAGE) | ||
| int fd = open("/sys/kernel/mm/transparent_hugepage/enabled", O_RDONLY); |
There was a problem hiding this comment.
Is this path always fixed? I read here that it can also be /sys/kernel/mm/redhat_transparent_hugepage/enabled for example?
There was a problem hiding this comment.
ah good to know
| close(fd); | ||
| if (nread >= 1) { | ||
| // in some configurations, it can occur (e.g. embedded) | ||
| if (!strstr(buf, "[never]")) os_transparent_huge_pages = true; |
There was a problem hiding this comment.
What are the valid contents here? I guess always, madvise and never.
But it seems also [never] is valid? Should we just check for madvise perhaps?
eccbff7 to
1ab104eCompare| for (i = 0; i < sizeof(paths) / sizeof(paths[0]); i ++) { | ||
| fd = open(paths[i], O_RDONLY); | ||
| if (fd > 0) break; |
There was a problem hiding this comment.
open can return 0, that's a valid value, not an error.
So I would change that check to fd >= 0
There was a problem hiding this comment.
in general yes even tough stdin might not be closed yet at this early stage but nevertheless I made it less ambiguous.
1ab104e to
83228bbCompareDaan (daanx)
commented
Dec 16, 2021
Ah, this is starting to look rather complicated and it is unclear whether we actually need it. Currently, it is only called if we cannot do it directly and via |
David CARLIER (devnexen)
commented
Dec 16, 2021
I see your point, that is fine if you prefer not merging it. |
| ssize_t nread = read(fd, &buf, sizeof(buf)); | ||
| close(fd); | ||
| if (nread >= 1) { | ||
| if (strstr(buf, "[madvise]")) os_transparent_huge_pages = true; |
There was a problem hiding this comment.
buf is not null-terminated, so strstr is undefined behavior. memmem(buf, nread, "[madvise]", strlen("[madvise]")) instead?
No description provided.