| Message ID | 20220512141710.116135-2-wanjiabing@vivo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: optimize the bpf_kprobe_multi_link_attach function | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on z15 + selftests |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | fail | Logs for Kernel LATEST on ubuntu-latest + selftests |
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
> On May 12, 2022, at 7:17 AM, Wan Jiabing <wanjiabing@vivo.com> wrote: > > Use 'error_addrs', 'error_cookies' and 'error_link' tags to make error > handling more efficient. > > Signed-off-by: Wan Jiabing <wanjiabing@vivo.com> > --- > kernel/trace/bpf_trace.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) I seriously think current version is simpler. Song
On Thu, May 12, 2022 at 10:17:08PM +0800, Wan Jiabing wrote: > Use 'error_addrs', 'error_cookies' and 'error_link' tags to make error > handling more efficient. Can you add a bit more context to this commit summary? The added goto labels aren't what make the code more performant, it's the avoidance of unnecessary free calls on NULL pointers that (supposedly) does. > > Signed-off-by: Wan Jiabing <wanjiabing@vivo.com> > --- > kernel/trace/bpf_trace.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 2eaac094caf8..3a8b69ef9a0d 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2467,20 +2467,20 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > if (uaddrs) { > if (copy_from_user(addrs, uaddrs, size)) { > err = -EFAULT; > - goto error; > + goto error_addrs; > } > } else { > struct user_syms us; > > err = copy_user_syms(&us, usyms, cnt); > if (err) > - goto error; > + goto error_addrs; > > sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL); > err = ftrace_lookup_symbols(us.syms, cnt, addrs); > free_user_syms(&us); > if (err) > - goto error; > + goto error_addrs; > } > > ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); > @@ -2488,18 +2488,18 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > cookies = kvmalloc(size, GFP_KERNEL); > if (!cookies) { > err = -ENOMEM; > - goto error; > + goto error_addrs; > } > if (copy_from_user(cookies, ucookies, size)) { > err = -EFAULT; > - goto error; > + goto error_cookies; > } > } > > link = kzalloc(sizeof(*link), GFP_KERNEL); > if (!link) { > err = -ENOMEM; > - goto error; > + goto error_cookies; > } > > bpf_link_init(&link->link, BPF_LINK_TYPE_KPROBE_MULTI, > @@ -2507,7 +2507,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > > err = bpf_link_prime(&link->link, &link_primer); > if (err) > - goto error; > + goto error_link; > > if (flags & BPF_F_KPROBE_MULTI_RETURN) > link->fp.exit_handler = kprobe_multi_link_handler; > @@ -2539,10 +2539,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > > return bpf_link_settle(&link_primer); > > -error: > +error_link: > kfree(link); > - kvfree(addrs); > +error_cookies: > kvfree(cookies); > +error_addrs: > + kvfree(addrs); > return err; > } > #else /* !CONFIG_FPROBE */ > -- > 2.35.1 > Could you clarify what performance gains you observed from doing this? I wouldn't have expected avoiding a couple of calls and NULL checks to have a measurable impact on performance, and I'm wondering whether the complexity from having multiple goto labels is really worth any supposed performance gains. Thanks, David
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 2eaac094caf8..3a8b69ef9a0d 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -2467,20 +2467,20 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr if (uaddrs) { if (copy_from_user(addrs, uaddrs, size)) { err = -EFAULT; - goto error; + goto error_addrs; } } else { struct user_syms us; err = copy_user_syms(&us, usyms, cnt); if (err) - goto error; + goto error_addrs; sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL); err = ftrace_lookup_symbols(us.syms, cnt, addrs); free_user_syms(&us); if (err) - goto error; + goto error_addrs; } ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); @@ -2488,18 +2488,18 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr cookies = kvmalloc(size, GFP_KERNEL); if (!cookies) { err = -ENOMEM; - goto error; + goto error_addrs; } if (copy_from_user(cookies, ucookies, size)) { err = -EFAULT; - goto error; + goto error_cookies; } } link = kzalloc(sizeof(*link), GFP_KERNEL); if (!link) { err = -ENOMEM; - goto error; + goto error_cookies; } bpf_link_init(&link->link, BPF_LINK_TYPE_KPROBE_MULTI, @@ -2507,7 +2507,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr err = bpf_link_prime(&link->link, &link_primer); if (err) - goto error; + goto error_link; if (flags & BPF_F_KPROBE_MULTI_RETURN) link->fp.exit_handler = kprobe_multi_link_handler; @@ -2539,10 +2539,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr return bpf_link_settle(&link_primer); -error: +error_link: kfree(link); - kvfree(addrs); +error_cookies: kvfree(cookies); +error_addrs: + kvfree(addrs); return err; } #else /* !CONFIG_FPROBE */
Use 'error_addrs', 'error_cookies' and 'error_link' tags to make error handling more efficient. Signed-off-by: Wan Jiabing <wanjiabing@vivo.com> --- kernel/trace/bpf_trace.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)