Tracepoint naming isn't libbpf 1:1 about libbpfgo HOT 11 CLOSED

What do you think about keeping the sugar : and also supporting the original separator?

func (p *BPFProg) AttachTracepoint(tp string) (*BPFLink, error) { 
 	tpEvent := strings.FieldsFunc(tp, func(r rune) bool { return r == ':' || r == '/' })
 	if len(tpEvent) != 2 {
 		return nil, fmt.Errorf("tracepoint must be in 'category{:,/}name' format") 
 	} 

To be honest, I don't see any advantage supporting both of these. It is more confusing to me than useful.
I do see, however, an advantage of spliting this into two arguments like libbpf does. The separation is then clearer between the tracepoint category and name, and there is no need to check for a specific format.

from libbpfgo.

The libbpf convention is to split the tracepoint SEC name using slash / as separator.

https://github.com/libbpf/libbpf/blob/a3c0cc19d4b93cb0b7088c5604b0cec1c6863fde/src/libbpf.c#L9433-L9456

Actually, this is how libbpf extracts the tracepoint category and name from the program section name.
When attaching a tracepoint to a bpf program, libbpf API accepts the category and tracepoint name arguments explicitly:
https://github.com/libbpf/libbpf/blob/a3c0cc19d4b93cb0b7088c5604b0cec1c6863fde/src/libbpf.c#L9426

from libbpfgo.

You're right. The libbpfgo AttachTracepoint(tp string) actually wraps the bpf_program__attach_tracepoint(struct bpf_program *prog, const char *tp_category, const char *tp_name).

So I suppose that they continue not 1:1, since libbpfgo function has an unique argument for the tracepoint and the libbf wrapped one uses two arguments explicitly.

I dug a bit more about the divergent separator used by the wrapped: /.

attach_tp() was introduced 2 years ago:

https://github.com/libbpf/libbpf/blame/a3c0cc19d4b93cb0b7088c5604b0cec1c6863fde/src/libbpf.c#L9433

Along with its use by SEC_DEF in the static const struct bpf_sec_def section_defs[] initialization:

https://github.com/libbpf/libbpf/blame/a3c0cc19d4b93cb0b7088c5604b0cec1c6863fde/src/libbpf.c#L7937-L7938

This struct is used by the public libbpf_attach_type_by_name() function:

https://github.com/libbpf/libbpf/blob/5579664205e42194e1921d69d0839f660c801a4d/src/libbpf.h#L182-L183

And by the public bpf_program__attach():

https://github.com/libbpf/libbpf/blob/5579664205e42194e1921d69d0839f660c801a4d/src/libbpf.h#L245-L246

What do you think?

from libbpfgo.

We currently don't wrap bpf_program__attach() which uses the section name to automatically attach a bpf program, but if we would have such a wrapper, it would have worked (attaching) when such section names are defined.

So I suppose that they continue not 1:1, since libbpfgo function has an unique argument for the tracepoint and the libbf wrapped one uses two arguments explicitly.

Yes, we are not 1:1 with libbpf bpf_program__attach_tracepoint(), and we can change the API to explicitly accept a tracepoint category and name. I don't mind to apply such a change, but I also think that being a wrapper doesn't say you always have to have the same API. For example, we chose to have a more "object oriented" approach in libbpfgo, and BPFProg/BPFMap/etc all have methods which can be invoked by it. This approach is different than libbpf's, but I think is more convenient.

from libbpfgo.

I got it and I and appreciate the approach used by libbpfgo. It's a very useful design.

What do you think about keeping the sugar : and also supporting the original separator?

func (p *BPFProg) AttachTracepoint(tp string) (*BPFLink, error) { 
 	tpEvent := strings.FieldsFunc(tp, func(r rune) bool { return r == ':' || r == '/' })
 	if len(tpEvent) != 2 {
 		return nil, fmt.Errorf("tracepoint must be in 'category{:,/}name' format") 
 	} 

from libbpfgo.

I agree.

func (p *BPFProg) AttachTracepoint(category, name string) (*BPFLink, error) { 

from libbpfgo.

ACTUALLY... before closing this, we should double check if this works after #34 fix is merged.

I don't see why not, but I'm curious...

Within the tracepoint probe attach execution logic:

bpf_program__attach_tracepoint() -> bpf_program__attach_tracepoint_opts() -> bpf_program__attach_perf_event_opts()

the backing function for the tracepoint attachment is the same as the one for the perf events.

AND I'm currently trying to merge the legacy kprobe support at:

https://x-lore.kernel.org/bpf/[email protected]/

but I'm waiting a patchset from Andrii to land so I can rebase:

https://x-lore.kernel.org/bpf/[email protected]/
https://x-lore.kernel.org/bpf/[email protected]/
https://x-lore.kernel.org/bpf/[email protected]/

I'm saying that because, in my patch... by Andrii's request, I did the following name convention for kprobe names:

+static int poke_kprobe_events(bool add, const char *name, bool retprobe, uint64_t offset) {
+	int fd, ret = 0;
+	pid_t p = getpid();
+	char cmd[192] = {}, probename[128] = {}, probefunc[128] = {};
+	const char *file = "/sys/kernel/debug/tracing/kprobe_events";
+
+	if (retprobe)
+		snprintf(probename, sizeof(probename), "kretprobes/%s_libbpf_%u", name, p);
+	else
+		snprintf(probename, sizeof(probename), "kprobes/%s_libbpf_%u", name, p);
+
+	if (offset)
+		snprintf(probefunc, sizeof(probefunc), "%s+%lu", name, offset);
+
+	if (add) {
+		snprintf(cmd, sizeof(cmd), "%c:%s %s",
+			 retprobe ? 'r' : 'p',
+			 probename,
+			 offset ? probefunc : name);
+	} else {
+		snprintf(cmd, sizeof(cmd), "-:%s", probename);
+	}
+
+	fd = open(file, O_WRONLY | O_APPEND, 0);
+	if (!fd)
+		return -errno;
+	ret = write(fd, cmd, strlen(cmd));
+	if (ret < 0)
+		ret = -errno;
+	close(fd);
+
+	return ret;
+}

from libbpfgo.

@rafaeldtinoco I'm not sure I understand how this kprobe patch is related to the API we choose for attaching a tracepoint.
Is there another suggestion for this API signature?

from libbpfgo.

@rafaeldtinoco I'm not sure I understand how this kprobe patch is related to the API we choose for attaching a tracepoint.
Is there another suggestion for this API signature?

Things such as:

"kretprobes/%s_libbpf_%u", name, p);
"kprobes/%s_libbpf_%u", name, p);
"%s+%lu", name, offset);

