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

make the depth of the io rings a configurable knob #463

Merged
merged 4 commits into from Nov 17, 2021

Conversation

HippoBaro
Copy link
Member

Previously our io rings had a depth of 128. A user can now override this
if they feel like it. If they do, the setting will be used as the concurrency
level of reqad_many as well.

Copy link
Collaborator

@bryandmc bryandmc left a comment

Choose a reason for hiding this comment

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

Barring the need to change the MEMLOCK values dynamically, I think this looks good..

@@ -1164,6 +1165,7 @@ impl Reactor {
pub(crate) fn new(
notifier: Arc<sys::SleepNotifier>,
mut io_memory: usize,
ring_depth: usize,
) -> io::Result<Reactor> {
const MIN_MEMLOCK_LIMIT: u64 = 512 * 1024;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you are changing the size of the rings does this need to be adjusted in any way? I don't know for a fact it does but it seems like it will require more memory.. Obviously this amount may still be enough but I think it's worth looking into.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The actual memlock limit is a bit unpredictable. It depends on the number of executors, amount of I/O memory, and etc. The message is supposed to indicate a "good default".

@glommer
Copy link
Collaborator

glommer commented Nov 17, 2021

@HippoBaro there is a test failing after updating the branch

@HippoBaro HippoBaro merged commit 072ab2a into DataDog:master Nov 17, 2021
@HippoBaro HippoBaro deleted the variable_ring_depth branch November 17, 2021 15:38
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.

None yet

3 participants