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

New CreateMap API implementing bpf_map_create, Type/name accessors #138

Merged
merged 3 commits into from Mar 23, 2022

Conversation

grantseltzer
Copy link
Contributor

No description provided.

Signed-off-by: grantseltzer <grantseltzer@gmail.com>
@grantseltzer grantseltzer changed the title New CreateMap API implementing bpf_map_create New CreateMap API implementing bpf_map_create, Type/name accessors Mar 15, 2022
Copy link
Contributor

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Looks awesome! Thanks for handling it, and examples are very self-explanatory. I think I'm gonna find some spare time next week to integrate this to Parca. I'll let you know how it goes.

I have just added a comment about tests in BPF program.

#include <bpf/bpf_helpers.h>

SEC("kprobe/sys_execve")
int kprobe__sys_execve(struct pt_regs *ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

// CreateMap creates a BPF map from userspace. This can be used for populating
// BPF array of maps or hash of maps. However, this function uses a low-level
// libbpf API; maps created in this way do not conform to libbpf map formats,
// and therefore do not have access to libbpf high level bpf_map__* APIS
// which causes different behavior from maps created in the kernel side code

Considering the comment above, is it possible to add links or example APIs here to demonstrate accessing the maps that are created in the user space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed a comment into the selftest with further explanation. If you want to try using this in Parca and are up for working on it together please tag me in the issue/PR and i'd gladly help figure it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

💯 Thanks!

Signed-off-by: grantseltzer <grantseltzer@gmail.com>
@grantseltzer
Copy link
Contributor Author

@yanivagman can you take a look at this when you have spare time

Copy link
Collaborator

@yanivagman yanivagman left a comment

Choose a reason for hiding this comment

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

Nice addition @grantseltzer
LGTM, just a small comment/question

@@ -341,6 +470,30 @@ func (m *Module) GetMap(mapName string) (*BPFMap, error) {
}, nil
}

func (b *BPFMap) Name() string {
cs := C.bpf_map__name(b.bpfMap)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quoting from CreateMap above:

// and therefore do not have access to libbpf high level bpf_map__* APIS

So, this won't work for maps created with CreateMap, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, the struct bpf_map is NULL which bpf_map__name() uses as it's parameter. CreateMap only provides a file descriptor for the map.

// with a file descriptor already. If the map is already associated
// with a file descriptor the libbpf API will return error code EBUSY
func (b *BPFMap) SetType(mapType MapType) error {
errC := C.bpf_map__set_type(b.bpfMap, C.enum_bpf_map_type(int(mapType)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

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.

None yet

3 participants