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

Fix ringbuffer ring size reporting #1492

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

usamasaqib
Copy link
Contributor

@usamasaqib usamasaqib commented Jun 20, 2024

The data pages of a ring buffer are mapped twice in a contiguous virtual region to allow easy wrap around when writing data. The code returns the ring size as the capacity of this mmaped region. However, this is wrong since the kernel double maps the underlying pages.

This PR fixes this by reporting the ring size as half the capacity of the underlying mmap region.

@usamasaqib usamasaqib requested a review from florianl as a code owner June 20, 2024 12:41
Signed-off-by: usamasaqib <usamasaqib.96@live.com>
@usamasaqib usamasaqib force-pushed the usama.saqib/ringbuf-cap-fix branch from e0bad69 to dcc6137 Compare June 20, 2024 14:30
func (rr *ringReader) size() int {
return cap(rr.ring)
return cap(rr.ring) / 2
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to fix TestRingbufReader to accommodate this change.

@florianl
Copy link
Contributor

Here we mmap() memory, that is later reported via (rr *ringReader) size() int {} and is publicly accessable via (r *Reader) BufferSize() int {}

prod, err := unix.Mmap(mapFD, (int64)(os.Getpagesize()), os.Getpagesize()+2*size, unix.PROT_READ, unix.MAP_SHARED)

I'm not yet sure about the proposed change. Maybe updating the documentation to (r *Reader) BufferSize() int {} could help, as an alternative?

@brycekahle
Copy link
Contributor

brycekahle commented Jun 20, 2024

As the person who added BufferSize in #1167, the intent was to return the total amount of data the user could write into the ring buffer. We use this primarily to determine ring buffer utilization in tandem with Remaining.

@usamasaqib
Copy link
Contributor Author

Here we mmap() memory, that is later reported via (rr *ringReader) size() int {} and is publicly accessable via (r *Reader) BufferSize() int {}

prod, err := unix.Mmap(mapFD, (int64)(os.Getpagesize()), os.Getpagesize()+2*size, unix.PROT_READ, unix.MAP_SHARED)

I'm not yet sure about the proposed change. Maybe updating the documentation to (r *Reader) BufferSize() int {} could help, as an alternative?

Hey @florianl, in the mmap call we set the size as PAGE_SIZE + 2*size, where size is the max entries specified by the user. The call doubles it because of the unique memory layout of the buffer discussed above. However, the capacity infact is still size.

As Bryce mentioned, BufferSize is used as the total amount of data the buffer can hold, so it should be adjusted according to this.

Signed-off-by: usamasaqib <usamasaqib.96@live.com>
ringbuf/ring.go Outdated Show resolved Hide resolved
Signed-off-by: Florian Lehner <florianl@users.noreply.github.com>
@lmb lmb merged commit fc4f4c5 into cilium:main Jun 24, 2024
17 checks passed
@lmb
Copy link
Collaborator

lmb commented Jun 24, 2024

Thanks all!

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.

4 participants