Message-ID: <20181124003455.GB99799@rdna-mbp.dhcp.thefacebook.com>
Date:   Sat, 24 Nov 2018 00:35:00 +0000
From:   Andrey Ignatov <rdna@...com>
To:     Alexei Starovoitov <ast@...com>, Yonghong Song <yhs@...com>,
        "Daniel Borkmann" <daniel@...earbox.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>, Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/2] libbpf: Add version script for DSO

Daniel Borkmann <daniel@...earbox.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 <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.

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

Powered by blists - more mailing lists