| Message ID | 20181213190301.65816-2-sdf@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | skip verifier/map tests if kernel support is missing | expand |
Hi Stanislav, 2018-12-13 11:02 UTC-0800 ~ Stanislav Fomichev <sdf@google.com> > 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. > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- > 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.h > > diff --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. > + > + 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
On 12/14, Quentin Monnet wrote: > Hi Stanislav, > > 2018-12-13 11:02 UTC-0800 ~ Stanislav Fomichev <sdf@google.com> > > 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? I thought that it's safer for verifier to FAIL in case we forgot to add a specific map support to bpf_map_type_supported(). This is not the case if we were to use your version where you try to support every map type. > 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. If we were to move this to libbpf, yes. For tests, I think we include bpr_rlimit.h everywhere and things just work :-) > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > 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.h > > > > diff --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? Can we extend it later as we go? This is just a test helper with a limited support. If you want to start with putting this to libbpf, then yes, we need to add ifindex and properly support all map types. > > > +{ > > + 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? ditto, see above > > > +{ > > + 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? Again, for a verifier, I'd rather fail for a case where we didn't explicitly allow it to skip. > > > + 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. Ack, again, this just for a limited set of maps where we do fixups in verifier. > > > + > > + 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? If we were to move them to the libbpf, yes. So far, it's only a test helper. > > > + > > +#endif
2018-12-14 10:16 UTC-0800 ~ Stanislav Fomichev <sdf@fomichev.me> > On 12/14, Quentin Monnet wrote: >> Hi Stanislav, >> >> 2018-12-13 11:02 UTC-0800 ~ Stanislav Fomichev <sdf@google.com> >>> 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? > I thought that it's safer for verifier to FAIL in case we forgot to add > a specific map support to bpf_map_type_supported(). This is not the case > if we were to use your version where you try to support every map type. > >> 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. > If we were to move this to libbpf, yes. For tests, I think we include > bpr_rlimit.h everywhere and things just work :-) Hmm. I was so focused on bpftool and libbpf that somehow I read you patch as a proposal to include these probes directly into libbpf. Which, as you explain (and as I should have read), is not the case. So please accept my apologies, in this case your decisions (here and in the rest of the patch) make sense to me :).
On 12/14, Quentin Monnet wrote: > 2018-12-14 10:16 UTC-0800 ~ Stanislav Fomichev <sdf@fomichev.me> > > On 12/14, Quentin Monnet wrote: > >> Hi Stanislav, > >> > >> 2018-12-13 11:02 UTC-0800 ~ Stanislav Fomichev <sdf@google.com> > >>> 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? > > I thought that it's safer for verifier to FAIL in case we forgot to add > > a specific map support to bpf_map_type_supported(). This is not the case > > if we were to use your version where you try to support every map type. > > > >> 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. > > If we were to move this to libbpf, yes. For tests, I think we include > > bpr_rlimit.h everywhere and things just work :-) > > Hmm. I was so focused on bpftool and libbpf that somehow I read you > patch as a proposal to include these probes directly into libbpf. Which, > as you explain (and as I should have read), is not the case. So please > accept my apologies, in this case your decisions (here and in the rest > of the patch) make sense to me :). No worries, I was just scratching my own itch with these (wanted to have a simple non-controversial probers for the test cases, I can migrate to your libbpf helpers whenever they are available).
diff --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) +{ + 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) +{ + 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: + return true; + } + + 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); + +#endif
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'. Signed-off-by: Stanislav Fomichev <sdf@google.com> --- 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.h