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

Sys ctl to env param #277

Open
wants to merge 2 commits into
base: vNext
Choose a base branch
from

Conversation

BasharRadya
Copy link
Collaborator

Description

Read system control parameters from environment variables
Configure TSO and LRO default to ON

What

Subject: what this PR is doing in one line.

Why ?

better control of sys vars in virtual and container environment

How ?

read from enviorn variables instead of sys ctl

Change type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests
  • Other

Check list

  • Code follows the style de facto guidelines of this project
  • Comments have been inserted in hard to understand places
  • Documentation has been updated (if necessary)
  • Test has been added (if possible)

Refactored the logic to retrieve system parameters via environment variables
instead of relying on system control (`sysctl`) parameters.
Improved compatibility with containerized and runtime-configurable
environments.

Signed-off-by: Bashar Abdelgafer <[email protected]>
Changed TSO and LRO defaults ON  instead of auto for simplified configuration.

Signed-off-by: Bashar Abdelgafer <[email protected]>
@BasharRadya
Copy link
Collaborator Author

bot:retest

1 similar comment
@BasharRadya
Copy link
Collaborator Author

bot:retest

@AlexanderGrissik
Copy link
Collaborator

After discussion with Igor, we need to consider control parameters. Such as bindipv6only. These parameters are better to set the same as Kernel as these affect app behavior rather than performance. Lets discuss how we should proceed with them.

@@ -904,6 +935,35 @@ extern mce_sys_var &safe_mce_sys();
#define STRQ_MIN_STRIDE_SIZE_BYTES 64
#define STRQ_MAX_STRIDE_SIZE_BYTES 8192

/* Sysctl configurations*/
#define MCE_DEFAULT_TCP_MAX_SYN_BACKLOG 4096
Copy link
Collaborator

@AlexanderGrissik AlexanderGrissik Jan 9, 2025

Choose a reason for hiding this comment

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

would increase MCE_DEFAULT_TCP_MAX_SYN_BACKLOG to 50000

#define MCE_DEFAULT_TCP_WMEM_MAX 4194304

#define MCE_DEFAULT_TCP_RMEM_MIN 4096
#define MCE_DEFAULT_TCP_RMEM_DEFAULT 131072
Copy link
Collaborator

Choose a reason for hiding this comment

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

would increase MCE_DEFAULT_TCP_RMEM_DEFAULT to 16MB


#define MCE_DEFAULT_TCP_RMEM_MIN 4096
#define MCE_DEFAULT_TCP_RMEM_DEFAULT 131072
#define MCE_DEFAULT_TCP_RMEM_MAX 6291456
Copy link
Collaborator

Choose a reason for hiding this comment

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

would increase MCE_DEFAULT_TCP_RMEM_MAX to 16MB


#define MCE_DEFAULT_TCP_WINDOW_SCALING 1
#define MCE_DEFAULT_NET_CORE_RMEM_MAX 16777216
#define MCE_DEFAULT_NET_CORE_WMEM_MAX 212992
Copy link
Collaborator

Choose a reason for hiding this comment

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

would increase MCE_DEFAULT_NET_CORE_WMEM_MAX to 16MB

#define MCE_DEFAULT_TCP_MAX_SYN_BACKLOG 4096
#define MCE_DEFAULT_LISTEN_MAXCONN 4096

#define MCE_DEFAULT_TCP_WMEM_MIN 4096
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see all these are unused in practice. so all this stuff and related code in xlio can be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

all MCE_DEFAULT_TCP_WMEM_x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants