diff mbox series

[bpf-next,v1,6/8] selftests/bpf: test autocreate behavior for struct_ops maps

Message ID 20240227204556.17524-7-eddyz87@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series libbpf: type suffixes and autocreate flag for struct_ops maps | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 9 maintainers not CCed: jolsa@kernel.org mykolal@fb.com john.fastabend@gmail.com shuah@kernel.org song@kernel.org sdf@google.com linux-kselftest@vger.kernel.org kpsingh@kernel.org haoluo@google.com
netdev/build_clang success Errors and warnings before: 1066 this patch: 1066
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 81 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18

Commit Message

Eduard Zingerman Feb. 27, 2024, 8:45 p.m. UTC 
Check that bpf_map__set_autocreate() can be used to disable automatic
creation for struct_ops maps.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../bpf/prog_tests/struct_ops_autocreate.c    | 79 +++++++++++++++++++
 .../bpf/progs/struct_ops_autocreate.c         | 42 ++++++++++
 2 files changed, 121 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate.c

Comments

David Vernet Feb. 28, 2024, 6:29 p.m. UTC | #1 
On Tue, Feb 27, 2024 at 10:45:54PM +0200, Eduard Zingerman wrote:
> Check that bpf_map__set_autocreate() can be used to disable automatic
> creation for struct_ops maps.
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../bpf/prog_tests/struct_ops_autocreate.c    | 79 +++++++++++++++++++
>  .../bpf/progs/struct_ops_autocreate.c         | 42 ++++++++++
>  2 files changed, 121 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
>  create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> new file mode 100644
> index 000000000000..b21b10f94fc2
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "struct_ops_autocreate.skel.h"
> +
> +#define EXPECTED_MSG "libbpf: struct_ops init_kern"
> +
> +static libbpf_print_fn_t old_print_cb;
> +static bool msg_found;
> +
> +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> +{
> +	old_print_cb(level, fmt, args);
> +	if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> +		msg_found = true;
> +
> +	return 0;
> +}
> +
> +static void cant_load_full_object(void)
> +{
> +	struct struct_ops_autocreate *skel;
> +	int err;
> +
> +	old_print_cb = libbpf_set_print(print_cb);
> +	skel = struct_ops_autocreate__open_and_load();

Optional suggestion: It might be useful to add a comment here explaining
exactly why we expect this to fail? Something like:

	/* The testmod_2 map BTF type (struct bpf_testmod_ops___v2) doesn't
	 * match the BTF of the actual struct bpf_testmod_ops defined in the
	 * kernel, so we should fail to load it if we don't disable autocreate
	 * for the map.
	 */

Feel free to ignore -- I recognize that some might just consider that
unnecessary noise.

> +	err = errno;
> +	libbpf_set_print(old_print_cb);
> +	if (!ASSERT_NULL(skel, "struct_ops_autocreate__open_and_load"))
> +		return;
> +
> +	ASSERT_EQ(err, ENOTSUP, "errno should be ENOTSUP");
> +	ASSERT_TRUE(msg_found, "expected message");
> +
> +	struct_ops_autocreate__destroy(skel);
> +}
> +
> +static void can_load_partial_object(void)
> +{
> +	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> +	struct struct_ops_autocreate *skel;
> +	struct bpf_link *link = NULL;
> +	int err;
> +
> +	skel = struct_ops_autocreate__open_opts(&opts);
> +	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
> +		return;
> +
> +	err = bpf_program__set_autoload(skel->progs.test_2, false);
> +	if (!ASSERT_OK(err, "bpf_program__set_autoload"))
> +		goto cleanup;

It feels a bit awkward to have to specify that a struct_ops prog isn't
autoloaded if it's not associated with an autoloaded / autocreated struct_ops
map. Would it be possible to teach libbpf to not autoload such progs by
default?

> +	err = bpf_map__set_autocreate(skel->maps.testmod_2, false);
> +	if (!ASSERT_OK(err, "bpf_map__set_autocreate"))
> +		goto cleanup;
> +
> +	err = struct_ops_autocreate__load(skel);
> +	if (ASSERT_OK(err, "struct_ops_autocreate__load"))
> +		goto cleanup;
> +
> +	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
> +	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
> +		goto cleanup;
> +
> +	/* test_1() would be called from bpf_dummy_reg2() in bpf_testmod.c */
> +	ASSERT_EQ(skel->bss->test_1_result, 42, "test_1_result");
> +
> +cleanup:
> +	bpf_link__destroy(link);
> +	struct_ops_autocreate__destroy(skel);
> +}
> +
> +void serial_test_struct_ops_autocreate(void)
> +{
> +	if (test__start_subtest("cant_load_full_object"))
> +		cant_load_full_object();
> +	if (test__start_subtest("can_load_partial_object"))
> +		can_load_partial_object();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> new file mode 100644
> index 000000000000..294d48bb8e3c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +int test_1_result = 0;
> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_1)
> +{
> +	test_1_result = 42;
> +	return 0;
> +}
> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_2)
> +{
> +	return 0;
> +}
> +
> +struct bpf_testmod_ops___v1 {
> +	int (*test_1)(void);
> +};
> +
> +struct bpf_testmod_ops___v2 {
> +	int (*test_1)(void);
> +	int (*does_not_exist)(void);
> +};
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops___v1 testmod_1 = {
> +	.test_1 = (void *)test_1
> +};
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops___v2 testmod_2 = {
> +	.test_1 = (void *)test_1,
> +	.does_not_exist = (void *)test_2
> +};
> -- 
> 2.43.0
>
David Vernet Feb. 28, 2024, 6:34 p.m. UTC | #2 
On Wed, Feb 28, 2024 at 12:29:49PM -0600, David Vernet wrote:
> On Tue, Feb 27, 2024 at 10:45:54PM +0200, Eduard Zingerman wrote:
> > Check that bpf_map__set_autocreate() can be used to disable automatic
> > creation for struct_ops maps.
> > 
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> >  .../bpf/prog_tests/struct_ops_autocreate.c    | 79 +++++++++++++++++++
> >  .../bpf/progs/struct_ops_autocreate.c         | 42 ++++++++++
> >  2 files changed, 121 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> > new file mode 100644
> > index 000000000000..b21b10f94fc2
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> > @@ -0,0 +1,79 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <test_progs.h>
> > +#include "struct_ops_autocreate.skel.h"
> > +
> > +#define EXPECTED_MSG "libbpf: struct_ops init_kern"
> > +
> > +static libbpf_print_fn_t old_print_cb;
> > +static bool msg_found;
> > +
> > +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> > +{
> > +	old_print_cb(level, fmt, args);
> > +	if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> > +		msg_found = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static void cant_load_full_object(void)
> > +{
> > +	struct struct_ops_autocreate *skel;
> > +	int err;
> > +
> > +	old_print_cb = libbpf_set_print(print_cb);
> > +	skel = struct_ops_autocreate__open_and_load();
> 
> Optional suggestion: It might be useful to add a comment here explaining
> exactly why we expect this to fail? Something like:
> 
> 	/* The testmod_2 map BTF type (struct bpf_testmod_ops___v2) doesn't
> 	 * match the BTF of the actual struct bpf_testmod_ops defined in the
> 	 * kernel, so we should fail to load it if we don't disable autocreate
> 	 * for the map.
> 	 */
> 
> Feel free to ignore -- I recognize that some might just consider that
> unnecessary noise.
> 
> > +	err = errno;
> > +	libbpf_set_print(old_print_cb);
> > +	if (!ASSERT_NULL(skel, "struct_ops_autocreate__open_and_load"))
> > +		return;
> > +
> > +	ASSERT_EQ(err, ENOTSUP, "errno should be ENOTSUP");
> > +	ASSERT_TRUE(msg_found, "expected message");
> > +
> > +	struct_ops_autocreate__destroy(skel);
> > +}
> > +
> > +static void can_load_partial_object(void)
> > +{
> > +	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> > +	struct struct_ops_autocreate *skel;
> > +	struct bpf_link *link = NULL;
> > +	int err;
> > +
> > +	skel = struct_ops_autocreate__open_opts(&opts);
> > +	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
> > +		return;
> > +
> > +	err = bpf_program__set_autoload(skel->progs.test_2, false);
> > +	if (!ASSERT_OK(err, "bpf_program__set_autoload"))
> > +		goto cleanup;
> 
> It feels a bit awkward to have to specify that a struct_ops prog isn't
> autoloaded if it's not associated with an autoloaded / autocreated struct_ops
> map. Would it be possible to teach libbpf to not autoload such progs by
> default?

I see you already added that in the next patch. Nice!!
Eduard Zingerman Feb. 28, 2024, 7:31 p.m. UTC | #3 
On Wed, 2024-02-28 at 12:29 -0600, David Vernet wrote:

[...]

> > +static void cant_load_full_object(void)
> > +{
> > +	struct struct_ops_autocreate *skel;
> > +	int err;
> > +
> > +	old_print_cb = libbpf_set_print(print_cb);
> > +	skel = struct_ops_autocreate__open_and_load();
> 
> Optional suggestion: It might be useful to add a comment here explaining
> exactly why we expect this to fail? Something like:
> 
> 	/* The testmod_2 map BTF type (struct bpf_testmod_ops___v2) doesn't
> 	 * match the BTF of the actual struct bpf_testmod_ops defined in the
> 	 * kernel, so we should fail to load it if we don't disable autocreate
> 	 * for the map.
> 	 */
> 
> Feel free to ignore -- I recognize that some might just consider that
> unnecessary noise.

I will add this comment, agree that it helps when tests explain what happens.

[...]
Andrii Nakryiko Feb. 28, 2024, 11:43 p.m. UTC | #4 
On Tue, Feb 27, 2024 at 12:46 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Check that bpf_map__set_autocreate() can be used to disable automatic
> creation for struct_ops maps.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../bpf/prog_tests/struct_ops_autocreate.c    | 79 +++++++++++++++++++
>  .../bpf/progs/struct_ops_autocreate.c         | 42 ++++++++++
>  2 files changed, 121 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
>  create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> new file mode 100644
> index 000000000000..b21b10f94fc2
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "struct_ops_autocreate.skel.h"
> +
> +#define EXPECTED_MSG "libbpf: struct_ops init_kern"
> +
> +static libbpf_print_fn_t old_print_cb;
> +static bool msg_found;
> +
> +static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
> +{
> +       old_print_cb(level, fmt, args);
> +       if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
> +               msg_found = true;
> +
> +       return 0;
> +}
> +
> +static void cant_load_full_object(void)
> +{
> +       struct struct_ops_autocreate *skel;
> +       int err;
> +
> +       old_print_cb = libbpf_set_print(print_cb);
> +       skel = struct_ops_autocreate__open_and_load();
> +       err = errno;
> +       libbpf_set_print(old_print_cb);
> +       if (!ASSERT_NULL(skel, "struct_ops_autocreate__open_and_load"))
> +               return;
> +
> +       ASSERT_EQ(err, ENOTSUP, "errno should be ENOTSUP");
> +       ASSERT_TRUE(msg_found, "expected message");
> +
> +       struct_ops_autocreate__destroy(skel);
> +}
> +
> +static void can_load_partial_object(void)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);

nit: prefer LIBBPF_OPTS() over DECLARE_LIBBPF_OPTS()

> +       struct struct_ops_autocreate *skel;
> +       struct bpf_link *link = NULL;
> +       int err;
> +
> +       skel = struct_ops_autocreate__open_opts(&opts);
> +       if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
> +               return;
> +
> +       err = bpf_program__set_autoload(skel->progs.test_2, false);
> +       if (!ASSERT_OK(err, "bpf_program__set_autoload"))
> +               goto cleanup;
> +
> +       err = bpf_map__set_autocreate(skel->maps.testmod_2, false);
> +       if (!ASSERT_OK(err, "bpf_map__set_autocreate"))
> +               goto cleanup;
> +
> +       err = struct_ops_autocreate__load(skel);
> +       if (ASSERT_OK(err, "struct_ops_autocreate__load"))
> +               goto cleanup;
> +
> +       link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
> +       if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
> +               goto cleanup;
> +
> +       /* test_1() would be called from bpf_dummy_reg2() in bpf_testmod.c */
> +       ASSERT_EQ(skel->bss->test_1_result, 42, "test_1_result");
> +
> +cleanup:
> +       bpf_link__destroy(link);
> +       struct_ops_autocreate__destroy(skel);
> +}
> +
> +void serial_test_struct_ops_autocreate(void)

same as in the previous patch, why serial?

> +{
> +       if (test__start_subtest("cant_load_full_object"))
> +               cant_load_full_object();
> +       if (test__start_subtest("can_load_partial_object"))
> +               can_load_partial_object();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> new file mode 100644
> index 000000000000..294d48bb8e3c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +int test_1_result = 0;
> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_1)
> +{
> +       test_1_result = 42;
> +       return 0;
> +}
> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_2)
> +{
> +       return 0;
> +}
> +
> +struct bpf_testmod_ops___v1 {
> +       int (*test_1)(void);
> +};
> +
> +struct bpf_testmod_ops___v2 {
> +       int (*test_1)(void);
> +       int (*does_not_exist)(void);
> +};
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops___v1 testmod_1 = {
> +       .test_1 = (void *)test_1
> +};
> +
> +SEC(".struct_ops.link")

can you please also have a test where we use SEC("?.struct_ops.link")
which set autoload to false by default?

> +struct bpf_testmod_ops___v2 testmod_2 = {
> +       .test_1 = (void *)test_1,
> +       .does_not_exist = (void *)test_2
> +};
> --
> 2.43.0
>
Eduard Zingerman Feb. 28, 2024, 11:55 p.m. UTC | #5 
On Wed, 2024-02-28 at 15:43 -0800, Andrii Nakryiko wrote:
[...]

> > +static void can_load_partial_object(void)
> > +{
> > +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> 
> nit: prefer LIBBPF_OPTS() over DECLARE_LIBBPF_OPTS()

Ok

[...]

> > +void serial_test_struct_ops_autocreate(void)
> 
> same as in the previous patch, why serial?

Because of the print callback hijacking.
Also, what would happen when two tests would set struct_ops for same
attachment point?

[...]

> > +SEC(".struct_ops.link")
> 
> can you please also have a test where we use SEC("?.struct_ops.link")
> which set autoload to false by default?

As far as I understand, that would be a new behavior, currently '?'
works only for programs. I'll add a separate patch to support this.

[...]
Andrii Nakryiko Feb. 29, 2024, 12:02 a.m. UTC | #6 
On Wed, Feb 28, 2024 at 3:55 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-02-28 at 15:43 -0800, Andrii Nakryiko wrote:
> [...]
>
> > > +static void can_load_partial_object(void)
> > > +{
> > > +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> >
> > nit: prefer LIBBPF_OPTS() over DECLARE_LIBBPF_OPTS()
>
> Ok
>
> [...]
>
> > > +void serial_test_struct_ops_autocreate(void)
> >
> > same as in the previous patch, why serial?
>
> Because of the print callback hijacking.
> Also, what would happen when two tests would set struct_ops for same
> attachment point?

I'm not sure, Martin?

But if this is a problem, then perhaps it's best to combine all
struct_ops tests that use bpf_testmod into a single test as multiple
subtests. And this use non-serial overall test approach.

>
> [...]
>
> > > +SEC(".struct_ops.link")
> >
> > can you please also have a test where we use SEC("?.struct_ops.link")
> > which set autoload to false by default?
>
> As far as I understand, that would be a new behavior, currently '?'
> works only for programs. I'll add a separate patch to support this.
>

Yep, thanks!

> [...]
Martin KaFai Lau Feb. 29, 2024, 12:56 a.m. UTC | #7 
On 2/28/24 4:02 PM, Andrii Nakryiko wrote:
>> Also, what would happen when two tests would set struct_ops for same
>> attachment point?
> I'm not sure, Martin?

The kernel bpf struct_ops infra can handle it. For example, different userspace 
processes may register bpf-tcp-cc in parallel also. It is a matter of the 
subsystem (tcp in the bpf-tcp-cc case) can handle it or not.

In the testmod case, it will be bpf_dummy_reg[2] and nothing in there is 
stopping parallel run afaict.
Eduard Zingerman March 1, 2024, 1:28 a.m. UTC | #8 
On Wed, 2024-02-28 at 16:02 -0800, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 3:55 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
[...]
> > On Wed, 2024-02-28 at 15:43 -0800, Andrii Nakryiko wrote:
> > > > +SEC(".struct_ops.link")
> > > 
> > > can you please also have a test where we use SEC("?.struct_ops.link")
> > > which set autoload to false by default?
> > 
> > As far as I understand, that would be a new behavior, currently '?'
> > works only for programs. I'll add a separate patch to support this.
> > 
> 
> Yep, thanks!

So, I have a draft for v2 with support for this feature in [0].
But there is a gotcha:

    libbpf: BTF loading error: -22
    libbpf: -- BEGIN BTF LOAD LOG ---
    ...
    [23] DATASEC ?.struct_ops size=8 vlen=1 Invalid name
    
    -- END BTF LOAD LOG --
    libbpf: Error loading .BTF into kernel: -22. BTF is mandatory, can't proceed.

Kernel rejects DATASEC name with '?'.
The options are:
1. Tweak kernel to allow '?' as a first char in DATASEC names;
2. Use some different name, e.g. ".struct_ops.opt";
3. Do some kind of BTF rewrite in libbpf to merge
   "?.struct_ops" and ".struct_ops" DATASECs as a single DATASEC.
  
(1) is simple, but downside is requirement for kernel upgrade;
(2) is simple, but goes against current convention for program section names;
(3) idk, will check if that is feasible tomorrow.

[0] https://github.com/eddyz87/bpf/tree/structops-multi-version
Andrii Nakryiko March 1, 2024, 6:03 p.m. UTC | #9 
On Thu, Feb 29, 2024 at 5:28 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-02-28 at 16:02 -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 28, 2024 at 3:55 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> [...]
> > > On Wed, 2024-02-28 at 15:43 -0800, Andrii Nakryiko wrote:
> > > > > +SEC(".struct_ops.link")
> > > >
> > > > can you please also have a test where we use SEC("?.struct_ops.link")
> > > > which set autoload to false by default?
> > >
> > > As far as I understand, that would be a new behavior, currently '?'
> > > works only for programs. I'll add a separate patch to support this.
> > >
> >
> > Yep, thanks!
>
> So, I have a draft for v2 with support for this feature in [0].
> But there is a gotcha:
>
>     libbpf: BTF loading error: -22
>     libbpf: -- BEGIN BTF LOAD LOG ---
>     ...
>     [23] DATASEC ?.struct_ops size=8 vlen=1 Invalid name
>
>     -- END BTF LOAD LOG --
>     libbpf: Error loading .BTF into kernel: -22. BTF is mandatory, can't proceed.
>
> Kernel rejects DATASEC name with '?'.
> The options are:
> 1. Tweak kernel to allow '?' as a first char in DATASEC names;
> 2. Use some different name, e.g. ".struct_ops.opt";
> 3. Do some kind of BTF rewrite in libbpf to merge
>    "?.struct_ops" and ".struct_ops" DATASECs as a single DATASEC.
>
> (1) is simple, but downside is requirement for kernel upgrade;
> (2) is simple, but goes against current convention for program section names;
> (3) idk, will check if that is feasible tomorrow.
>
> [0] https://github.com/eddyz87/bpf/tree/structops-multi-version
>
>

Let's do 1) for sure (ELF allows pretty much any name for DATASEC).
And then detect this in libbpf, and if the kernel is old and doesn't
yet support it, change DATASEC name from "?.struct_ops" to just
".struct_ops". Merging DATASECs seems like an unnecessary
complication.

>
Eduard Zingerman March 1, 2024, 6:07 p.m. UTC | #10 
On Fri, 2024-03-01 at 10:03 -0800, Andrii Nakryiko wrote:
[...]

> > Kernel rejects DATASEC name with '?'.
> > The options are:
> > 1. Tweak kernel to allow '?' as a first char in DATASEC names;
> > 2. Use some different name, e.g. ".struct_ops.opt";
> > 3. Do some kind of BTF rewrite in libbpf to merge
> >    "?.struct_ops" and ".struct_ops" DATASECs as a single DATASEC.
> > 
> > (1) is simple, but downside is requirement for kernel upgrade;
> > (2) is simple, but goes against current convention for program section names;
> > (3) idk, will check if that is feasible tomorrow.
> > 
> > [0] https://github.com/eddyz87/bpf/tree/structops-multi-version
> > 
> > 
> 
> Let's do 1) for sure (ELF allows pretty much any name for DATASEC).
> And then detect this in libbpf, and if the kernel is old and doesn't
> yet support it, change DATASEC name from "?.struct_ops" to just
> ".struct_ops". Merging DATASECs seems like an unnecessary
> complication.

Yes, makes sense, half way there already, thank you.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
new file mode 100644
index 000000000000..b21b10f94fc2
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_autocreate.c
@@ -0,0 +1,79 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "struct_ops_autocreate.skel.h"
+
+#define EXPECTED_MSG "libbpf: struct_ops init_kern"
+
+static libbpf_print_fn_t old_print_cb;
+static bool msg_found;
+
+static int print_cb(enum libbpf_print_level level, const char *fmt, va_list args)
+{
+	old_print_cb(level, fmt, args);
+	if (level == LIBBPF_WARN && strncmp(fmt, EXPECTED_MSG, strlen(EXPECTED_MSG)) == 0)
+		msg_found = true;
+
+	return 0;
+}
+
+static void cant_load_full_object(void)
+{
+	struct struct_ops_autocreate *skel;
+	int err;
+
+	old_print_cb = libbpf_set_print(print_cb);
+	skel = struct_ops_autocreate__open_and_load();
+	err = errno;
+	libbpf_set_print(old_print_cb);
+	if (!ASSERT_NULL(skel, "struct_ops_autocreate__open_and_load"))
+		return;
+
+	ASSERT_EQ(err, ENOTSUP, "errno should be ENOTSUP");
+	ASSERT_TRUE(msg_found, "expected message");
+
+	struct_ops_autocreate__destroy(skel);
+}
+
+static void can_load_partial_object(void)
+{
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+	struct struct_ops_autocreate *skel;
+	struct bpf_link *link = NULL;
+	int err;
+
+	skel = struct_ops_autocreate__open_opts(&opts);
+	if (!ASSERT_OK_PTR(skel, "struct_ops_autocreate__open_opts"))
+		return;
+
+	err = bpf_program__set_autoload(skel->progs.test_2, false);
+	if (!ASSERT_OK(err, "bpf_program__set_autoload"))
+		goto cleanup;
+
+	err = bpf_map__set_autocreate(skel->maps.testmod_2, false);
+	if (!ASSERT_OK(err, "bpf_map__set_autocreate"))
+		goto cleanup;
+
+	err = struct_ops_autocreate__load(skel);
+	if (ASSERT_OK(err, "struct_ops_autocreate__load"))
+		goto cleanup;
+
+	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
+		goto cleanup;
+
+	/* test_1() would be called from bpf_dummy_reg2() in bpf_testmod.c */
+	ASSERT_EQ(skel->bss->test_1_result, 42, "test_1_result");
+
+cleanup:
+	bpf_link__destroy(link);
+	struct_ops_autocreate__destroy(skel);
+}
+
+void serial_test_struct_ops_autocreate(void)
+{
+	if (test__start_subtest("cant_load_full_object"))
+		cant_load_full_object();
+	if (test__start_subtest("can_load_partial_object"))
+		can_load_partial_object();
+}
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
new file mode 100644
index 000000000000..294d48bb8e3c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_autocreate.c
@@ -0,0 +1,42 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+int test_1_result = 0;
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_1)
+{
+	test_1_result = 42;
+	return 0;
+}
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_2)
+{
+	return 0;
+}
+
+struct bpf_testmod_ops___v1 {
+	int (*test_1)(void);
+};
+
+struct bpf_testmod_ops___v2 {
+	int (*test_1)(void);
+	int (*does_not_exist)(void);
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops___v1 testmod_1 = {
+	.test_1 = (void *)test_1
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops___v2 testmod_2 = {
+	.test_1 = (void *)test_1,
+	.does_not_exist = (void *)test_2
+};