Skip to content

Add rust TcpSocket to network interface as LegacySocket#2626

Merged
stevenengler merged 1 commit intoshadow:mainfrom
stevenengler:wrap-tcp-rust
Dec 21, 2022
Merged

Add rust TcpSocket to network interface as LegacySocket#2626
stevenengler merged 1 commit intoshadow:mainfrom
stevenengler:wrap-tcp-rust

Conversation

@stevenengler
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler commented Dec 21, 2022 •
edited
Loading

When #2611 was merged, it added the first use of rust InetSocket objects in the network interface. This seems to have affected the graphs in the tgen benchmark.

1671657833_grim
1671657857_grim

I don't think the simulation itself changed, just the tracker was became broken since only metrics that came from Shadow changed, but not metrics that came from tgen.

This PR changes it back to adding the C LegacySocket to the network interface instead, which causes the shadow metrics to go back to how they were before.

1671658236_grim
1671658261_grim

@stevenengler stevenengler self-assigned this Dec 21, 2022
@github-actions github-actions bot added the Component: Main Composing the core Shadow executable label Dec 21, 2022
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 21, 2022 •
edited
Loading

Codecov Report

Base: 67.47% // Head: 67.71% // Increases project coverage by +0.24% 🎉

Coverage data is based on head (8e000f4) compared to base (f866ed7).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2626      +/-   ##
==========================================
+ Coverage   67.47%   67.71%   +0.24%     
==========================================
  Files         200      200              
  Lines       29522    29523       +1     
  Branches     5791     5791              
==========================================
+ Hits        19919    19992      +73     
+ Misses       5017     4936      -81     
- Partials     4586     4595       +9
Flag Coverage Δ
tests 67.71% <100.00%> (+0.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/host/descriptor/socket/inet/mod.rs 29.03% <100.00%> (-12.44%) ⬇️
src/main/host/descriptor/socket/inet/tcp.rs 66.48% <0.00%> (-3.30%) ⬇️
...c/main/utility/synchronization/count_down_latch.rs 85.10% <0.00%> (-0.54%) ⬇️
src/main/core/sim_config.rs 54.14% <0.00%> (ø)
src/main/host/process.rs 85.35% <0.00%> (+0.27%) ⬆️
src/main/host/memory_manager/mod.rs 76.45% <0.00%> (+0.32%) ⬆️
src/main/host/descriptor/mod.rs 67.22% <0.00%> (+0.41%) ⬆️
src/main/host/descriptor/pipe.rs 84.37% <0.00%> (+0.62%) ⬆️
src/lib/shadow-shim-helper-rs/src/signals.rs 70.12% <0.00%> (+0.64%) ⬆️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@stevenengler stevenengler marked this pull request as ready for review December 21, 2022 21:33
@sporksmith
Copy link
Copy Markdown
Contributor

sporksmith commented Dec 21, 2022

I don't think the simulation itself changed, just the tracker was broken since only metrics that came from Shadow changed, but not metrics that came from tgen.

If we think the differences in the PR were due to fixing the tracker, why revert? Or is the plan to confirm that more carefully but revert in the meantime...?

@sporksmith
Copy link
Copy Markdown
Contributor

sporksmith commented Dec 21, 2022

Oh I think I mis-parsed. You're saying the previous change broke the tracker, so this should fix it?

@stevenengler
Copy link
Copy Markdown
Contributor Author

stevenengler commented Dec 21, 2022 •
edited
Loading

I don't think the simulation itself changed, just the tracker was broken since only metrics that came from Shadow changed, but not metrics that came from tgen.

If we think the differences in the PR were due to fixing the tracker, why revert? Or is the plan to confirm that more carefully but revert in the meantime...?

There are two parts to this. In #2603 support for rust InetSockets was added to the network interface, and in #2611 we added the first use of these InetSockets in the network interface. So the issue was probably introduced in #2603, but that code path was only "activated" in #2611. I'm not entirely sure what the underlying issue is since I had looked into this beforehand and didn't think it was an issue, but for now it's simple enough to revert to the old behaviour and I'll look into it more later.

My current guess is that we're not counting some bytes in the tracker for these rust sockets, leading to shadow undercounting the number of bytes received on these sockets.

@stevenengler stevenengler merged commit 7e029bd into shadow:main Dec 21, 2022
@stevenengler stevenengler deleted the wrap-tcp-rust branch December 21, 2022 23:08
@stevenengler stevenengler mentioned this pull request Jan 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sporksmith sporksmith sporksmith approved these changes

Assignees

@stevenengler stevenengler

Labels

Component: Main Composing the core Shadow executable

Projects

None yet

Milestone

Convert existing modules to Rust

Development

Successfully merging this pull request may close these issues.

2 participants

@stevenengler @sporksmith