Daniel Borkmann <dan...@iogearbox.net> [Thu, 2018-11-22 02:28 -0800]: > On 11/21/2018 11:22 PM, Alexei Starovoitov wrote: > > On 11/21/18 12:18 PM, Yonghong Song wrote: > >> On 11/21/18 9:40 AM, Andrey Ignatov wrote: > >>> More and more projects use libbpf and one day it'll likely be packaged > >>> and distributed as DSO and that requires ABI versioning so that both > >>> compatible and incompatible changes to ABI can be introduced in a safe > >>> way in the future without breaking executables dynamically linked with a > >>> previous version of the library. > >>> > >>> Usual way to do ABI versioning is version script for the linker. Add > >>> such a script for libbpf. All global symbols currently exported via > >>> LIBBPF_API macro are added to the version script libbpf.map. > >>> > >>> The version name LIBBPF_0.0.1 is constructed from the name of the > >>> library + version specified by $(LIBBPF_VERSION) in Makefile. > >>> > >>> Version script does not duplicate the work done by LIBBPF_API macro, it > >>> rather complements it. The macro is used at compile time and can be used > >>> by compiler to do optimization that can't be done at link time, it is > >>> purely about global symbol visibility. The version script, in turn, is > >>> used at link time and takes care of ABI versioning. Both techniques are > >>> described in details in [1]. > >>> > >>> Whenever ABI is changed in the future, version script should be changed > >>> appropriately. > >> > >> Maybe we should clarify the policy of how version numbers should be > >> change? Each commit which changes default global symbol ABI? Each kernel > >> release? > > +1, could you add a documentation file into tools/lib/bpf/ where we > keep note on this process?
That makes sense. I'll add documentation. I think it'll take time to figure out a policy to maintain ABI that works well (like when to bump version, etc). I'll describe what is reasonable from my point of view so that we have a starting point and we can refine / adjust it to reality later. > >>> [1] > >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.akkadia.org_drepper_dsohowto.pdf&d=DwICaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=3jAokpHyGuCuJ834j-tttQ&m=DaYaGCQXLC7Lqf82VhtHjSPrf6R4RdDMKrDDR2T9XPA&s=nN4Sz6re4n-pP50ICk8s0M-nu_535bblSiVPeEdGiFk&e= > >>> > >>> Signed-off-by: Andrey Ignatov <r...@fb.com> > >>> --- > >>> tools/lib/bpf/Makefile | 4 +- > >>> tools/lib/bpf/libbpf.map | 120 +++++++++++++++++++++++++++++++++++++++ > >>> 2 files changed, 123 insertions(+), 1 deletion(-) > >>> create mode 100644 tools/lib/bpf/libbpf.map > >>> > >>> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile > >>> index 425b480bda75..d76c41fa2d39 100644 > >>> --- a/tools/lib/bpf/Makefile > >>> +++ b/tools/lib/bpf/Makefile > >>> @@ -145,6 +145,7 @@ include $(srctree)/tools/build/Makefile.include > >>> > >>> BPF_IN := $(OUTPUT)libbpf-in.o > >>> LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE)) > >>> +VERSION_SCRIPT := libbpf.map > >>> > >>> CMD_TARGETS = $(LIB_FILE) > >>> > >>> @@ -170,7 +171,8 @@ $(BPF_IN): force elfdep bpfdep > >>> $(Q)$(MAKE) $(build)=libbpf > >>> > >>> $(OUTPUT)libbpf.so: $(BPF_IN) > >>> - $(QUIET_LINK)$(CC) --shared $^ -o $@ > >>> + $(QUIET_LINK)$(CC) --shared -Wl,--version-script=$(VERSION_SCRIPT) \ > >>> + $^ -o $@ > >>> > >>> $(OUTPUT)libbpf.a: $(BPF_IN) > >>> $(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^ > >>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > >>> new file mode 100644 > >>> index 000000000000..9fe416b68c7d > >>> --- /dev/null > >>> +++ b/tools/lib/bpf/libbpf.map > >>> @@ -0,0 +1,120 @@ > >>> +LIBBPF_0.0.1 { > >>> + global: > >>> + bpf_btf_get_fd_by_id; > >> > >> Do you think we could use this opportunities to > >> make naming more consistent? For example, > >> bpf_btf_get_fd_by_id => btf__get_fd_by_id? > > > > I think this one is fine since it matches > > bpf_[map|prog]_get_fd_by_id() > > and it's a wrapper. > > > >>> + bpf_create_map; > >>> + bpf_create_map_in_map; > >>> + bpf_create_map_in_map_node; > >>> + bpf_create_map_name; > >>> + bpf_create_map_node; > >>> + bpf_create_map_xattr; > >>> + bpf_load_btf; > >>> + bpf_load_program; > >>> + bpf_load_program_xattr; > >>> + bpf_map__btf_key_type_id; > >>> + bpf_map__btf_value_type_id; > >>> + bpf_map__def; > >>> + bpf_map_delete_elem; > + bpf_map__fd; > >>> + bpf_map_get_fd_by_id; > >>> + bpf_map_get_next_id; > >>> + bpf_map_get_next_key; > + > >>> bpf_map__is_offload_neutral; > >>> + bpf_map_lookup_and_delete_elem; > >>> + bpf_map_lookup_elem; > >>> + bpf_map__name; > >>> + bpf_map__next; > >>> + bpf_map__pin; > >>> + bpf_map__prev; > >>> + bpf_map__priv; > >>> + bpf_map__reuse_fd; > >>> + bpf_map__set_ifindex; > >>> + bpf_map__set_priv; > >>> + bpf_map__unpin; > >>> + bpf_map_update_elem; > >>> + bpf_object__btf_fd; > >>> + bpf_object__close; > >>> + bpf_object__find_map_by_name; > >>> + bpf_object__find_map_by_offset; > >>> + bpf_object__find_program_by_title; > >>> + bpf_object__kversion; > >>> + bpf_object__load; > >>> + bpf_object__name; > >>> + bpf_object__next; > >>> + bpf_object__open; > >>> + bpf_object__open_buffer; > >>> + bpf_object__open_xattr; > >>> + bpf_object__pin; > >>> + bpf_object__pin_maps; > >>> + bpf_object__pin_programs; > >>> + bpf_object__priv; > >>> + bpf_object__set_priv; > >>> + bpf_object__unload; > >>> + bpf_object__unpin_maps; > >>> + bpf_object__unpin_programs; > >>> + bpf_obj_get; > >>> + bpf_obj_get_info_by_fd; > >>> + bpf_obj_pin; > >>> + bpf_perf_event_read_simple; > >>> + bpf_prog_attach; > >>> + bpf_prog_detach; > >>> + bpf_prog_detach2; > >>> + bpf_prog_get_fd_by_id; > >>> + bpf_prog_get_next_id; > >>> + bpf_prog_load; > >>> + bpf_prog_load_xattr; > >>> + bpf_prog_query; > >>> + bpf_program__fd; > >>> + bpf_program__is_kprobe; > >>> + bpf_program__is_perf_event; > >>> + bpf_program__is_raw_tracepoint; > >>> + bpf_program__is_sched_act; > >>> + bpf_program__is_sched_cls; > >>> + bpf_program__is_socket_filter; > >>> + bpf_program__is_tracepoint; > >>> + bpf_program__is_xdp; > >>> + bpf_program__load; > >>> + bpf_program__next; > >>> + bpf_program__nth_fd; > >>> + bpf_program__pin; > >>> + bpf_program__pin_instance; > >>> + bpf_program__prev; > >>> + bpf_program__priv; > >>> + bpf_program__set_expected_attach_type; > >>> + bpf_program__set_ifindex; > >>> + bpf_program__set_kprobe; > >>> + bpf_program__set_perf_event; > >>> + bpf_program__set_prep; > >>> + bpf_program__set_priv; > >>> + bpf_program__set_raw_tracepoint; > >>> + bpf_program__set_sched_act; > >>> + bpf_program__set_sched_cls; > >>> + bpf_program__set_socket_filter; > >>> + bpf_program__set_tracepoint; > >>> + bpf_program__set_type; > >>> + bpf_program__set_xdp; > >>> + bpf_program__title; > >>> + bpf_program__unload; > >>> + bpf_program__unpin; > >>> + bpf_program__unpin_instance; > >>> + bpf_prog_test_run; > >>> + bpf_raw_tracepoint_open; > >>> + bpf_set_link_xdp_fd; > >>> + bpf_task_fd_query; > >>> + bpf_verify_program; > >>> + btf__fd; > >>> + btf__find_by_name; > >>> + btf__free; > >>> + btf_get_from_id; > >> btf_get_from_id => btf__get_from_id? > > > > this one makes sense indeed. Martin, thoughts? > > > >>> + btf__name_by_offset; > >>> + btf__new; > >>> + btf__resolve_size; > >>> + btf__resolve_type; > >>> + btf__type_by_id; > >>> + libbpf_attach_type_by_name; > >>> + libbpf_get_error; > >>> + libbpf_prog_type_by_name; > >>> + libbpf_set_print; > >>> + libbpf_strerror; > >> > >> Anything else? We have btf__, bpf_program__ prefixes with double "_" > >> while with libbpf_, bpf_<wrapper> as single "_". Not sure whether they > >> need change or not. > > > > the convention is that syscall wrappers like bpf_map_lookup_elem() > > have single _ and map one-to-one to syscall commands. > > Double _ is for objects. That's a coding style of perf. > > Today btf, bpf_program, bpf_map, bpf_objects are objects. > > libbpf_ is a prefix for auxiliary funcs. > > With that in mind, should we also change prototype for things like > bpf_set_link_xdp_fd() and bpf_perf_event_read_simple()? It's not a > syscall wrapper mapping one-to-one either. > > Would libbpf_ prefix be a better fit in here, or some new bpf_misc__ > prefix for everything else which is not directly related to libbpf > internals and neither to the existing objects? (bpf_misc__ would > probably be my slight preference fwiw.) > > > We need to document it in a readme file. > > Fully agree, this should probably go to that same document so that > this convention is clear to everyone extending libbpf. These both are good points. I'll try to document API naming convetion to the extent I have context on this and combine it with ABI documentation. As for bpf_set_link_xdp_fd() and bpf_perf_event_read_simple() I'm not really sure since I hardly ever use. I can rename things that we're sure about (like Martin's s/btf_get_from_id/btf__get_from_id/ above, that I'll include in v2). > > Thanks, > Daniel -- Andrey Ignatov
