Hi Stanislav,

2018-12-13 11:02 UTC-0800 ~ Stanislav Fomichev <s...@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 <s...@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

Reply via email to