Messages in this thread | | | Date | Thu, 30 Nov 2017 10:28:08 -0800 | From | Martin KaFai Lau <> | Subject | Re: [PATCH/RFC] Re: 'perf test BPF' failing, libbpf regression wrt "basic API for BPF obj name" |
| |
On Thu, Nov 30, 2017 at 01:53:58PM -0300, Arnaldo Carvalho de Melo wrote: > Em Thu, Nov 30, 2017 at 12:01:10AM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Wed, Nov 29, 2017 at 02:31:36PM -0800, Martin KaFai Lau escreveu: > > > On Wed, Nov 29, 2017 at 06:15:43PM -0300, Arnaldo Carvalho de Melo wrote: > > > > Em Wed, Nov 29, 2017 at 01:07:34PM -0800, Martin KaFai Lau escreveu: > > > > > On Tue, Nov 28, 2017 at 04:05:19PM -0300, Arnaldo Carvalho de Melo wrote: > > > > > > [root@jouet ~]# perf test -v bpf > > > > > > 39: BPF filter : > > > > > > 39.1: Basic BPF filtering : > > > > > > Kernel build dir is set to /lib/modules/4.14.0+/build > > > > > [ ... ] > > > > > > libbpf: failed to create map (name: 'flip_table'): Invalid argument > > > > > > libbpf: failed to load object '[basic_bpf_test]' > > > > > > bpf: load objects failed > > > > > 88cda1c9da02 ("bpf: libbpf: Provide basic API support to specify BPF obj name") > > > > > is introduced in 4.15. > > > > > > > I think the perf@kernel-4.15 broke on older kernels like 4.14 because > > > > > the new bpf prog/map name is only introduced since 4.15. > > > > > > > The newer perf needs to be compatible with an older kernel? > > > > > > Sure :-) > > > > Would the latest features introduced in perf/libbpf supposed to be > > > available in the latest kernel only? What may be the reason that the > > > > Yes, then the new perf binary should try to use the new stuff, if it > > fails, use the old one, there is no requirement that one uses perf 4.14 > > in lockstep with the kernel 4.14 (or any other version), perf 4.15 > > should work with the 4.14 kernel as well as with 4.15 (or any other > > future kernel), only limited by what it can grok up to when it was > > released. > > So, see the patch below, that makes a 'perf test bpf' and my other test > cases, including that one for probe_read_str() work again, it just > fallbacks to a behaviour the older kernels can accept. Thanks for the patch.
> > We can improve it so that that EINVAL fallback happens only for > MAP_CREATE, and probably we don't need to change the size arg, just zero > the unused fields, but I haven't checked that. > > - Arnaldo > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > index 5128677e4117..3084f07c7c33 100644 > --- a/tools/lib/bpf/bpf.c > +++ b/tools/lib/bpf/bpf.c > @@ -19,6 +19,7 @@ > * License along with this program; if not, see <http://www.gnu.org/licenses> > */ > > +#include <errno.h> > #include <stdlib.h> > #include <memory.h> > #include <unistd.h> > @@ -53,10 +54,26 @@ static inline __u64 ptr_to_u64(const void *ptr) > return (__u64) (unsigned long) ptr; > } > > -static inline int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr, > - unsigned int size) > +static int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr, unsigned int size) > { > - return syscall(__NR_bpf, cmd, attr, size); > + int err = syscall(__NR_bpf, cmd, attr, size); > + if (err == -1 && (errno == EINVAL || errno == E2BIG)) { I would add a check to the length of map_name/prog_name.
> + const unsigned int old_union_size = offsetof(union bpf_attr, prog_name); > + /* > + * These were the ones that added fields after the old bpf_attr > + * layout in commit 88cda1c9da02 ("bpf: libbpf: Provide basic > + * API support to specify BPF obj name") so zero that out to > + * pass the CHECK_ATTR() test in kernel/bpf/syscall.c in older > + * kernels. > + */ > + if (cmd == BPF_MAP_CREATE) > + memset(&attr->map_name, 0, size - offsetof(union bpf_attr, map_name)); > + else > + memset(&attr->prog_name, 0, size - old_union_size); If bpf_attr is extended in the future, map_name/prog_name will still be used as the anchor for backward compatibility instead of trial and error attribute by attribute?
Instead of sinking all future bpf_attr's backward compatibility requirements to sys_bpf, I would push it up to its own BPF_* command helper which has a better sense of its bpf_attr, i.e. push it up to bpf_create_map_node() and bpf_load_program_name() in this case.
> + > + err = syscall(__NR_bpf, cmd, attr, old_union_size); > + } > + return err; > } > > int bpf_create_map_node(enum bpf_map_type map_type, const char *name,
| |