Message-ID: <2c7cdce3-8add-c506-5ceb-24fbdf02b8fb@iogearbox.net>
Date:   Thu, 22 Nov 2018 11:27:52 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Alexei Starovoitov <ast@...com>, Yonghong Song <yhs@...com>,
        Andrey Ignatov <rdna@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     "ast@...nel.org" <ast@...nel.org>, Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/2] libbpf: Add version script for DSO

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 <rdna@...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

Powered by blists - more mailing lists