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 15 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't follow. How does Linux differentiate? Why would we want a different answer?

docs/RingBuffer.md Outdated Show resolved Hide resolved
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

shpalani
shpalani previously approved these changes Oct 26, 2024
Comment on lines +17 to +20
1. Call `ebpf_ring_buffer_get_buffer` to get pointers to the mapped producer/consumer pages.
2. Call `ebpf_ring_buffer_get_wait_handle` to get the wait handle.
3. Directly read records from the producer pages (and update consumer offset as we read).
4. Call `WaitForSingleObject`/`WaitForMultipleObject` as needed to wait for new data to be available.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line 6 says Linux supports polling consumers. What APIs do callers use on Linux to get this behavior?
Why can't those APIs work on Windows? Why are new ebpf_ APIs needed?

This document doesn't appear to answer any of these questions.

* @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
Collaborator

Choose a reason for hiding this comment

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

I don't follow. How does Linux differentiate? Why would we want a different answer?

const struct ring_buffer_opts *opts);

/**
* @brief poll ringbuf for new data (NOT CURRENTLY SUPPORTED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not currently supported?

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.

7 participants