Skip to content

Conversation

brycekahle
Copy link
Contributor

This allows tracking utilization of the underlying buffers. Processing the values to make sense in a user's monitoring system is left to the user.

Using the post-read remaining bytes allows the value to reach zero, whereas a pre-read value would never reach zero.

@brycekahle brycekahle requested a review from lmb October 16, 2023 19:06
@brycekahle brycekahle force-pushed the bryce.kahle/perf-usage branch from 52c47f9 to 3b522ca Compare October 16, 2023 19:08
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Looks good, can you add test coverage please?

perf/reader.go Outdated
Remaining int

// The total size of the per-CPU perf buffer in bytes.
Size int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Size is fixed for the reader, why not expose this as Reader.Size() int instead? You can move the call to perfBufferSize from newPerfEventRing to NewReaderWithOptions. Same idea applies to ringbuf.Reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The size we are interested in, is the data portion, without the metadata page. Moving the call to perfBufferSize would've required changes to a bunch of other places that expect newPerfEventRing to handle the size manipulation. I did something different, let me know what you think.

@brycekahle brycekahle force-pushed the bryce.kahle/perf-usage branch from 433565f to 515ea9b Compare October 18, 2023 21:24
@brycekahle
Copy link
Contributor Author

Looks good, can you add test coverage please?

Added. For the backward/overwriteable buffer it isn't possible to get a precise remaining count once it has been overwritten.

@brycekahle brycekahle requested a review from lmb October 18, 2023 21:27
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for adding the tests! Final question: seems like the semantics for overwritable aren't clear. Should we just set remaining to -1 for now in that case and document that this means the data remaining can't be determined?

@brycekahle
Copy link
Contributor Author

Should we just set remaining to -1 for now in that case and document that this means the data remaining can't be determined?

We could. It does work correctly until any kind of overwrite happens, and then it becomes inaccurate by reporting more data remaining than their actually is (because of the partial leftover records).

@brycekahle brycekahle requested a review from lmb October 19, 2023 19:16
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Pushed a doc fix. Please squash then we are good to go.

Signed-off-by: Bryce Kahle <bryce.kahle@datadoghq.com>
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@brycekahle brycekahle force-pushed the bryce.kahle/perf-usage branch from 220e1e7 to 5a0992d Compare October 20, 2023 15:34
@brycekahle
Copy link
Contributor Author

squashed

@brycekahle
Copy link
Contributor Author

@lmb ready to go.

@lmb lmb merged commit 41377ae into cilium:main Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants