new(config): add `falco_libs.thread_table_size` #3071

incertum · 2024-02-10T20:06:51Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

We recently increased the default threadtable size to accommodate modern infrastructures better as we have observed drops related to a full internal thread table. This PR makes this setting configurable by the end user.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(config): add `falco_libs.thread_table_size`

incertum · 2024-02-10T20:07:03Z

/milestone 0.38.0

incertum · 2024-02-10T20:08:10Z

falco.yaml

+# and the fields called `n_drops_full_threadtable` or `n_store_evts_drops` will inform you
+# if you should increase this value for optimal performance.
+state_engine:
+  thread_table_size: 262144


@FedeDP and @jasondellaluce since you reviewed the libs changes. Thank you!

FedeDP · 2024-02-12T14:01:55Z

userspace/falco/configuration.cpp

@@ -63,6 +63,7 @@ falco_configuration::falco_configuration():
 	m_syscall_evt_drop_max_burst(1),
 	m_syscall_evt_simulate_drops(false),
 	m_syscall_evt_timeout_max_consecutives(1000),
+	m_state_engine_thread_table_size(262144),


Can we have a macro for the default value? Given it is a "weird" number used twice :)

Agreed, funny enough if you search that number in the Linux kernel it is not so odd.

Before I re-push, was wondering about the Windows build failure. is there something in libs that needs to be adjusted?

was wondering about the Windows build failure. is there something in libs that needs to be adjusted?

Don't really know why it is complaining :/

jsoncpp.lib(json_value.obj) : error LNK2019: unresolved external symbol __imp_modf referenced in function "public: bool __cdecl Json::Value::isInt64(void)const " (?isInt64@Value@Json@@QEBA_NXZ) [D:\a\falco\falco\build\libsinsp\examples\sinsp-example.vcxproj] jsoncpp.lib(json_writer.obj) : error LNK2019: unresolved external symbol __imp__finite referenced in function "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl Json::`anonymous namespace'::valueToString(double,bool,unsigned int,enum Json::PrecisionType)" (?valueToString@?A0x7fc2ca38@Json@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@N_NIW4PrecisionType@2@@Z) [D:\a\falco\falco\build\libsinsp\examples\sinsp-example.vcxproj] jsoncpp.lib(json_writer.obj) : error LNK2019: unresolved external symbol __imp__isnan referenced in function "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl Json::`anonymous namespace'::valueToString(double,bool,unsigned int,enum Json::PrecisionType)" (?valueToString@?A0x7fc2ca38@Json@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@N_NIW4PrecisionType@2@@Z) [D:\a\falco\falco\build\libsinsp\examples\sinsp-example.vcxproj]

Seems a libs issue on jsoncpp; perhaps triggered by: falcosecurity/libs@a89fd5d

cc @geraldcombs can you take a look? 🙏 thanks!!

Perhaps you could write it as 0x40000 so it's easier to understand. Not sure if you can do the same in yaml as well.

Seems a libs issue on jsoncpp

It might be because libs is linking with the dynamic CRT (MultiThreadedDLL) instead of the static one (MultiThreaded). If that's the case we should be able to fix it here by passing -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded to CMake in the build-win32-package job, similar to the build-libs-others-amd64 job in libs.

Alternatively we could try to fix it in libs, e.g. by trying to override CMAKE_MSVC_RUNTIME_LIBRARY in CMakeLists.txt.

@geraldcombs whichever you think is the best approach. I can bump libs here to the new latest after we address it in libs if we choose that path.

@LucaGuerra internally yes we can default to 0x40000 in the macro, but don't you think for adopters that is even more weird than a normal number? 🙃

@geraldcombs whichever you think is the best approach. I can bump libs here to the new latest after we address it in libs if we choose that path.

I opened a libs PR (1695) to link to the static CRT by default. That seemed to make the most sense, since the project favors static builds.

leogr

One thought after a quick look.

I would like to group some top-level config keys instead of continuously adding new ones.

I mean, the top-level config keys are growing exponentially, and this may not be ideal for users approaching the configuration.

incertum · 2024-02-13T16:22:25Z

One thought after a quick look.

I would like to group some top-level config keys instead of continuously adding new ones.

I mean, the top-level config keys are growing exponentially, and this may not be ideal for users approaching the configuration.

What would it be? It seems that we don't have a new top level category for state_engine related configs that may grow over time?

Andreagit97 · 2024-02-14T07:59:54Z

maybe we could add a key called libsinsp or internals that in the future will collect all the libs configurations... for example, we already have the libs_logger config that we could move behind this new config (not now of course) WDYT?

FedeDP · 2024-02-14T08:09:17Z

@Andreagit97 that's a great idea actually, even if it is exposing some "internal knowledge" about who owns what.
I'd rather call it just falco-libs:.

incertum · 2024-02-14T20:35:49Z

@Andreagit97 that's a great idea actually, even if it is exposing some "internal knowledge" about who owns what. I'd rather call it just falco-libs:.

falco_libs with underscore instead? Perhaps in the future the libs_logger can be moved under falco_libs as well. It should be less of a breaking change as likely few folks use that setting.

Additional question: Do we know what's wrong with the Windows build?

Andreagit97 · 2024-02-15T09:27:11Z

falco_libs with underscore instead?

Yep, we need to use the underscore

leogr · 2024-02-15T10:43:15Z

In general, I wouldn't say I like the idea of exposing internal names to the users (because internals can change, but the external function remains the same).

So perhaps something like libs_internals (to make it explicitly, these are internal settings and may potentially change very often)

However, falco_libs or just libs is still for me if the consensus goes toward this.

incertum · 2024-02-16T20:34:49Z

@geraldcombs using the latest libs commit and also rebased w/ falco master, still having the windows failure. Could you take another look? Thank you!

geraldcombs · 2024-02-16T23:43:34Z

@geraldcombs using the latest libs commit and also rebased w/ falco master, still having the windows failure. Could you take another look? Thank you!

I'm able to replicate the issue here. If I make the following change here

--- a/.github/workflows/reusable_build_packages.yaml
+++ b/.github/workflows/reusable_build_packages.yaml
@@ -233,7 +233,7 @@ jobs:
         run: |
           mkdir build
           cd build
-          cmake -DCMAKE_BUILD_TYPE=Release -DMINIMAL_BUILD=On -DUSE_BUNDLED_DEPS=On -DBUILD_FALCO_UNIT_TESTS=On -DFALCO_VERSION=${{ inputs.version }} ..
+          cmake -DCMAKE_BUILD_TYPE=Release -DMINIMAL_BUILD=On -DUSE_BUNDLED_DEPS=On -DBUILD_FALCO_UNIT_TESTS=On -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded -DFALCO_VERSION=${{ inputs.version }} ..

I can build and run falco_unit_tests.exe, although I'm not sure why that's necessary. If I build a standalone version of libs here without setting CMAKE_MSVC_RUNTIME_LIBRARY I get a static build.

incertum · 2024-02-17T00:13:13Z

Thanks @geraldcombs I'll then rebase, thanks!

incertum · 2024-02-21T17:56:33Z

@geraldcombs using the latest libs commit and also rebased w/ falco master, still having the windows failure. Could you take another look? Thank you!

I'm able to replicate the issue here. If I make the following change here
--- a/.github/workflows/reusable_build_packages.yaml
+++ b/.github/workflows/reusable_build_packages.yaml
@@ -233,7 +233,7 @@ jobs:
         run: |
           mkdir build
           cd build
-          cmake -DCMAKE_BUILD_TYPE=Release -DMINIMAL_BUILD=On -DUSE_BUNDLED_DEPS=On -DBUILD_FALCO_UNIT_TESTS=On -DFALCO_VERSION=${{ inputs.version }} ..
+          cmake -DCMAKE_BUILD_TYPE=Release -DMINIMAL_BUILD=On -DUSE_BUNDLED_DEPS=On -DBUILD_FALCO_UNIT_TESTS=On -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded -DFALCO_VERSION=${{ inputs.version }} ..
I can build and run falco_unit_tests.exe, although I'm not sure why that's necessary. If I build a standalone version of libs here without setting CMAKE_MSVC_RUNTIME_LIBRARY I get a static build.

@geraldcombs just double-checking if I interpret this correctly. You or someone else will open a PR and I shall rebase afterwards? Thanks in advance for clarifying!

github-actions · 2024-02-23T17:38:44Z

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

incertum · 2024-02-23T18:15:50Z

@jasondellaluce already bumped libs before hence I could remove it here. However, the CI still seems to be in a not good state (unrelated to these changes).

jasondellaluce

@incertum I have no major concerns here if not the naming decision, as pointed out above. I'm pretty much onboard with the suggestions of the other reviewers.

jasondellaluce · 2024-02-26T11:02:35Z

userspace/engine/falco_common.h

@@ -28,6 +28,8 @@ limitations under the License.
 //
 #define DEFAULT_OUTPUTS_QUEUE_CAPACITY_UNBOUNDED_MAX_LONG_VALUE std::ptrdiff_t(~size_t(0) / 2)

+#define DEFAULT_FALCO_LIBS_INTERNALS_THREAD_TABLE_SIZE 0x4000


Shouldn't this be 0x40000? Seems like this is a quite smaller number

uhm yeah you see hey @LucaGuerra I reverted to the human readable value, less prone to such issues.

incertum · 2024-02-26T16:56:22Z

@incertum I have no major concerns here if not the naming decision, as pointed out above. I'm pretty much onboard with the suggestions of the other reviewers.

@jasondellaluce Could you elaborate? I tried to pick a name that addresses all feedback. If this should not be it, what should the final name be? No strong opinion.

jasondellaluce · 2024-02-26T17:46:12Z

@incertum I have no major concerns here if not the naming decision, as pointed out above. I'm pretty much onboard with the suggestions of the other reviewers.

@jasondellaluce Could you elaborate? I tried to pick a name that addresses all feedback. If this should not be it, what should the final name be? No strong opinion.

I think I'd go with either falco_libs or libs_internals. But again, just my personal taste!

leogr · 2024-02-29T16:27:51Z

@incertum I have no major concerns here if not the naming decision, as pointed out above. I'm pretty much onboard with the suggestions of the other reviewers.

@jasondellaluce Could you elaborate? I tried to pick a name that addresses all feedback. If this should not be it, what should the final name be? No strong opinion.

I think I'd go with either falco_libs or libs_internals. But again, just my personal taste!

After thinking about this again, I believe falco_libs can be nice, especially if we start distributing Falco's libs at some point in the future.

incertum · 2024-02-29T18:27:43Z

SGTM, just changed it. Thanks.

FedeDP

/approve

poiana · 2024-03-01T07:55:19Z

LGTM label has been added.

Git tree hash: 2a6cb1f153d2edb03f88bba04b0f167decafb967

Andreagit97

a minor nit and then we can go! thank you!

Andreagit97 · 2024-03-01T09:26:32Z

userspace/falco/configuration.h

@@ -136,6 +136,8 @@ class falco_configuration

 	uint32_t m_syscall_evt_timeout_max_consecutives;

+	std::uint32_t m_falco_libs_thread_table_size;


Suggested change

std::uint32_t m_falco_libs_thread_table_size;

uint32_t m_falco_libs_thread_table_size;

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

poiana · 2024-03-05T09:37:13Z

LGTM label has been added.

Git tree hash: 8be4088d216f0f73aa595b0db4a073ca47c744dd

Andreagit97

/approve

poiana · 2024-03-05T10:36:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, incertum, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Andreagit97,FedeDP,incertum,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Andreagit97 · 2024-03-05T10:36:37Z

/unhold

poiana added release-note dco-signoff: yes kind/feature area/engine approved labels Feb 10, 2024

poiana requested review from Kaizhe and leogr February 10, 2024 20:06

poiana added this to the 0.38.0 milestone Feb 10, 2024

poiana added the size/M label Feb 10, 2024

incertum commented Feb 10, 2024

View reviewed changes

incertum force-pushed the set-threadtable-size branch 2 times, most recently from e7db2f9 to d5db824 Compare February 10, 2024 21:03

FedeDP reviewed Feb 12, 2024

View reviewed changes

leogr reviewed Feb 13, 2024

View reviewed changes

FedeDP mentioned this pull request Feb 15, 2024

[PROPOSAL] Move config descriptions from falco.yaml to Falco documentation #3086

Open

incertum force-pushed the set-threadtable-size branch from d5db824 to f1da096 Compare February 16, 2024 20:12

incertum changed the title ~~new(config): add state_engine.thread_table_size~~ new(config): add falco_libs_internals.thread_table_size Feb 16, 2024

incertum force-pushed the set-threadtable-size branch from f1da096 to 1a6f4c1 Compare February 23, 2024 17:38

jasondellaluce reviewed Feb 26, 2024

View reviewed changes

incertum force-pushed the set-threadtable-size branch from 1a6f4c1 to e17c886 Compare February 26, 2024 16:53

incertum force-pushed the set-threadtable-size branch from e17c886 to 14b3364 Compare February 29, 2024 18:26

incertum changed the title ~~new(config): add falco_libs_internals.thread_table_size~~ new(config): add falco_libs.thread_table_size Feb 29, 2024

FedeDP previously approved these changes Mar 1, 2024

View reviewed changes

poiana assigned FedeDP Mar 1, 2024

poiana added the lgtm label Mar 1, 2024

Andreagit97 reviewed Mar 1, 2024

View reviewed changes

new(config): add falco_libs.thread_table_size

b635f79

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum dismissed FedeDP’s stale review via b635f79 March 1, 2024 18:14

incertum force-pushed the set-threadtable-size branch from 14b3364 to b635f79 Compare March 1, 2024 18:14

poiana removed the lgtm label Mar 1, 2024

poiana requested a review from FedeDP March 1, 2024 18:14

leogr approved these changes Mar 5, 2024

View reviewed changes

poiana assigned leogr Mar 5, 2024

poiana added the lgtm label Mar 5, 2024

Andreagit97 approved these changes Mar 5, 2024

View reviewed changes

poiana assigned Andreagit97 Mar 5, 2024

poiana removed the do-not-merge/hold label Mar 5, 2024

poiana merged commit 5185f15 into falcosecurity:master Mar 5, 2024
29 checks passed

incertum deleted the set-threadtable-size branch March 5, 2024 22:04

		@@ -136,6 +136,8 @@ class falco_configuration

		uint32_t m_syscall_evt_timeout_max_consecutives;

		std::uint32_t m_falco_libs_thread_table_size;

	std::uint32_t m_falco_libs_thread_table_size;
	uint32_t m_falco_libs_thread_table_size;

new(config): add falco_libs.thread_table_size #3071

new(config): add falco_libs.thread_table_size #3071

Conversation

incertum commented Feb 10, 2024 • edited Loading

incertum commented Feb 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leogr left a comment

Choose a reason for hiding this comment

incertum commented Feb 13, 2024

Andreagit97 commented Feb 14, 2024

FedeDP commented Feb 14, 2024

incertum commented Feb 14, 2024

Andreagit97 commented Feb 15, 2024

leogr commented Feb 15, 2024

incertum commented Feb 16, 2024

geraldcombs commented Feb 16, 2024

incertum commented Feb 17, 2024

incertum commented Feb 21, 2024

github-actions bot commented Feb 23, 2024

incertum commented Feb 23, 2024

jasondellaluce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum commented Feb 26, 2024

jasondellaluce commented Feb 26, 2024

leogr commented Feb 29, 2024

incertum commented Feb 29, 2024

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Mar 1, 2024

Andreagit97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poiana commented Mar 5, 2024

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Mar 5, 2024

Andreagit97 commented Mar 5, 2024

new(config): add `falco_libs.thread_table_size` #3071

new(config): add `falco_libs.thread_table_size` #3071

incertum commented Feb 10, 2024 •

edited

Loading