Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hugepages: run hugepage check only on DPDK runmode and on Linux v2 #10385

Closed

Conversation

lukashino
Copy link
Contributor

Follow-up of #10358

The previous implementation allowed FreeBSD to enter into the hugepage analysis. It then failed with an error message because hugepage/ NUMA node paths that are used in the codebase to retrieve info about the system are not the same as the structure in Linux.

Additionally, the messages were logged on an error level. It has been demoted to the info level because the whole hugepage analysis checkup is only for informational purposes and does not affect Suricata operation.

The hugepage analysis and the hugepage snapshots are now limited to only run in the DPDK runmode.

Ticket: #6760
https://redmine.openinfosecfoundation.org/issues/6760
Ticket: #6762
https://redmine.openinfosecfoundation.org/issues/6762

Previous implementation allowed FreeBSD to enter into the hugepage
analysis. It then failed with an error message because hugepage/
NUMA node paths that are used in the codebase to retrieve info about
the system are not the same with the structure in Linux.

Additionally, the messages were logged on error level. It has been
demoted to info level because the whole hugepage analysis checkup is
only for informational purposes and does not affect Suricata operation.

The hugepage analysis and the hugepage snapshots are now limited to
only run in the DPDK runmode.

Ticket: OISF#6760
Ticket: OISF#6762
@suricata-qa
Copy link

Information:

ERROR: QA failed on SURI_TLPW2_autofp_suri_time.

field baseline test %
SURI_TLPW2_autofp_stats_chk
.uptime 101 113 111.88%
SURI_TLPR1_stats_chk
.http.memuse 336824 388136 115.23%

Pipeline 18394

@@ -38,15 +38,15 @@ static void SystemHugepageSnapshotDump(SystemHugepageSnapshot *s);

static bool SystemHugepageSupported(void)
{
#if !defined __CYGWIN__ && !defined OS_WIN32 && !defined __OpenBSD__ && !defined sun
#if defined __linux__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was the reason to make this use ifdefs instead of just checking at runtime if hugepage support is enabled?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the function seems wrong, because it doesnt seem to be true that linux means hugepages are enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guards were there to avoid Windows compilation errors:
https://github.com/lukashino/suricata/actions/runs/7004361732/job/19052076585

extracted from: #9898

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that seems like a pretty small issue that can be addressed by refactoring things a bit, and/or investigating how to do this properly on windows

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, it seems it should work, so perhaps it's an include issue? https://github.com/luzexi/MinGW/blob/master/x86/include/dirent.h#L26

@@ -57,7 +57,7 @@ static uint16_t SystemNodeCountGetLinux(void)
char dir_path[] = "/sys/devices/system/node/";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, my Raspberry PI (more or less default) does not have /sys/devices/system/node. So would a Linux specific check to see if huge pages are enabled be "if linux and /sys/devices/system/node exists"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which for all intends and purposes can just be the path check. this will save us messy ifdefs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks, that's a good catch, I forgot that the path might not be present on every Linux, I'll redo the path check.

@lukashino
Copy link
Contributor Author

continues in #10448

@lukashino lukashino closed this Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants