Re: [PATCH bpf-next 1/6] selftests/bpf: add map/prog type probe helpers
From: Quentin Monnet <hidden>
Date: 2018-12-14 12:32:09
Hi Stanislav, 2018-12-13 11:02 UTC-0800 ~ Stanislav Fomichev [off-list ref]
Export bpf_map_type_supported() and bpf_prog_type_supported() which return true/false to indicate kernel support for the appropriate program or map type. These helpers will be used in the next commits to selectively skip test_verifier/test_maps tests. bpf_map_type_supported() supports only limited set of maps for which we do fixups in the test_verifier, for unknown maps it falls back to 'supported'.
Why would you fall back on “supported” if it does not know about them? Would that be worth having an enum as a return type (..._SUPPORTED, ..._UNSUPPORTED, ..._UNKNOWN) maybe? Or default to not supported? Not related - We would need to put a warning somewhere, maybe a comment in the header, that using probes repeatedly in a short amount of time needs to update resources limits (setrlimit()), otherwise probes won't work correctly.
quoted hunk ↗ jump to hunk
Signed-off-by: Stanislav Fomichev <redacted> --- tools/testing/selftests/bpf/probe_helpers.c | 68 +++++++++++++++++++++ tools/testing/selftests/bpf/probe_helpers.h | 10 +++ 2 files changed, 78 insertions(+) create mode 100644 tools/testing/selftests/bpf/probe_helpers.c create mode 100644 tools/testing/selftests/bpf/probe_helpers.hdiff --git a/tools/testing/selftests/bpf/probe_helpers.c b/tools/testing/selftests/bpf/probe_helpers.c new file mode 100644 index 000000000000..00467fdda813 --- /dev/null +++ b/tools/testing/selftests/bpf/probe_helpers.c@@ -0,0 +1,68 @@ +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +#include <unistd.h> +#include <bpf/bpf.h> + +#include "cgroup_helpers.h" +#include "bpf_util.h" +#include "../../../include/linux/filter.h" + +bool bpf_prog_type_supported(enum bpf_prog_type prog_type)
Can we please make it possible to add an ifindex for testing offload support?
+{
+ struct bpf_load_program_attr attr;
+ struct bpf_insn insns[] = {
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ };
+ int ret;
+
+ if (prog_type == BPF_PROG_TYPE_UNSPEC)
+ return true;
+
+ memset(&attr, 0, sizeof(attr));
+ attr.prog_type = prog_type;
+ attr.insns = insns;
+ attr.insns_cnt = ARRAY_SIZE(insns);
+ attr.license = "GPL";
+
+ ret = bpf_load_program_xattr(&attr, NULL, 0);
+ if (ret < 0)
+ return false;
+ close(ret);
+
+ return true;
+}
+
+bool bpf_map_type_supported(enum bpf_map_type map_type)Could we take an ifindex here as well?
+{
+ int key_size, value_size, max_entries;
+ int fd;
+
+ key_size = sizeof(__u32);
+ value_size = sizeof(__u32);
+ max_entries = 1;
+
+ /* limited set of maps for test_verifier.c and test_maps.c */
+ switch (map_type) {
+ case BPF_MAP_TYPE_SOCKMAP:
+ case BPF_MAP_TYPE_SOCKHASH:
+ case BPF_MAP_TYPE_XSKMAP:
+ break;
+ case BPF_MAP_TYPE_STACK_TRACE:
+ value_size = sizeof(__u64);
+ case BPF_MAP_TYPE_CGROUP_STORAGE:
+ case BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE:
+ key_size = sizeof(struct bpf_cgroup_storage_key);
+ value_size = sizeof(__u64);
+ max_entries = 0;
+ break;
+ default:Why not probing the other types of maps and blindly assume everything else is supported?
+ return true; + }
For the record if you were to probe all existing map types at this date you have would have issues here for LPM_TRIE (key_size, value_size, map_flags), QUEUE and STACK (key_size). Also, maps in maps.
quoted hunk ↗ jump to hunk
+ + fd = bpf_create_map(map_type, key_size, value_size, max_entries, 0); + if (fd < 0) + return false; + close(fd); + + return true; +}diff --git a/tools/testing/selftests/bpf/probe_helpers.h b/tools/testing/selftests/bpf/probe_helpers.h new file mode 100644 index 000000000000..9a107d6fe936 --- /dev/null +++ b/tools/testing/selftests/bpf/probe_helpers.h@@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ +#ifndef __PROBE_HELPERS_H +#define __PROBE_HELPERS_H + +#include <linux/bpf.h> + +bool bpf_prog_type_supported(enum bpf_prog_type prog_type); +bool bpf_map_type_supported(enum bpf_map_type map_type);
Should these get a visibility attribute with "LIBBPF_API" in front of the declarations?
+ +#endif