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

Design proposal for new ring buffer API #3848

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

mikeagun
Copy link
Collaborator

@mikeagun mikeagun commented Sep 18, 2024

Description

Add docs/RingBuffer.md with proposal for new ebpf ring buffer map API exposing mapped memory so consumers can directly read the records written by the producer (similar to linux mmap/epoll style ring buffer consumer).

Testing

N/A

Documentation

Documentation only.

Installation

N/A

Alan-Jowett
Alan-Jowett previously approved these changes Sep 23, 2024
@mikeagun mikeagun changed the title New ringbuffer proposal Design proposal for new ring buffer API Sep 24, 2024
docs/RingBuffer.md Show resolved Hide resolved
docs/RingBuffer.md Outdated Show resolved Hide resolved
docs/RingBuffer.md Outdated Show resolved Hide resolved
@mikeagun mikeagun marked this pull request as draft October 2, 2024 22:51
@mikeagun
Copy link
Collaborator Author

mikeagun commented Oct 2, 2024

Converting to draft while I make a few design updates.

@mikeagun mikeagun marked this pull request as ready for review October 3, 2024 19:03
docs/RingBuffer.md Outdated Show resolved Hide resolved
docs/RingBuffer.md Show resolved Hide resolved
docs/RingBuffer.md Outdated Show resolved Hide resolved
matthewige
matthewige previously approved these changes Oct 9, 2024
* @returns Pointer to ring buffer manager.
*/
struct ring_buffer *
ring_buffer__new(int map_fd, ring_buffer_sample_fn sample_cb, void *ctx,
Copy link
Member

Choose a reason for hiding this comment

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

How do we differentiate between callback vs poll?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the design so callback consumers now use the libbpf ring buffer manager, and direct mapped memory consumers use the new functions.

// … log error …
break;
}
have_data = *prod_offset > *cons_offset; // It's possible we still have data.
Copy link
Member

Choose a reason for hiding this comment

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

You need a ReadAcquire here (or an explicit memory barrier). With out that, you can potentially read the record before its been written to by the producer. Likewise you will need a WriteRelease in the producer.

Copy link
Member

Choose a reason for hiding this comment

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

Also, reading it multiple times like this is going to cause issues. It should read prod_offset once using ReadAcquire and then use the value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now uses ReadAcquire64/WriteRelease64 for access to the producer and consumer offset.

I went with bit fields for the record header for simpler example code, so that part is already pseudocode and I didn't add the ReadAcquire needed to read the header, but added a comment in the struct.

Copy link
Member

@Alan-Jowett Alan-Jowett left a comment

Choose a reason for hiding this comment

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

Please fix the sample code. Its broken as it will not work correctly on weakly ordered systems (ARM64 as an example).

docs/RingBuffer.md Outdated Show resolved Hide resolved
Splits the design so callback consumers use the existing libbpf APIs,
and mapped memory consumers use the new windows-specific functions.

// Update consumer offset (and pad record length to multiple of 8).
consumer_offset += sizeof(rb_header_t) + (record->length + 7 & ~7);
WriteRelease64(cons_offset, consumer_offset);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

WriteNoFence64

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