Skip to content

Conversation

@Phantomical
Copy link
Owner

These were originally added (in the perf_event crate) as a way to mock out the perf_event_open syscall and related ioctls for testing purposes. The problem ends up being that perf_event_open is a really complicated syscall. Writing a mock for most behaviour ends up being extremely hard to do.

I haven't managed to find a use case for this module and, according to sourcegraph, neither has anybody else. As such, I think removing it is the right thing to do. This is a breaking change.

Some notes on what makes mocking the system API not so useful

The hooks API allows a test to mock

  • the perf_event_open syscall, and,
  • all ioctls involved in modifying the resulting fd

However, actually using the API mostly involves:

  • reading from the returned fd to read the counter
  • mmaping the returned fd to access the sampling ringbuffer
  • interacting with said ringbuffer

The main features of this crate are:

  • A builder API to configure the perf_event_attr struct before calling the perf_event_open syscall
  • A wrapper around the returned fd to make reading from it easier, and,
  • A wrapper around the returned fd + the mmaped ring buffer to read records from the kernel ringbuffer (+ a few extras)

Of the 3 features, the only one that can really be usefully tested with the mocked hooks module is the second. The 1st can only be usefully tested against the kernel itself and the 3rd is too complicated to actually make a mock for. However, it is really quite easy to write some for the 2nd feature that work on any machine, which kinda gets rid of the whole point of mocking them in the first place.

The one exception is that github actions completely blocks the perf_event_open API for some reason and mocking it could allow a few tests to run in CI. While this is true, it's not incredibly useful since the tests that could actually run are rather limited. It is more useful to have contributors run the unit tests on their own machines before making the PR instead.

These were originally added (in the perf_event crate) as a way to mock
out the perf_event_open syscall and related ioctls for testing purposes.
The problem ends up being that perf_event_open is a really complicated
syscall. Writing a mock for most behaviour ends up being extremely hard
to do.

I haven't managed to find a use case for this module and, according to
sourcegraph, neither has anybody else. As such, I think removing it is
the right thing to do. This is a breaking change.

Some notes on what makes mocking the system API so hard
-------------------------------------------------------
The hooks API allows a test to mock
- the perf_event_open syscall, and,
- all ioctls involved in modifying the resulting fd

However, actually using the API mostly involves:
- reading from the returned fd to read the counter
- mmaping the returned fd to access the sampling ringbuffer
- interacting with said ringbuffer

The main features of this crate are:
- A builder API to configure the perf_event_attr struct before calling
  the perf_event_open syscall
- A wrapper around the returned fd to make reading from it easier, and,
- A wrapper around the returned fd + the mmaped ring buffer to read
  records from the kernel ringbuffer (+ a few extras)

Of the 3 features, the only one that can really be usefully tested with
the mocked hooks module is the second. The 1st can only be usefully
tested against the kernel itself and the 3rd is too complicated to
actually make a mock for. However, it is really quite easy to write some
for the 2nd feature that work on any machine, which kinda gets rid of
the whole point of mocking them in the first place.

The _one_ exception is that github actions completely blocks the
perf_event_open API for some reason and mocking it could allow a few
tests to run in CI. While this is true, it's not incredibly useful since
the tests that could actually run are rather limited. It is more useful
to have contributors run the unit tests on their own machines before
making the PR instead.
@Phantomical Phantomical marked this pull request as draft May 28, 2024 08:55
@Phantomical Phantomical added the breaking-change Merging this PR would require a major version bump label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Merging this PR would require a major version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants