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: AddRingBuf() using ring_buffer__add #430

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mighty1231
Copy link

@mighty1231 mighty1231 commented May 20, 2024

libbpf provides ring_buffer__add to handle multiple ringbuf event callbacks

  • single RingBuffer object now handles multiple slots
  • implements AddRingBuf() for ring_buffer__add wrapper
  • updates ringbuffers selftest to show how to handle it

Reference

@CLAassistant
Copy link

CLAassistant commented May 20, 2024

CLA assistant check
All committers have signed the CLA.

@mighty1231 mighty1231 force-pushed the feat/add-ring-buf branch 2 times, most recently from 26b1112 to f4f8407 Compare May 20, 2024 06:19
@geyslan
Copy link
Member

geyslan commented May 21, 2024

@mighty1231 tks for this PR. There's a formatting issue https://github.com/aquasecurity/libbpfgo/pull/430/checks. As it is solved I'll stash it for later review. Tks again.

@mighty1231
Copy link
Author

mighty1231 commented May 22, 2024

@geyslan I ran make fmt-fix. Now make fmt-check returns no error

EDIT: with new amended commit, make lint-check also returns no error

@mighty1231 mighty1231 force-pushed the feat/add-ring-buf branch 2 times, most recently from 14e07f1 to 6b6b706 Compare May 27, 2024 00:43
@mighty1231
Copy link
Author

mighty1231 commented May 28, 2024

Failed test map-getmapsbyname seems not related to my PR, because it seems to have no ringbuffer-related code, and it runs before the ringbuffers test.

@geyslan
Copy link
Member

geyslan commented May 28, 2024

@mighty1231, I suspect that self-test/iter sometimes fails due to ping usage and internal network malfunction. Well, it seems like a flaky test.

@geyslan geyslan self-requested a review June 6, 2024 20:35
@geyslan geyslan added the feature New feature or request label Jun 6, 2024
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

@mighty1231 I tested it and it works nice. That's awesome.

I would just ask you to make the selftest more robust, since you have touched the logic of the slot (index) inside RingBuffer that's used by rwArray which .

Would you mind to improve the the respective selftest to submit a greater volume of data through a greater number of slots (ringbuffers) not reusing the same data holder? I mean, try using more than one program to submit the data.

libbpfgo.c Outdated
Comment on lines 66 to 70
fprintf(stderr, "Failed to add ring buffer: %s\n", strerror(-ret));
return ret;
Copy link
Member

Choose a reason for hiding this comment

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

Please use the same pattern of cgo_init_ring_buf regarding saving and restoring errno.

Copy link
Author

Choose a reason for hiding this comment

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

I changed it.

libbpf provides ring_buffer__add to handle multiple ringbuf event
callbacks

- single RingBuffer object now handles multiple slots
- implements AddRingBuf() for ring_buffer__add wrapper
- updates ringbuffers selftest to show how to handle it

Reference
- https://github.com/torvalds/linux/blob/eb6a9339efeb6f3d2b5c86fdf2382cdc293eca2c/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
- https://github.com/torvalds/linux/blob/eb6a9339efeb6f3d2b5c86fdf2382cdc293eca2c/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c#41
@mighty1231
Copy link
Author

mighty1231 commented Jun 10, 2024

@mighty1231 I tested it and it works nice. That's awesome.

I would just ask you to make the selftest more robust, since you have touched the logic of the slot (index) inside RingBuffer that's used by rwArray which .

Would you mind to improve the the respective selftest to submit a greater volume of data through a greater number of slots (ringbuffers) not reusing the same data holder? I mean, try using more than one program to submit the data.

I am confused. Which of the following did you mean?

  1. More tests to check thread safety of Module *AddRingBuf(), RingBuffer *Stop(), and RingBuffer *Close(), which modifies RingBuffer.slots
  2. More tests with two or more struct *RingBuffer in golang
  3. More tests wtih two or more NewModuleFromFile("main.bpf.o") call
  4. More tests wtih two or more probes - other than kprobe__sys_mmap from selftest/ringbuffers/main.bpf.c
  5. Make similar to ringbuf_multi test from linux repository - ringbuf_multi.c and test_ringbuf_multi.c

1 may be resolved with notifying the limitation for users. If it doesn't seem right, I would appreciate any feedback you may have.
2 is the motivation of this PR. I have got completely lost to use more than one RingBuffer, so I decided to use ring_buffer__add.
3, 4 - it's difficult to guess what can be improved for it
5 seems interesting. if you mean for it, I will take it

@geyslan
Copy link
Member

geyslan commented Jun 10, 2024

5. Make similar to ringbuf_multi test from linux repository

It would be awesome to have a huge amount of tests like ringbuf_multi. Anyway, I think it would take longer.

I meant:

  • increasing numberOfEvent*Received* to a considerable amount.
  • split the recvLoop for select into the amount of ringbuf slots, raising one goroutine for each.

@mighty1231
Copy link
Author

mighty1231 commented Jun 11, 2024

I don't know how to split the for-select loop, since I don't know golang much. And I didn't understand what is improved with new test.

@geyslan
Copy link
Member

geyslan commented Jun 13, 2024

@NDStrahilevitz do you mind reviewing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants