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

refact: use mio for daemon exit signal monitoring #340

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

ccx1024cc
Copy link
Contributor

Nydusd responds to SYS_TERM and SYS_INT based on event poll. In fact, it needs a way to communicate between threads. Event poll is platform-coherent.
This PR replace raw eventfd with MIO, which is a I/O library for focusing on non-blocking APIs.

@jiangliu
Copy link
Collaborator

Linux EventFd is missing on macos, and it seems like a common requirements. So should we enhance vmm-sys-utils to provide EventFd for macos?
rust-vmm/vhost#110

src/bin/nydusd/main.rs Outdated Show resolved Hide resolved
@ccx1024cc
Copy link
Contributor Author

The eventfd is not required for nydus, but a implementation for communication of threads. So this PR remove the coherent between eventfd and nydusd. The mio maybe a better way to provide abstract API.
PS: It provides different ways to run by os, which is not cared by nydus.

src/bin/nydusd/main.rs Outdated Show resolved Hide resolved
};
use std::thread;
use std::thread::{self, spawn};
use std::{io, process};

use clap::{App, Arg};
use event_manager::{EventManager, EventSubscriber, SubscriberOps};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these still needed after switch to mio?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"spawn" is still needed for starting monitor so far(line 496).

Copy link
Member

Choose a reason for hiding this comment

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

The http thread is still using eventfd to exit itself. Are you planning to switch it (via the new micro_http crate) in a different PR?

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 http thread is still using eventfd to exit itself. Are you planning to switch it (via the new micro_http crate) in a different PR?

Yes. In a later PR.

};
use std::thread;
use std::thread::{self, spawn};
use std::{io, process};

use clap::{App, Arg};
use event_manager::{EventManager, EventSubscriber, SubscriberOps};
Copy link
Member

Choose a reason for hiding this comment

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

The http thread is still using eventfd to exit itself. Are you planning to switch it (via the new micro_http crate) in a different PR?

src/bin/nydusd/main.rs Outdated Show resolved Hide resolved
src/bin/nydusd/main.rs Outdated Show resolved Hide resolved
@@ -91,12 +92,12 @@ fn get_default_rlimit_nofile() -> Result<rlim> {
}

pub fn exit_event_manager() {
Copy link
Collaborator

@imeoer imeoer Mar 22, 2022

Choose a reason for hiding this comment

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

We'd better use as few global variables/functions as possible, how about wrapping an exit manager as a struct implementation, and putting it to FuseDaemon/VirtiofsDaemon? (This is not a blocker for this PR at all)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd better use as few global variables/functions as possible, how about wrapping an exit manager as a struct implementation, and putting it to FuseDaemon/VirtiofsDaemon? (This is not a blocker for this PR at all)

Absolutely right. Event manager maybe removed, together with global shared memory - EXIT_MANAGER_RUN in later PR.

@bergwolf
Copy link
Member

LGTM! Please fold the two commits into a single one.

Signed-off-by: 泰友 <cuichengxu.ccx@antgroup.com>

fix: chore

Signed-off-by: 泰友 <cuichengxu.ccx@antgroup.com>
@bergwolf bergwolf merged commit cc87025 into dragonflyoss:master Mar 22, 2022
@ccx1024cc ccx1024cc deleted the morgan/mio branch March 23, 2022 01:34
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.

4 participants