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

feat(ringbuf): Add AvailableBytes() #1533

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

dave-tucker
Copy link
Contributor

@dave-tucker dave-tucker commented Aug 5, 2024

Adds AvailableBytes(), which is equivalent to ring__avail_data_size in libbpf.

@ti-mo
Copy link
Collaborator

ti-mo commented Aug 6, 2024

Hi @dave-tucker, could you give an example of a practical use case? A test would be nice to have as well. Thanks!

@dave-tucker
Copy link
Contributor Author

@ti-mo added some assertions to the tests to exercise this API.
Use case is for doing ring buffer tuning work. For example, you may have some logic in your bpf program that decides whether to use BPF_RB_NO_WAKEUP or BPF_RB_FORCE_WAKEUP in bpf_ringbuf_submit

By periodically calling AvailableData() we can see from userspace how far ahead the producer is from the consumer and whether we're at risk of the ringbuf "wrapping around" and overwriting unread records.

@brycekahle
Copy link
Contributor

Does #1167 accomplish what you need?

@dave-tucker
Copy link
Contributor Author

dave-tucker commented Aug 7, 2024

@brycekahle I can try it out and compare both.
The difference between the two approaches is that record.Remaining is calculated once every time a record is read.
AvailableData() will always show the difference between the consumer/producer values at any point in time, so it would handle the case of the producer pointer incrementing multiple times since last read, but we haven't been woken up to read an event yet.

@dave-tucker
Copy link
Contributor Author

dave-tucker commented Aug 7, 2024

@brycekahle I can try it out and compare both.

I tried it out.

  • AvailableData was really easy to wire up to a prometheus.GaugeFunc as it easily be queried once every scrape interval.
  • Record.Remaining was less easy. I used a prometheus.Gauge and Set the value within my ringbuf reading function after a record was read.

You can see this on the graphs below - Top is AvailableData(), Bottom is Record.Remaining
Screenshot 2024-08-07 at 14 35 42

The gap in the middle was when I restarted my process to only use BPF_RB_FORCE_WAKEUP when there are 10 entries in the ringbuffer - each entry is 80 bytes.

The top graph is more useful for my purposes of evaluating what ☝️ does to the ring buffer.

I find the bottom graph less useful for my purposes right now, but thinking it through that metric might be better suited to creating a histogram.

TL;DR I think both are useful

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.

Thanks for checking, very informative, especially with the graphs! You're right that putting .Remaining in a gauge isn't going to be very helpful. Did you end up trying with a histogram?

What did you decide to change based on the metrics? Increase buffer size? Wake up less frequently?

The upside of AvailableData is that it's simple to export and graph. The downside is that we can't extend the concept to perf, since that wraps multiple buffers. So far we've always tried to keep the API of both packages very similar, but that is not a deal breaker.

Left some nits, overall I agree with Florian that this is fine to merge.

ringbuf/reader.go Outdated Show resolved Hide resolved
ringbuf/reader_test.go Outdated Show resolved Hide resolved
ringbuf/reader_test.go Outdated Show resolved Hide resolved
ringbuf/reader.go Outdated Show resolved Hide resolved
@dave-tucker
Copy link
Contributor Author

Thanks for checking, very informative, especially with the graphs! You're right that putting .Remaining in a gauge isn't going to be very helpful. Did you end up trying with a histogram?

I played around with it and it looked promising but I ended up bailing on it because I couldn't find a satisfactory size for the buckets.

What did you decide to change based on the metrics? Increase buffer size? Wake up less frequently?

All the above. I'm still working on it, but I plan to write up what I've learned soon and I'll share it on Slack when I'm done.

The upside of AvailableData is that it's simple to export and graph. The downside is that we can't extend the concept to perf, since that wraps multiple buffers. So far we've always tried to keep the API of both packages very similar, but that is not a deal breaker.

Left some nits, overall I agree with Florian that this is fine to merge.

Addressed! Thanks!

Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

@dave-tucker Thanks for the context you've provided so far! Left one nit, it's common in our lib (and in Go in general) to name functions after the unit of data they return. 'Data' is redundant, since what else would a ring buffer contain? 🙂 'Bytes' would be more useful here. Thanks!

ringbuf/reader.go Outdated Show resolved Hide resolved
ringbuf/reader.go Outdated Show resolved Hide resolved
@dave-tucker
Copy link
Contributor Author

Thanks for the review and suggestions @ti-mo. I've incorporated them and force-pushed an update.

@dave-tucker dave-tucker changed the title feat(ringbuf): Add AvailableData() feat(ringbuf): Add AvailableBytes() Aug 13, 2024
Adds AvailableBytes(), which is equivalent to ring__avail_data_size in
libbpf.

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo
Copy link
Collaborator

ti-mo commented Aug 14, 2024

Pushed a small fixup, will merge when CI is green.

@ti-mo ti-mo merged commit a31a756 into cilium:main Aug 14, 2024
17 checks passed
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.

5 participants