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? >>> [1] https://www.akkadia.org/drepper/dsohowto.pdf >>> >>> 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. Thanks, Daniel
