libbpf-rs: Addressed API code review

[danobi]

No description provided.

[danobi]

cc @anakryiko .

One thing I'm wondering is if we want an Object being dropped to unload the Programs or if we want to instead refcount LoadedMap and unload the program when all LoadedMap instances are dropped. It could probably map cleanly to libbpf b/c there's already a bpf_program__unload function.

Not sure if that would be necessary for LoadedMap. I'm looking at libbpf code and it doesn't look like there's any closing of map FDs. I'm guessing that's b/c the kernel will hold a refcount as long as the prog is active.

[anakryiko]

can fail, also doesn't need mutable reference, probably

[danobi]

If callbacks receive a mutable ref to PerfBuffer, I think it makes sense to keep this as mutable.

[anakryiko]

do callbacks need mutable reference? what for?

[danobi]

Do we need a reference back to the PerfBuffer at all? We don't currently have any properties that can really be changed after construction.

If we wanna add it later we can easily add a poll_mut() or poll_ref() method. But TBH I can't see a use case.

[anakryiko]

yeah, I don't have specific use case to have any kind of references, but it's usually a good idea to have reference to an "object" that's calling a callback function. E.g., just in case someone has multiple PerfBuffers, but the same callback. Having reference to PerfBuffer would allow to differentiate between them, if necessary.

But I think we are talking about different things here. PerfBuffer::poll needs &self -- I don't think PerfBuffer needs to be modified as a result of poll operation. What I was describing above is what should be passed to user-provided callback. There, I think, having PerfBuffer reference is useful.

[danobi]

Sounds reasonable

[anakryiko]

cc @anakryiko .

One thing I'm wondering is if we want an Object being dropped to unload the Programs or if we want to instead refcount LoadedMap and unload the program when all LoadedMap instances are dropped. It could probably map cleanly to libbpf b/c there's already a bpf_program__unload function.

I think unloading unconditionally. Map and Program are owned by Object, they can't leave outside of Object. libbpf doesn't do anything special here, when object is closed/destroyed, all map and program FDs are closed (see bpf_object__unload()). If map is referenced from some other program, it will be kept around by kernel.

Not sure if that would be necessary for LoadedMap. I'm looking at libbpf code and it doesn't look like there's any closing of map FDs. I'm guessing that's b/c the kernel will hold a refcount as long as the prog is active.

[danobi]

I think unloading unconditionally. Map and Program are owned by Object, they can't leave outside of Object.

Ok. We can make Object the guy that owns everything.

[danobi]

I'm gonna merge this and start implementing everything. Feel free to leave more comments on here if you see anything later.

I'll put up a PR for the implementation as well.

[anakryiko]

@danobi, sounds good, I think currently it looks good. You'll probably find some inconsistencies or missing things along the way, but nothing is set in stone and we can adjust and revisit as needed. Thanks for working on this!