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

revise network integration and add support of futures #117

Merged
merged 22 commits into from
Aug 7, 2021

Conversation

stlankes
Copy link
Contributor

smoltcp 0.7 provides async/await waker support (smoltcp-rs/smoltcp#394).
I followed the example implementation https://github.com/embassy-rs/embassy/tree/net/embassy-net
to add waker support in hermit-sys.

However, hermit-sys can be used in a multi-threaded applications.
Consequently, the implementation must be thread safed. In addition,
we still have an "network thread", which is wakeup by an interrupt.
The thread wakeups all waiting future. If no future is available,
the thread calls directly the IP stack.

@stlankes stlankes marked this pull request as draft March 22, 2021 10:20
stlankes added a commit to stlankes/kernel that referenced this pull request Mar 22, 2021
@mkroening
Copy link
Member

I am not sure if I understand this approach correctly.

How is the polling interface exposed to the user? All newly introduced async fns in hermit-sys are not publicly visible. The sys_*-wrappers for std are blocking the current thread, matching std's API, which is non-async. To me this currently looks like an additional thread-scheduling-system in the kernel while still context-switching without any benefits from being async.

As far as I understand it, we would want to expose an API for polling on sockets like epoll, kqueue, event ports and wepoll. Such an API is not part of std, but we would have to add support for it to the async-frameworks like tokio, async-std or smol (in case of smol in smol-rs/polling). That way, users could use the async-ecosystem (executor, abstractions) of their choice in their applications to avoid unnecessary context switches.

Am I missing something? What do you think?

Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

In general this looks good to me.

Regarding the wrongly ordered packages we talked about: With this PR I get a few “TCP Previous segment not captured” and “TCP ACKed unseen segment” in Wireshark. Are these the same symptoms you meant? They are but an artifact of Wireshark not being able to keep up. My symptoms vanished when I set my CPU governor to performance and gave the Wireshark process a higher priority. Conversely, I could produce these symptoms without this PR by lowering the priority of the Wireshark process. I am not sure though, if this is caused by the additional CPU usage by the executor of if this is avoidable, and we do something wrong.

I'll have a deeper look into the implementation and will create PRs with suggestions for simplification to your branch.

Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Our executor is never actually run, so network_run is currently dead code. I'll have a look at this.

Edit: Instead, network_poll is called manually in the block_on loop.

stlankes added a commit to stlankes/kernel that referenced this pull request May 26, 2021
stlankes added a commit to stlankes/kernel that referenced this pull request Jun 25, 2021
@stlankes
Copy link
Contributor Author

@mkroening I merge the current master to this PR. I am not sure, if I merge everything correctly. Can you check it?

mkroening pushed a commit to mkroening/libhermit-rs that referenced this pull request Jun 29, 2021
mkroening pushed a commit to mkroening/libhermit-rs that referenced this pull request Jun 29, 2021
mkroening pushed a commit to mkroening/libhermit-rs that referenced this pull request Jun 29, 2021
mkroening pushed a commit to mkroening/libhermit-rs that referenced this pull request Jul 1, 2021
stlankes added a commit to stlankes/kernel that referenced this pull request Aug 2, 2021
stlankes and others added 7 commits August 2, 2021 23:35
smoltcp 0.7 provides async/await waker support (smoltcp-rs/smoltcp#394).
I followed the example implementation https://github.com/embassy-rs/embassy/tree/net/embassy-net
to add waker support in hermit-sys.

However, hermit-sys can be used in a multi-threaded applications.
Consequently, the implementation must be thread safed. In addition,
we still have an "network thread", which is wakeup by an interrupt.
The thread wakeups all waiting future. If no future is available,
the thread calls directly the IP stack.
mkroening and others added 7 commits August 2, 2021 23:35
smoltcp 0.7 provides async/await waker support (smoltcp-rs/smoltcp#394).
I followed the example implementation https://github.com/embassy-rs/embassy/tree/net/embassy-net
to add waker support in hermit-sys.

However, hermit-sys can be used in a multi-threaded applications.
Consequently, the implementation must be thread safed. In addition,
we still have an "network thread", which is wakeup by an interrupt.
The thread wakeups all waiting future. If no future is available,
the thread calls directly the IP stack.
- the kernel uses the C calling convention and requires minor changes
stlankes added a commit to stlankes/kernel that referenced this pull request Aug 3, 2021
stlankes added a commit to hermit-os/kernel that referenced this pull request Aug 7, 2021
- workaround is required for Windows
- it seems that the latest Qemu version doesn't work correctly on Windows
@stlankes stlankes marked this pull request as ready for review August 7, 2021 22:38
@stlankes stlankes merged commit d2ad9ae into hermit-os:master Aug 7, 2021
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