Add BPF_MAP_TYPE_RINGBUF support with RingBufMap

[yobiscus]

Closes #161.

[yobiscus]

RINGBUF is a relatively new addition to the kernel. Should I put it behind a feature gate (default disabled)?

[rhdxmr]

Hello @yobiscus

It looks great! I feel interested in this PR a lot. Thanks for your
contribution :)
Actually I was working on ringbuf as well so I can discuss ringbuf.
I am glad to see this PR since it is a good opportunity to talk about the same
topic we both are interested in.

But first, it would be better to fix the build failure issue. I also feel embarassed
to see this failure since I also didn't experience this failure in my latest
development environment (fedora 35). But that's the point of distro build
tests. RedBPF tries to support the latest Ubuntu LTS which is 20.04 now.

Currently the default kernel of ubuntu 20.04 LTS is v5.4 which does not support
ring buffer. It's a General Availability(GA) kernel of ubuntu. So it would be
nice to make ringbuf feature as an optional feature not to cause build failure
in ubuntu 20.04 LTS. And nevermind ubuntu 18.04.. it's older ubuntu LTS so I'm
going to disable it soon.

[yobiscus]

Hey @rhdxmr, thanks for your interest. Personally I have been testing on Centos 8.5, which uses a slightly more awkward versioning system for the kernel (kernel version is 4.18 officially, but in reality it has all the BPF features of kernel 5.9).

I'll spin up a 20.04 VM and fix the build here with conditional compilation. It might take me a few days to learn about conditional compilation and to juggle other priorities, so thanks in advance for your patience :)

[rhdxmr]

Hey, @yobiscus

Personally I have been testing on Centos 8.5, which uses a slightly more
awkward versioning system for the kernel (kernel version is 4.18 officially,
but in reality it has all the BPF features of kernel 5.9).

4.18 + BPF of 5.9?! That sounds peculiar to me. But I'm just curious about why
they did that. Anyway it is interesting to see that they think BPF is important.

It might take me a few days to learn about conditional compilation and to
juggle other priorities

It's absolutely fine. Thank you for your effort.

[yobiscus]

I fixed the build in Ubuntu 20.04 and confirmed that use of ringbuf still works when the feature is explicitly enabled (on systems that support it). I realize documentation and examples may be useful to add as well. I can add those once we're happy with the implementation.

[rhdxmr]

Hello @yobiscus

I checked your code carefully and I leaved review messages.
Some of them are my opinion and perhaps seem pedantic
so if you have better idea, please share your thought with me.

I hope this review helpful.

[rsdy]

@yobiscus Sorry to nudge you on this, should we expect a follow up patch? I'd be keen on merging this functionality, and I think this patchset is 95% there.

[yobiscus]

@rsdy no problem, apologies for the delay. I was preoccupied with other duties over the last couple of weekends. Let me try to address all outstanding issues this coming weekend.

[yobiscus]

AFAIK all review comments have been addressed. Please let me know if there are other outstanding items. Thanks!

[rhdxmr]

Thanks for your follow-up modifications.

I added some comment.

And please modify your new 3 commit messages to contain Signed-off-by trailer.

[rhdxmr]

As a brief usage you showed before,
it is convenient to make use of this method if it works correctly.
Users can directly assign values to the fields of the returned mutable reference.

However, I am still worried the safety of this method..
In BPF program, how can users make sure that the pointer is properly aligned?
I heard that the pointer arithmetic is not permitted in the BPF program.
I tried ptr as usize % mem::align_of::<T>() but the resulting BPF bytecode is rejected by BPF verifier.

And passing a struct by value and calling write_unaligned seems not incur double copy.
Fields of a struct are directly written to the place where bpf_ringbuf_reserve returns.
I guess the copy is opted out.
I inspected a resulting BPF bytecode.

But I am not sure that all types of data will show the same result.
I just tested a structure that has 5 fields and the size of the struct is 24 bytes.

[nbaksalyar]

I've used this branch to write some production-ish code and I have some feedback.

This works like a charm but I think the API here is a bit restrictive because there's no easy way to listen on ringbufs individually. Example case: I want to start my own async task for each ringbuf to forward incoming events to a TCP connection. With this approach, I'll have to filter all events to look for particular ringbufs that interest me, then likely forward these events through an mpsc queue, etc. etc.

Instead, I propose to store ringbufs in a separate HashMap:

Suggested change

	let fut = stream.for_each(move |events| {
	s.start_send((name.clone(), events)).unwrap();
	future::ready(())
	});
	let mut ringbufs = HashMap::new();
	...
	ringbufs.insert(name, stream);

and make it publicly accessible from struct Loaded, e.g.:

pub struct Loaded {
    pub events: mpsc::UnboundedReceiver<(String, <PerfMessageStream as Stream>::Item)>,
    //...
    pub ringbufs: HashMap<String, RingBufMessageStream>,
}

This would also simplify the implementation.

What do you think? cc @rhdxmr

@yobiscus and thanks for working on this, it's an awesome feature! :)

[rsdy]

@nbaksalyar that makes a lot of sense.

I'm afraid this PR is a bit stale, so at this stage we might need another revision, + we can't merge without DCO from @yobiscus . Hang in there for another week or so to see if they re-appear, and we can figure out a way forward if not.

[yobiscus]

Thanks for the much needed poke @nbaksalyar :) I am currently updating my environment so that it is compatible with the latest redbpf changes. I have rebased my changes against the main branch and hope to push something functional tomorrow.

Unfortunately I have not found time to address @rhdxmr comments above, specifically the comment about pointer alignment. There's a bit of a learning curve needed to address this which I don't currently have bandwidth for. Any help closing on these issues after I rebase my branch is very much appreciated!

[yobiscus]

Pushed and signed-off. Sorry I wasn't able to implement your suggestion @nbaksalyar. Since this content is feature gated, would it be okay to merge this as is and address the comments after-the-fact, even if the changes are not backwards compatible?

[nbaksalyar]

Sure, no worries! I can help with making this change if you'd like @yobiscus.

[rsdy]

Thanks everyone, this looks good, once the CI passes I can merge this!