mbox series

[bpf-next,v2,0/4] migrate from bpf_prog_test_run{,_xattr}

Message ID 20220128012319.2494472-1-delyank@fb.com (mailing list archive)
Headers show
Series migrate from bpf_prog_test_run{,_xattr} | expand

Message

Delyan Kratunov Jan. 28, 2022, 1:23 a.m. UTC 
Fairly straight-forward mechanical transformation from bpf_prog_test_run
and bpf_prog_test_run_xattr to the bpf_prog_test_run_opts goodness.
Most of the changes are in tests, though bpftool and libbpf (xsk.c) have one
call site each as well.

The only aspect that's still a bit RFC is that prog_run_xattr is testing
behavior specific to bpf_prog_test_run_xattr, which does not exist in prog_run_opts.
Namely, -EINVAL return on data_out == NULL && data_size_out > 0.
Adding this behavior to prog_test_run_opts is one option, keeping the test as-is
and cloning it to use bpf_prog_test_run_opts is another possibility.
The current version just suppresses the deprecation warning.

As an aside, checkpatch really doesn't like that LIBBPF_OPTS looks like
a function call but is formatted like a struct declaration. If anyone
cares about formatting, now would be a good time to mention it.

v1 -> v2:
Split selftests/bpf changes into two commits to appease the mailing list.

Delyan Kratunov (4):
  selftests: bpf: migrate from bpf_prog_test_run
  selftests: bpf: migrate from bpf_prog_test_run_xattr
  bpftool: migrate from bpf_prog_test_run_xattr
  libbpf: Deprecate bpf_prog_test_run_xattr and bpf_prog_test_run

 tools/bpf/bpftool/prog.c                      |  55 ++--
 tools/lib/bpf/bpf.h                           |   8 +-
 tools/lib/bpf/xsk.c                           |  11 +-
 .../selftests/bpf/prog_tests/atomics.c        |  86 +++---
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  10 +-
 .../selftests/bpf/prog_tests/check_mtu.c      |  47 ++--
 .../selftests/bpf/prog_tests/cls_redirect.c   |  30 +--
 .../selftests/bpf/prog_tests/dummy_st_ops.c   |  31 +--
 .../selftests/bpf/prog_tests/fentry_fexit.c   |  33 ++-
 .../selftests/bpf/prog_tests/fentry_test.c    |   9 +-
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  |  33 ++-
 .../selftests/bpf/prog_tests/fexit_stress.c   |  26 +-
 .../selftests/bpf/prog_tests/fexit_test.c     |   9 +-
 .../selftests/bpf/prog_tests/flow_dissector.c |  75 +++---
 .../prog_tests/flow_dissector_load_bytes.c    |  27 +-
 .../selftests/bpf/prog_tests/for_each.c       |  32 ++-
 .../bpf/prog_tests/get_func_args_test.c       |  14 +-
 .../bpf/prog_tests/get_func_ip_test.c         |  12 +-
 .../selftests/bpf/prog_tests/global_data.c    |  25 +-
 .../bpf/prog_tests/global_func_args.c         |  13 +-
 .../selftests/bpf/prog_tests/kfree_skb.c      |  16 +-
 .../selftests/bpf/prog_tests/kfunc_call.c     |  46 ++--
 .../selftests/bpf/prog_tests/ksyms_module.c   |  23 +-
 .../selftests/bpf/prog_tests/l4lb_all.c       |  35 ++-
 .../selftests/bpf/prog_tests/map_lock.c       |  15 +-
 .../selftests/bpf/prog_tests/map_ptr.c        |  18 +-
 .../selftests/bpf/prog_tests/modify_return.c  |  38 +--
 .../selftests/bpf/prog_tests/pkt_access.c     |  27 +-
 .../selftests/bpf/prog_tests/pkt_md_access.c  |  15 +-
 .../selftests/bpf/prog_tests/prog_run_xattr.c |   5 +
 .../bpf/prog_tests/queue_stack_map.c          |  43 +--
 .../bpf/prog_tests/raw_tp_test_run.c          |  85 +++---
 .../bpf/prog_tests/raw_tp_writable_test_run.c |  16 +-
 .../selftests/bpf/prog_tests/signal_pending.c |  24 +-
 .../selftests/bpf/prog_tests/skb_ctx.c        |  93 +++----
 .../selftests/bpf/prog_tests/skb_helpers.c    |  16 +-
 .../selftests/bpf/prog_tests/sockmap_basic.c  |  19 +-
 .../selftests/bpf/prog_tests/spinlock.c       |  15 +-
 .../selftests/bpf/prog_tests/syscall.c        |  12 +-
 .../selftests/bpf/prog_tests/tailcalls.c      | 245 ++++++++++--------
 .../selftests/bpf/prog_tests/test_profiler.c  |  16 +-
 .../bpf/prog_tests/test_skb_pkt_end.c         |  15 +-
 .../testing/selftests/bpf/prog_tests/timer.c  |   9 +-
 .../selftests/bpf/prog_tests/timer_mim.c      |   9 +-
 .../selftests/bpf/prog_tests/trace_ext.c      |  28 +-
 tools/testing/selftests/bpf/prog_tests/xdp.c  |  35 ++-
 .../bpf/prog_tests/xdp_adjust_frags.c         |  34 ++-
 .../bpf/prog_tests/xdp_adjust_tail.c          | 148 ++++++-----
 .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    |  16 +-
 .../selftests/bpf/prog_tests/xdp_noinline.c   |  45 ++--
 .../selftests/bpf/prog_tests/xdp_perf.c       |  19 +-
 tools/testing/selftests/bpf/test_lru_map.c    |  11 +-
 tools/testing/selftests/bpf/test_progs.h      |   2 +
 tools/testing/selftests/bpf/test_verifier.c   |  16 +-
 54 files changed, 993 insertions(+), 802 deletions(-)

--
2.30.2

Comments

Daniel Borkmann Jan. 28, 2022, 1:07 p.m. UTC | #1 
On 1/28/22 2:23 AM, Delyan Kratunov wrote:
> Fairly straight-forward mechanical transformation from bpf_prog_test_run
> and bpf_prog_test_run_xattr to the bpf_prog_test_run_opts goodness.
> Most of the changes are in tests, though bpftool and libbpf (xsk.c) have one
> call site each as well.
> 
> The only aspect that's still a bit RFC is that prog_run_xattr is testing
> behavior specific to bpf_prog_test_run_xattr, which does not exist in prog_run_opts.
> Namely, -EINVAL return on data_out == NULL && data_size_out > 0.
> Adding this behavior to prog_test_run_opts is one option, keeping the test as-is
> and cloning it to use bpf_prog_test_run_opts is another possibility.

I would suggest to do the former rather than duplicating, if there's nothing
particularly blocking us from adding this to prog_test_run_opts.

> The current version just suppresses the deprecation warning.
> 
> As an aside, checkpatch really doesn't like that LIBBPF_OPTS looks like
> a function call but is formatted like a struct declaration. If anyone
> cares about formatting, now would be a good time to mention it.

As you have it looks good to me. One small nit, please also add a non-empty
commit message with rationale to each of the patches rather than just SoB alone.

Thanks a lot!

> v1 -> v2:
> Split selftests/bpf changes into two commits to appease the mailing list.
> 
> Delyan Kratunov (4):
>    selftests: bpf: migrate from bpf_prog_test_run
>    selftests: bpf: migrate from bpf_prog_test_run_xattr
>    bpftool: migrate from bpf_prog_test_run_xattr
>    libbpf: Deprecate bpf_prog_test_run_xattr and bpf_prog_test_run
>
Delyan Kratunov Jan. 28, 2022, 8:12 p.m. UTC | #2 
Thanks for taking a look, Daniel!

On Fri, 2022-01-28 at 14:07 +0100, Daniel Borkmann wrote:
> On 1/28/22 2:23 AM, Delyan Kratunov wrote:
> > Adding this behavior to prog_test_run_opts is one option, keeping the test as-is
> > and cloning it to use bpf_prog_test_run_opts is another possibility.
> 
> I would suggest to do the former rather than duplicating, if there's nothing
> particularly blocking us from adding this to prog_test_run_opts.

I looked into this a bit more. Unfortunately, bpf_test_finish unconditionally
copies data_size_out back into the uattr, even if data_out is NULL.
(net/bpf/test_run.c:180)

This makes the ergonomics of reusing the same topts struct for multiple
bpf_prog_test_run calls pretty horrendous - you'd need to clear data_size_out
before every call, even if you don't care about it otherwise (and you don't, you
didn't set data_out!).

In practicality, adding the logic from _xattr to _opts results in a significant
number of test failures. I'm a bit worried it might break libbpf users if they
use similar opts reuse patterns.

> As you have it looks good to me. One small nit, please also add a non-empty
> commit message with rationale to each of the patches rather than just SoB alone.

Will do!
Andrii Nakryiko Feb. 1, 2022, 1:04 a.m. UTC | #3 
On Fri, Jan 28, 2022 at 12:12 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> Thanks for taking a look, Daniel!
>
> On Fri, 2022-01-28 at 14:07 +0100, Daniel Borkmann wrote:
> > On 1/28/22 2:23 AM, Delyan Kratunov wrote:
> > > Adding this behavior to prog_test_run_opts is one option, keeping the test as-is
> > > and cloning it to use bpf_prog_test_run_opts is another possibility.
> >
> > I would suggest to do the former rather than duplicating, if there's nothing
> > particularly blocking us from adding this to prog_test_run_opts.
>
> I looked into this a bit more. Unfortunately, bpf_test_finish unconditionally
> copies data_size_out back into the uattr, even if data_out is NULL.
> (net/bpf/test_run.c:180)
>
> This makes the ergonomics of reusing the same topts struct for multiple
> bpf_prog_test_run calls pretty horrendous - you'd need to clear data_size_out
> before every call, even if you don't care about it otherwise (and you don't, you
> didn't set data_out!).
>
> In practicality, adding the logic from _xattr to _opts results in a significant
> number of test failures. I'm a bit worried it might break libbpf users if they
> use similar opts reuse patterns.
>

Seems like kernel doesn't enforce that data_size_out == 0 if data_out
is NULL, so I'd say let's just keep bpf_test_run_opts() as is and
defer to kernel for error checking (e.g., for raw_tp data_out isn't
allowed to be NULL, but libbpf doesn't check that, right, it just lets
kernel do all the nuanced error checking).

So I'd say let's keep _opts() as is and let's remove parts of
prog_run_xattr.c selftest that check this specific error check?

> > As you have it looks good to me. One small nit, please also add a non-empty
> > commit message with rationale to each of the patches rather than just SoB alone.
>
> Will do!
>