Support BTF-based pretty printing #2651

chengshuyi · 2023-06-21T04:01:00Z

A short example: bpftrace -e 'kprobe:kfree_skb {printb((struct sk_buff *)arg0)}'. It will display complete struct sk_buff information, which can improve debugging efficiency. Part of the output is shown below:

(struct sk_buff){
        (union){
                (struct){
                        .next = (struct sk_buff *)(nil),
                        .prev = (struct sk_buff *)(nil),
                        (union){
                                .dev = (struct net_device *)(nil),
                                .dev_scratch = (long unsigned int)0,
                        },
                },
...

Checklist

Language changes are updated in man/adoc/bpftrace.adoc and if needed in docs/reference_guide.md
User-visible and non-trivial changes updated in CHANGELOG.md
The new behaviour is covered by tests

add the keyword `printb` which means BTF-based pretty printing. Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

Add `struct BTFDump` to save `struct btf_dump` information. In general, one BTF corresponds to one `struct BTFDump`. In addition, a new `MessageType::printb` has been added for printing formatted structure data. Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

`Type::printb` is the type of the printb function. `PrintBTF` is the private information of this type, used to store dump_id, type_id and size information. - dump_id: indicates the BTFDump corresponding to the printed structure - type_id: indicates the id of the structure in BTF - size: indicates the size of the structure Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

The main checks are: - Refer to non-map print, printb also only accepts one parameter - The parameter type received by printb must be a pointer type, and the pointer points to the record type In addition, the maximum value size of the percpu map that needs to be created is also recorded. Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

The resources involved are: - save the parameter of printb - create percpu map Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

- Refer to CreateGetJoinMap, add CreateGetPrintBTFMap to get percpu memory. - Created new async event type `struct PrintBTF`. Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

viktormalik · 2023-06-21T06:33:35Z

This seems closely related to #1834. That one also suggest to use the bpf_snprintf_btf helper which produces practically the same output as your implementation here. In addition, it doesn't need async approach so I'd be inclined towards it more.

chengshuyi · 2023-06-21T07:02:39Z

This seems closely related to #1834. That one also suggest to use the bpf_snprintf_btf helper which produces practically the same output as your implementation here. In addition, it doesn't need async approach so I'd be inclined towards it more.

bpf_snprintf_btf also needs an async approach, because bpf_snprintf_btf also needs to allocate a large piece of memory through map to store formatted data.

I use the current approach for several reasons:

Better compatibility, can run on the older kernel.
The size of the required map value is determined, but bpf_snprintf_btf cannot be determined
This method is also implemented in BCC, see https://github.com/iovisor/bcc/blob/master/libbpf-tools/ksnoop.c#L756
The original binary data transmitted, compared with string data, the amount of transmitted data is smaller

Thanks!

viktormalik · 2023-06-21T07:21:08Z

This seems closely related to #1834. That one also suggest to use the bpf_snprintf_btf helper which produces practically the same output as your implementation here. In addition, it doesn't need async approach so I'd be inclined towards it more.

bpf_snprintf_btf also needs an async approach, because bpf_snprintf_btf also needs to allocate a large piece of memory through map to store formatted data.

Good point, I didn't realize that.

I use the current approach for several reasons:

Better compatibility, can run on the older kernel.

The size of the required map value is determined, but bpf_snprintf_btf cannot be determined

This method is also implemented in BCC, see https://github.com/iovisor/bcc/blob/master/libbpf-tools/ksnoop.c#L756

The original binary data transmitted, compared with string data, the amount of transmitted data is smaller

These are all valid reasons, thanks! It seems that even the ksnoop tool, which was the original use-case for bpf_snprintf_btf doesn't use it anymore.

I'll try to get to review this soon. Thanks for the work!

viktormalik · 2023-06-28T12:50:47Z

src/ast/passes/semantic_analyser.cpp

+      return;
+
+    Expression *arg = call.vargs->at(0);
+    if (arg->type.type == Type::pointer)


Could we directly accept a record here? It would require to update the codegen but would be quite a nice feature. In addition, codegen could end up being even simpler than now as printb could directly use any data that it receives (e.g. from the dereference operator).

I feel pointer could be better. Because from the scripting point of view, it will be a little simpler. For example printb($skb->sk) instead of printb(*($skb->sk)). What do you think ?

If you go with structs, it will allow to use printb in more use-cases, e.g. @ = *($skb->sk); ... printb(@). Also, it'll be easier implementation-wise b/c now you're always doing proberead to access the memory but that's not always the correct way (e.g. BTF-based probes don't need it).

If you go with structs, it will allow to use printb in more use-cases, e.g. @ = *($skb->sk); ... printb(@).

In fact, it is now supported. `@=$skb->sk; ... printb(@);.

Also, it'll be easier implementation-wise b/c now you're always doing proberead to access the memory but that's not always the correct way (e.g. BTF-based probes don't need it).

I suddenly remembered why I can't dereference directly, because the general kernel structure is relatively large, and if I dereference it directly, it will occupy the eBPF stack space.

In fact, it is now supported. `@=$skb->sk; ... printb(@);.

True, but that requires kernel to be able to store pointers into maps which hasn't been around for long.

I suddenly remembered why I can't dereference directly, because the general kernel structure is relatively large, and if I dereference it directly, it will occupy the eBPF stack space.

IIUC, proberead will also read the data onto BPF stack so there's no difference on that front.

Since printb is printing the structure, it still feels natural to me to pass directly the structure and not a pointer to the call.

In fact, it is now supported. `@=$skb->sk; ... printb(@);.

True, but that requires kernel to be able to store pointers into maps which hasn't been around for long.

I suddenly remembered why I can't dereference directly, because the general kernel structure is relatively large, and if I dereference it directly, it will occupy the eBPF stack space.

IIUC, proberead will also read the data onto BPF stack so there's no difference on that front.

We can directly use proberead to read the data into the map. The prototype of proberead is long bpf_probe_read(void *dst, u32 size, const void *unsafe_ptr), we can set dst as the value pointer of map.

Since printb is printing the structure, it still feels natural to me to pass directly the structure and not a pointer to the call.

We can directly use proberead to read the data into the map. The prototype of proberead is long bpf_probe_read(void *dst, u32 size, const void *unsafe_ptr), we can set dst as the value pointer of map.

Fair point, I didn't realize that. Could we do the same for BTF-based probes by directly loading into the map?

Also, I'm wondering if LLVM would be able to optimize away the stack usage even if we had it in the codegen. If not, perhaps it'd make sense to support both variants (pointer and non-pointer)?

We can directly use proberead to read the data into the map. The prototype of proberead is long bpf_probe_read(void *dst, u32 size, const void *unsafe_ptr), we can set dst as the value pointer of map.

Fair point, I didn't realize that. Could we do the same for BTF-based probes by directly loading into the map?

I understand it should be possible, we can even use percpu map instead of eBPF stack. That is, store all the data in the percpu map.

Also, I'm wondering if LLVM would be able to optimize away the stack usage even if we had it in the codegen. If not, perhaps it'd make sense to support both variants (pointer and non-pointer)?

Sorry, I don't quite understand the meaning here.:)

The question was that if we have something like this in codegen:

get pointer to skb proberead skb to BPF stack store the data (from BPF stack) into printb map

if LLVM optimizations are able to turn it into

get pointer to skb proberead skb to printb map

If not, then, we'll have to do the optimization on the level of the printb call and then, passing a pointer seems a better option implementation-wise. On the other hand, user-wise, it seems more appropriate to pass a structure so I'm thinking if we could allow both.

I understand what you mean, llvm should not have this optimization, because the two of them are completely independent memory spaces.

viktormalik · 2023-06-28T13:38:45Z

src/ast/passes/semantic_analyser.cpp

+        if (is_final_pass())
+        {
+          auto ids = bpftrace_.btf_->get_dumpid_typeid(ty->GetName());
+          if (ids.first < 0 || ids.second < 0)
+          {
+            LOG(ERROR, call.loc, err_)
+                << "Failed to get BTFDump for " << ty->GetName();
+            return;
+          }
+          // extra space for async event header, see `struct PrintBTF`'s
+          // definition in async_event_types.h
+          bpftrace_.max_printb_map_size_ = std::max(
+              bpftrace_.max_printb_map_size_, ty->GetSize() + 8 + 8);
+          call.type = CreatePrintBTF(ids.first, ids.second, ty->GetSize());
+        }


I think that this should be done in ResourceAnalyser instead of here. Also, we don't need a new type as printb doesn't return anything and it cannot be used in expressions, so we should just use the none type here.

Ok, will do, thanks.

viktormalik · 2023-06-28T13:52:09Z

src/btf.h

@@ -72,6 +79,12 @@ class BTF

  std::pair<int, int> get_btf_id_fd(const std::string& func,
                                    const std::string& mod) const;
+  // get dump id and type id
+  std::pair<int, int> get_dumpid_typeid(const std::string& type_name);


The dump id/type id pair is also used in other places outside of this class. Maybe it'd be worth defining a type for it?

Ok, will do, thanks!

danobi · 2023-07-07T15:42:37Z

Is it necessary to have a new printb() builtin? I realize we can dump structs using print() today so there's symmetry with printb(), but I wonder if it's better to have printf() format string specifiers instead. Those are composable and will come out atomically with other metadata the user might have (rather than possibly being dropped due to ring buffer overflow).

It would feel quite natural to have a specifier for raw struct dump as well as pretty printed dump.

chengshuyi · 2023-07-08T05:08:59Z

Using printf is also acceptable. We can convert it into a string through other builtins, such as printf("%s", btf(skb));. Or, we can use something like {:?} in rust. However, I haven't figured out what symbols should be used in printf yet.

viktormalik · 2023-07-10T12:36:59Z

Having new printf format specifiers sounds good to me, although I'm not sure if it won't be confusing to users as they'd expect printf to have the "commonly known specifiers" only. OTOH, we already have a few custom specifiers so there's a precedent for it (and it'd be documented). I like it better than adding new builtins.

However, I haven't figured out what symbols should be used in printf yet.

Good question, wdyt @danobi?

Maybe %t (as in "struct type"), optionally followed by an extra modifier, i.e. %tr for raw struct and %th or %tp for pretty-printed struct?

danobi · 2023-07-10T16:54:48Z

The kernel seems to dump most of their pointer-y printf extensions under %p prefix: https://www.kernel.org/doc/html/latest/core-api/printk-formats.html#pointer-types

Perhaps we can try to follow that? A lot of kernel devs use bpftrace so it would probably feel natural for them.

chengshuyi · 2023-07-11T10:02:10Z

Now printf uses stack memory, do we need to change printf to use percpu map memory first? Because the stack memory is only 512 bytes, it is definitely not enough.

danobi · 2023-07-12T22:52:30Z

@chengshuyi good point. Improving printf() to use percpu map memory would be a nice change. I don't know off the top of my head how much work it'd be, though.

I'd rather not block this PR on a big refactor, as printb() does provide some level of symmetry with printf(). But I think it'd be nice not to add too many more builtin functions.

Mind taking a look at how much effort a printf() refactor is? If it's an obnoxiously large amount of work, maybe we should merge this first.

chengshuyi requested review from ajor, viktormalik, danobi and fbs as code owners June 21, 2023 04:01

chengshuyi added 7 commits June 21, 2023 13:29

lexer: add the keyword printb

ffa80ee

add the keyword `printb` which means BTF-based pretty printing. Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

resource: resource analysis of printb

d746b0a

The resources involved are: - save the parameter of printb - create percpu map Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

codegen: generate ir of printb

9c0b88d

- Refer to CreateGetJoinMap, add CreateGetPrintBTFMap to get percpu memory. - Created new async event type `struct PrintBTF`. Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

process the received typed data

0e8d822

Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>

chengshuyi force-pushed the printb branch from c0a0d04 to 0e8d822 Compare June 21, 2023 05:31

viktormalik reviewed Jun 28, 2023

View reviewed changes

xh4n3 mentioned this pull request Aug 22, 2023

add a MapError async event type for join() #2719

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support BTF-based pretty printing #2651

Support BTF-based pretty printing #2651

chengshuyi commented Jun 21, 2023 •

edited

viktormalik commented Jun 21, 2023

chengshuyi commented Jun 21, 2023 •

edited

viktormalik commented Jun 21, 2023

viktormalik Jun 28, 2023

chengshuyi Jun 29, 2023 •

edited

viktormalik Jun 29, 2023

chengshuyi Jun 29, 2023 •

edited

viktormalik Jun 29, 2023

chengshuyi Jun 29, 2023

viktormalik Jun 29, 2023

chengshuyi Jun 29, 2023 •

edited

viktormalik Jun 29, 2023

chengshuyi Jul 3, 2023

viktormalik Jun 28, 2023

chengshuyi Jun 29, 2023

viktormalik Jun 28, 2023

chengshuyi Jun 29, 2023

danobi commented Jul 7, 2023

chengshuyi commented Jul 8, 2023

viktormalik commented Jul 10, 2023

danobi commented Jul 10, 2023

chengshuyi commented Jul 11, 2023

danobi commented Jul 12, 2023

Support BTF-based pretty printing #2651

Are you sure you want to change the base?

Support BTF-based pretty printing #2651

Conversation

chengshuyi commented Jun 21, 2023 • edited

Checklist

viktormalik commented Jun 21, 2023

chengshuyi commented Jun 21, 2023 • edited

viktormalik commented Jun 21, 2023

Choose a reason for hiding this comment

chengshuyi Jun 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chengshuyi Jun 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chengshuyi Jun 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danobi commented Jul 7, 2023

chengshuyi commented Jul 8, 2023

viktormalik commented Jul 10, 2023

danobi commented Jul 10, 2023

chengshuyi commented Jul 11, 2023

danobi commented Jul 12, 2023

chengshuyi commented Jun 21, 2023 •

edited

chengshuyi commented Jun 21, 2023 •

edited

chengshuyi Jun 29, 2023 •

edited

chengshuyi Jun 29, 2023 •

edited

chengshuyi Jun 29, 2023 •

edited