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

Deleting scalar map just zeros out the entry #3498

Closed
danobi opened this issue Oct 7, 2024 · 4 comments · Fixed by #3611
Closed

Deleting scalar map just zeros out the entry #3498

danobi opened this issue Oct 7, 2024 · 4 comments · Fixed by #3611
Labels
bug Something isn't working
Milestone

Comments

@danobi
Copy link
Member

danobi commented Oct 7, 2024

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 39f3996 ("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 danobi added the bug Something isn't working label Oct 7, 2024
@ajor
Copy link
Member

ajor commented Oct 8, 2024

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 ajor added this to the v0.22 milestone Oct 8, 2024
@ajor
Copy link
Member

ajor commented Oct 8, 2024

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

@danobi
Copy link
Member Author

danobi commented Oct 30, 2024

Partial workaround landed in #3547

We'll still want to fix this

@danobi
Copy link
Member Author

danobi commented Nov 26, 2024

#3611 to revert hash optimization

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@danobi @ajor and others