Skip to content

emulate sched_{get,set}affinity and sysconf(_SC_NPROCESSORS_*)#2602

Merged
stevenengler merged 8 commits intoshadow:mainfrom
trinity-1686a:sched-getaffinity
Dec 15, 2022
Merged

emulate sched_{get,set}affinity and sysconf(_SC_NPROCESSORS_*)#2602
stevenengler merged 8 commits intoshadow:mainfrom
trinity-1686a:sched-getaffinity

Conversation

@trinity-1686a
Copy link
Copy Markdown
Contributor

fix #2101

I've confirmed openjdk no longer warns about sched_getaffinity, and when disabling the syscall but keeping the overriding of sysfs, it complains and reports 1 online processor.

I did my best for the filesystem part, but I'm not very confident in writing C

@github-actions github-actions bot added Component: Build Build/install tools and dependencies Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks labels Dec 11, 2022
@trinity-1686a
Copy link
Copy Markdown
Contributor Author

hum, looks like in docker in ubuntu (and probably in ubuntu out of docker), sysconf(_SC_NPROCESSORS_CONF) does

00:00:01.000 [tid 1000] openat(-100, "/sys/devices/system/cpu", O_CLOEXEC | O_DIRECTORY | O_NDELAY | O_NONBLOCK, (empty)) = 3
00:00:01.000 [tid 1000] fstat(...) = 0
00:00:01.000 [tid 1000] getdents64(...) = 992
00:00:01.000 [tid 1000] getdents64(...) = 0
00:00:01.000 [tid 1000] close(3) = 0

instead of reading /sys/devices/system/cpu/possible like it does on my (archlinux) machine. I've also seen it try reading /proc/stat when it fails to parse /sys/devices/system/cpu/possible

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 11, 2022

Codecov Report

Base: 68.06% // Head: 66.87% // Decreases project coverage by -1.18% ⚠️

Coverage data is based on head (179c0a2) compared to base (3fbe804).
Patch coverage: 70.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2602      +/-   ##
==========================================
- Coverage   68.06%   66.87%   -1.19%     
==========================================
  Files         199      200       +1     
  Lines       29199    29279      +80     
  Branches     5735     5759      +24     
==========================================
- Hits        19873    19581     -292     
- Misses       4739     5128     +389     
+ Partials     4587     4570      -17     
Flag Coverage Δ
tests 66.87% <70.00%> (-1.19%) ⬇️

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

Impacted Files Coverage Δ
src/main/host/syscall/handler/sched.rs 44.77% <60.60%> (+18.30%) ⬆️
src/test/sched_affinity/test_sched_affinity.rs 75.00% <75.00%> (ø)
src/main/host/host.rs 79.92% <80.00%> (-0.75%) ⬇️
src/main/host/syscall/handler/mod.rs 77.50% <100.00%> (-8.40%) ⬇️
src/main/host/syscall/handler/eventfd.rs 0.00% <0.00%> (-68.58%) ⬇️
src/main/host/syscall/handler/time.rs 0.00% <0.00%> (-65.86%) ⬇️
src/main/host/descriptor/eventfd.rs 0.00% <0.00%> (-65.66%) ⬇️
src/main/host/syscall/handler/ioctl.rs 34.37% <0.00%> (-21.88%) ⬇️
src/main/host/syscall/handler/fcntl.rs 42.42% <0.00%> (-21.22%) ⬇️
src/main/host/descriptor/pipe.rs 66.25% <0.00%> (-18.75%) ⬇️
... and 29 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.

Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

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

I think in general we'd want a slightly different design for this.

  1. The union in RegularFile makes it easier to introduce bugs in the future since we have a single struct trying to emulate two different behaviours at once with different fields. I think we'd want a different struct for files representing kernel data (not a RegularFile).
  2. The type should be more flexible for supporting other shadow data, including data that isn't represented by a string. For example, we'd want a type that can represent things like /sys/devices/system/cpu/online, but also /proc/uptime and other dynamically updated kernel files.

But the RegularFile struct is essentially in maintenance mode at the moment and has some other issues, and we'll want to overhaul it when we move it to rust anyways. So I think it's fine to add this as-is. @sporksmith Any thoughts?

Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

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

Thanks! And for the merge conflict, feel free to rebase or merge main, whichever is easier.

@stevenengler stevenengler added this to the Support missing syscalls milestone Dec 15, 2022
@stevenengler stevenengler merged commit 7bdadc0 into shadow:main Dec 15, 2022
@sporksmith
Copy link
Copy Markdown
Contributor

Thanks both for your work on this! I'm looking forward to the reduced warning log noise (and corresponding element of uncertainty about program behavior when these fail) :)

@trinity-1686a trinity-1686a deleted the sched-getaffinity branch December 15, 2022 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Build Build/install tools and dependencies Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Emulate sched_getaffinity

3 participants