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

Handle hot-path time syscalls in the shim #1224

Merged
merged 7 commits into from Mar 29, 2021
Merged

Handle hot-path time syscalls in the shim #1224

merged 7 commits into from Mar 29, 2021

Conversation

robgjansen
Copy link
Member

Shadow stores the latest sim time in memory shared with the shim. When time-related syscalls are executed, the shim side can read the latest simulation time directly from shared memory and avoid the more expensive inter-process syscall serviced by shadow.

Fixes #1141

@robgjansen robgjansen added Type: Enhancement New functionality or improved design Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable Tag: Performance Related to improving shadow's run-time labels Mar 29, 2021
@robgjansen robgjansen self-assigned this Mar 29, 2021
@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #1224 (aa3e207) into dev (10966c5) will increase coverage by 0.56%.
The diff coverage is 51.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1224      +/-   ##
==========================================
+ Coverage   55.47%   56.03%   +0.56%     
==========================================
  Files         140      141       +1     
  Lines       20300    20443     +143     
  Branches     4910     4944      +34     
==========================================
+ Hits        11261    11455     +194     
+ Misses       6015     5931      -84     
- Partials     3024     3057      +33     
Flag Coverage Δ
tests 56.03% <51.27%> (+0.56%) ⬆️

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

Impacted Files Coverage Δ
src/support/logger/logger.c 64.86% <0.00%> (+27.02%) ⬆️
src/shim/preload_syscall.c 22.58% <16.66%> (+3.53%) ⬆️
src/main/host/process.c 70.50% <33.33%> (-0.29%) ⬇️
src/shim/shim.c 40.90% <43.66%> (+19.85%) ⬆️
src/shim/shim_syscall.c 54.54% <54.54%> (ø)
src/main/host/thread_ptrace.c 55.20% <91.66%> (+3.95%) ⬆️
src/shim/shim_event.c 66.66% <100.00%> (+66.66%) ⬆️
src/shim/shim_logger.c 84.21% <100.00%> (+84.21%) ⬆️
src/main/host/syscall/time.c 42.55% <0.00%> (-14.90%) ⬇️
src/main/host/syscall_handler.c 51.29% <0.00%> (-0.44%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10966c5...aa3e207. Read the comment docs.

@robgjansen robgjansen marked this pull request as ready for review March 29, 2021 01:38
Copy link
Contributor

@sporksmith sporksmith left a comment

Choose a reason for hiding this comment

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

Looks great!

Might we want a command-line flag to disable this for benchmarking/debugging purposes?

#include "shim/shim_syscall.h"
#include "support/logger/logger.h"

#define SIMTIME_NANOS_PER_SEC 1000000000l
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the ending 1's? And should we move these constants to a header shared between shadow and the shim to ensure they stay sync'd?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an l (a lower case L). I think I refactored this from a previously defined constant to this #define, but yeah we should use an existing definition or create one (e.g. in definitions.h).

_cached_simulation_time.tv_nsec;
}

bool shim_syscall_is_supported(long syscall_num) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of trying to keep this in sync with which syscalls are supported in shim_syscall, could the caller just try calling shim_syscall and fallback if it returns false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that was one wrinkle I noticed but left in place. But I agree and I'll change it.

@robgjansen robgjansen merged commit cc5f4f6 into shadow:dev Mar 29, 2021
@robgjansen robgjansen deleted the shmem-time branch March 29, 2021 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable Tag: Performance Related to improving shadow's run-time Type: Enhancement New functionality or improved design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants