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

[inetstack] Move TCP stack to use shared state machine #1008

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

iyzhang
Copy link
Contributor

@iyzhang iyzhang commented Nov 16, 2023

This PR moves the TCP portion of the inetstack to using the shared state machine that is in the runtime. A few important things to note:

  1. The TCP queue is now maintaining 2 state machines. We should deduplicate this and I have filed an issue to track it [inetstack] Deduplicate state machine #1007.
  2. This PR moves the inetstack closer to the same architecture as Catnap with functions passed in that move the state machine forward/rollback when there are errors (e.g., we were not able to insert the coroutine into the scheduler).
  3. This PR has added a list of pending ops to the TCP queue. However, because the packing for each returned result is in the libOS, I cannot remove the operation from the pending list. I think this will be resolved with Remove deprecated function remove_pending_op() #888 however.
  4. To align with Catnap, we now return queue tokens on synchronous paths for the task that was inserted to run the coroutine. This required adding another call to the runtime so we can remove coroutines using queue tokens because it is needed for the test engine. This should all be deduplicated once we merge the return path and the testing infrastructure.
  5. There was one place where the existing inetstack state machine and the shared runtime state machine diverged on the return value when listening on an already listening socket. The former returns EINVAL and the latter returns EADDRINUSE. I chose to go with the runtime but we should make sure that is the expected behavior because I had to change a test case.
  6. I've placed profiler and trace messages in the TCP peer because I suspect the other file will go away. I had started adding some of them anyway when debugging the other test cases and it seemed like we should just have them at a finer granularity.

@iyzhang iyzhang added the enhancement Enhancement Request on an Existing Feature label Nov 16, 2023
@iyzhang iyzhang requested a review from ppenna November 16, 2023 01:05
@iyzhang iyzhang self-assigned this Nov 16, 2023
@iyzhang iyzhang force-pushed the enhancement-inetstack-use-state-machine branch from b755b36 to ae5a3b1 Compare November 16, 2023 01:21
@iyzhang iyzhang requested a review from anandbonde November 16, 2023 01:28
Copy link
Collaborator

@ppenna ppenna left a comment

Choose a reason for hiding this comment

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

Look good to me. Thanks for working on this unification.

@iyzhang iyzhang force-pushed the enhancement-inetstack-use-state-machine branch from ae5a3b1 to c38446d Compare November 16, 2023 16:50
@iyzhang iyzhang merged commit a633ce3 into dev Nov 16, 2023
21 checks passed
@iyzhang iyzhang deleted the enhancement-inetstack-use-state-machine branch November 16, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement Request on an Existing Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants