Message-ID: <20181214181649.GA32470@mini-arch.hsd1.ca.comcast.net>
Date:   Fri, 14 Dec 2018 10:16:49 -0800
From:   Stanislav Fomichev <sdf@...ichev.me>
To:     Quentin Monnet <quentin.monnet@...ronome.com>
Cc:     Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org,
        ast@...nel.org, davem@...emloft.net, daniel@...earbox.net,
        ecree@...arflare.com,
        OSS-drivers Netronome <oss-drivers@...ronome.com>
Subject: Re: [PATCH bpf-next 1/6] selftests/bpf: add map/prog type probe
 helpers

On 12/14, Quentin Monnet wrote:
> Hi Stanislav,
> 
> 2018-12-13 11:02 UTC-0800 ~ Stanislav Fomichev <sdf@...gle.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@...gle.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

Powered by blists - more mailing lists