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

Add multiple migration support for vmcall-vsock #320

Merged
merged 6 commits into from
Nov 19, 2024

Conversation

gaojiaqi7
Copy link
Contributor

No description provided.

@gaojiaqi7 gaojiaqi7 mentioned this pull request Nov 15, 2024
log::info!("Set MSK and report status\n");
exchange_information.key.clear();
remote_information.key.clear();

Ok(())
}

fn exchange_info(info: &MigrationInformation) -> Result<ExchangeInformation> {

Check warning

Code scanning / clippy

associated functions exchange_info, read_msk, and write_msk are never used Warning

associated functions exchange\_info, read\_msk, and write\_msk are never used
Ok(exchange_info)
}

fn read_msk(mig_info: &MigtdMigrationInformation, msk: &mut MigrationSessionKey) -> Result<()> {

Check warning

Code scanning / clippy

associated functions exchange_info, read_msk, and write_msk are never used Warning

associated functions exchange\_info, read\_msk, and write\_msk are never used
Ok(())
}

fn write_msk(mig_info: &MigtdMigrationInformation, msk: &MigrationSessionKey) -> Result<()> {

Check warning

Code scanning / clippy

associated functions exchange_info, read_msk, and write_msk are never used Warning

associated functions exchange\_info, read\_msk, and write\_msk are never used
@@ -32,7 +27,7 @@ pub fn schedule_timeout(timeout: u64) -> Option<u64> {
reset_timer();
let cpuid = unsafe { core::arch::x86_64::__cpuid_count(0x15, 0) };
let tsc_frequency = cpuid.ecx * (cpuid.ebx / cpuid.eax);
let timeout = timeout / 1000 * tsc_frequency as u64;
let timeout = tsc_frequency as u64 * timeout / 1000;
Copy link
Contributor

@jyao1 jyao1 Nov 18, 2024

Choose a reason for hiding this comment

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

  1. timeout function should guarantee to timeout enough time.

A proper way for x / 1000 should be (x + 999) / 1000.

  1. Please describe the unit (ms? or us?, or s?)

  2. I feel u32 timeout value is enough for schedule_timeout(timeout: u32)

  3. Can we give a different name for local variable timeout, what is difference from input param timeout ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For 1, the purpose of this line of code is to calculate the increase in the Time Stamp Counter (TSC) value over a specified delay period, I think rewrite as tsc_frequency as u64 / 1000 * timeout as u64 is a better way to address the potential overflow.

For 2/3/4, the code/comments has been updated.

@@ -135,6 +137,50 @@ fn handle_pre_mig() {
}
}

#[cfg(feature = "async")]
fn handle_pre_mig() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need to handle sync and async differently? Why we need a feature?

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 original wait_for_request on Linux blocks the program from reading/writing from virtio vsock/serial until next migration request comes. I suggest to file a new issue to track the async compatiblity on block version wait_for_request.

@@ -155,7 +162,7 @@ impl MigrationSession {
Ok(())
}

pub fn wait_for_request(&mut self) -> Result<()> {
pub fn wait_for_request(&mut self, block: bool) -> Result<Option<u64>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need to support block here? Can we use false for original use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. The async and sync code blocks are merged.

src/async/async_runtime/src/executor.rs Fixed Show fixed Hide fixed
src/async/async_runtime/src/executor.rs Fixed Show fixed Hide fixed
src/async/async_runtime/src/executor.rs Fixed Show fixed Hide fixed
src/migtd/src/migration/session.rs Fixed Show fixed Hide fixed
@gaojiaqi7 gaojiaqi7 force-pushed the 1101/async_vmcall branch 5 times, most recently from 95cbeea to 9473f17 Compare November 19, 2024 09:38
`async_io` and `async_runtime` are introduced to provide async/await
support.

Signed-off-by: Jiaqi Gao <[email protected]>
If the block flag of the vsock/serial is not set, transport layer will
not wait for the data and return `NotReady` error if the data is not
ready.

Signed-off-by: Jiaqi Gao <[email protected]>
To avoid potential integer overflow.

Signed-off-by: Jiaqi Gao <[email protected]>
A system tick is maintained to support timeout future.

Signed-off-by: Jiaqi Gao <[email protected]>
@jyao1 jyao1 merged commit 746746b into intel:dev/async_multiple_session Nov 19, 2024
11 checks passed
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