are used from the 'name' argument, coming from sec_name within the bpf object structure. I just want to make sure any convention (or not convention) we use won't brake libbpf assumptions.

from libbpfgo.

I just want to make sure any convention (or not convention) we use won't brake libbpf assumptions.

The current API libbpfgo defines is:
func (p *BPFProg) AttachTracepoint(tp string) (*BPFLink, error)
Which uses ':' to split the tracepoint category and name.

This is different from what libbpf uses in its API:
https://github.com/libbpf/libbpf/blob/a3c0cc19d4b93cb0b7088c5604b0cec1c6863fde/src/libbpf.c#L9426
struct bpf_link *bpf_program__attach_tracepoint(struct bpf_program *prog, const char *tp_category, const char *tp_name)
Which explicitly accepts the tracepoint category and name as different arguments.

This is exactly the change suggested above:
func (p *BPFProg) AttachTracepoint(category, name string) (*BPFLink, error)

So, from my understanding, this has nothing to do with the section name (or anything derived from it)

from libbpfgo.

This is exactly the change suggested above:
func (p *BPFProg) AttachTracepoint(category, name string) (*BPFLink, error)

So, from my understanding, this has nothing to do with the section name (or anything derived from it)

So, I have tested this change within new kernels:

#77 (comment)

and it works. It does not work in older kernels though:

#77 (comment)

Still needs investigation to discover why.

from libbpfgo.