bpftrace icon indicating copy to clipboard operation
bpftrace copied to clipboard

Deleting scalar map just zeros out the entry

Open danobi opened this issue 1 year ago • 2 comments

What reproduces the bug? Provide code if possible.

$ sudo ./build/src/bpftrace -e 'BEGIN { @=1; delete(@); exit() }'
Attaching 1 probe...

@: 0

This was added in 39f3996d ("Support delete() on count()-based map"). It seemed reasonable at the time, but thinking about this again, not sure it makes sense. Seems better to just disallow this. What is the user supposed to expect?

One edge case to consider is if user is storing aggregate type in scalar map. Eg:

 $ sudo ./build/src/bpftrace -e 'BEGIN { @=*curtask->objcg; exit() }'
Attaching 1 probe...


@: { .refcnt = { .percpu_count_ptr = 72848295460832, .data = 0xffff8a6c575ca840 }, .memcg = 0xffff8a6b41835000, .nr_charged_bytes = , .list = { .next = 0xffff8a6c575ca7a0, .prev = 0xffff8a6c575ca7a0 }, .rcu = { .next = 0xffff8a6c575ca7a0, .func = 0xffff8a6c575ca7a0 } }

There's no other way for user to synchronously set the value to 0, as they cannot construct an aggregate. But clear() and zero()` exist. Unfortunately they're asynchronous.

We could only allow delete() on aggregate scalar maps, but maybe that's getting too confusing.

An alternate acceptable answer is to just do nothing. Maybe things do make sense.

bpftrace --info output

System
  OS: Linux 6.10.8-arch1-1 #1 SMP PREEMPT_DYNAMIC Wed, 04 Sep 2024 15:16:37 +0000
  Arch: x86_64

Build
  version: v0.21.0-165-gcd68
  LLVM: 18.1.8
  bfd: yes
  liblldb (DWARF support): yes
  libsystemd (systemd notify support): no

Kernel helpers
  probe_read: yes
  probe_read_str: yes
  probe_read_user: yes
  probe_read_user_str: yes
  probe_read_kernel: yes
  probe_read_kernel_str: yes
  get_current_cgroup_id: yes
  send_signal: yes
  override_return: yes
  get_boot_ns: yes
  dpath: yes
  skboutput: yes
  get_tai_ns: yes
  get_func_ip: yes
  jiffies64: yes
  for_each_map_elem: yes

Kernel features
  Instruction limit: 1000000
  Loop support: yes
  btf: yes
  module btf: yes
  map batch: yes
  uprobe refcount (depends on Build:bcc bpf_attach_uprobe refcount): yes

Map types
  hash: yes
  percpu hash: yes
  array: yes
  percpu array: yes
  stack_trace: yes
  perf_event_array: yes
  ringbuf: yes

Probe types
  kprobe: yes
  tracepoint: yes
  perf_event: yes
  kfunc: yes
  kprobe_multi: yes
  uprobe_multi: yes
  raw_tp_special: yes
  iter: yes

danobi avatar Oct 07 '24 22:10 danobi

Deleting scalar maps has actually been allowed since the very beginning of bpftrace. Here's an old example of a script using it: https://github.com/brendangregg/bpf-perf-tools-book/blob/09f8a3a101fdc23b0e51c33158ea3a1782aea427/originals/Ch14_Kernel/numamove.bt#L36-L37

It was actually #3300 which unintentionally changed this functionality in the current release cycle. That said, I think the new behaviour is reasonable.

RE aggregate types: we'll hopefully have support for constructing them in BpfScript soon, which will fix that issue.

Another case we don't yet have a solution for is suppressing the printing of scalar map values at the end of a script. The previous solution of:

END
{
  delete(@myscalar);
}

won't work to stop @myscalar being printed any more. I think #3147 is the way to go for this.

ajor avatar Oct 08 '24 10:10 ajor

Tagging this with v0.22 for now as the inability to suppress scalar map printing is a breaking change.

ajor avatar Oct 08 '24 10:10 ajor

Partial workaround landed in https://github.com/bpftrace/bpftrace/pull/3547

We'll still want to fix this

danobi avatar Oct 30 '24 05:10 danobi

#3611 to revert hash optimization

danobi avatar Nov 26 '24 22:11 danobi