[PATCH bpf-next v2 0/4] libbpf: ABI versioning and documentation

[PATCH bpf-next v2 0/4] libbpf: ABI versioning and documentation

[Andrey Ignatov]

This patch set adds ABI versioning and documentation to libbpf.

Patch 1 renames btf_get_from_id to btf__get_from_id to follow naming
convention.
Patch 2 adds version script and has more details on ABI versioning.
Patch 3 adds simple check that all global symbols are versioned.
Patch 4 documents a few aspects of libbpf API and ABI in dev process.

v1->v2:
* add patch from Martin KaFai Lau <kafai@fb.com> to rename btf_get_from_id;
* add documentation for libbpf API and ABI.


Andrey Ignatov (3):
  libbpf: Add version script for DSO
  libbpf: Verify versioned symbols
  libbpf: Document API and ABI conventions

Martin KaFai Lau (1):
  libbpf: Name changing for btf_get_from_id

 tools/bpf/bpftool/map.c                |   4 +-
 tools/bpf/bpftool/prog.c               |   2 +-
 tools/lib/bpf/Makefile                 |  23 +++-
 tools/lib/bpf/README.rst               | 139 +++++++++++++++++++++++++
 tools/lib/bpf/btf.c                    |   2 +-
 tools/lib/bpf/btf.h                    |   2 +-
 tools/lib/bpf/libbpf.map               | 121 +++++++++++++++++++++
 tools/testing/selftests/bpf/test_btf.c |   2 +-
 8 files changed, 287 insertions(+), 8 deletions(-)
 create mode 100644 tools/lib/bpf/README.rst
 create mode 100644 tools/lib/bpf/libbpf.map

-- 
2.17.1

[PATCH bpf-next v2 1/4] libbpf: Name changing for btf_get_from_id

[Andrey Ignatov]

From: Martin KaFai Lau <kafai@fb.com>

s/btf_get_from_id/btf__get_from_id/ to restore the API naming convention.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
 tools/bpf/bpftool/map.c                | 4 ++--
 tools/bpf/bpftool/prog.c               | 2 +-
 tools/lib/bpf/btf.c                    | 2 +-
 tools/lib/bpf/btf.h                    | 2 +-
 tools/testing/selftests/bpf/test_btf.c | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index a1ae2a3e9fef..96be42f288f5 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -711,7 +711,7 @@ static int do_dump(int argc, char **argv)
 
 	prev_key = NULL;
 
-	err = btf_get_from_id(info.btf_id, &btf);
+	err = btf__get_from_id(info.btf_id, &btf);
 	if (err) {
 		p_err("failed to get btf");
 		goto exit_free;
@@ -855,7 +855,7 @@ static int do_lookup(int argc, char **argv)
 	}
 
 	/* here means bpf_map_lookup_elem() succeeded */
-	err = btf_get_from_id(info.btf_id, &btf);
+	err = btf__get_from_id(info.btf_id, &btf);
 	if (err) {
 		p_err("failed to get btf");
 		goto exit_free;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 37b1daf19da6..521a1073d1b4 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -622,7 +622,7 @@ static int do_dump(int argc, char **argv)
 		goto err_free;
 	}
 
-	if (info.btf_id && btf_get_from_id(info.btf_id, &btf)) {
+	if (info.btf_id && btf__get_from_id(info.btf_id, &btf)) {
 		p_err("failed to get btf");
 		goto err_free;
 	}
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 13ddc4bd24ee..eadcf8dfd295 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -415,7 +415,7 @@ const char *btf__name_by_offset(const struct btf *btf, __u32 offset)
 		return NULL;
 }
 
-int btf_get_from_id(__u32 id, struct btf **btf)
+int btf__get_from_id(__u32 id, struct btf **btf)
 {
 	struct bpf_btf_info btf_info = { 0 };
 	__u32 len = sizeof(btf_info);
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 701ad2b6c41f..5336b2f37293 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -73,7 +73,7 @@ LIBBPF_API __s64 btf__resolve_size(const struct btf *btf, __u32 type_id);
 LIBBPF_API int btf__resolve_type(const struct btf *btf, __u32 type_id);
 LIBBPF_API int btf__fd(const struct btf *btf);
 LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
-LIBBPF_API int btf_get_from_id(__u32 id, struct btf **btf);
+LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
 
 struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log);
 void btf_ext__free(struct btf_ext *btf_ext);
diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
index bcbda7037840..dff755a7940b 100644
--- a/tools/testing/selftests/bpf/test_btf.c
+++ b/tools/testing/selftests/bpf/test_btf.c
@@ -2604,7 +2604,7 @@ static int do_test_file(unsigned int test_num)
 		goto done;
 	}
 
-	err = btf_get_from_id(info.btf_id, &btf);
+	err = btf__get_from_id(info.btf_id, &btf);
 	if (CHECK(err, "cannot get btf from kernel, err: %d", err))
 		goto done;
 
-- 
2.17.1

[PATCH bpf-next v2 2/4] libbpf: Add version script for DSO

[Andrey Ignatov]

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.

[1] https://www.akkadia.org/drepper/dsohowto.pdf

Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
 tools/lib/bpf/Makefile   |   4 +-
 tools/lib/bpf/libbpf.map | 121 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+), 1 deletion(-)
 create mode 100644 tools/lib/bpf/libbpf.map

diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 1b4a683a00fc..22c5ffe22825 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)
 
@@ -176,7 +177,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..4fb29f6d7a80
--- /dev/null
+++ b/tools/lib/bpf/libbpf.map
@@ -0,0 +1,121 @@
+LIBBPF_0.0.1 {
+	global:
+		bpf_btf_get_fd_by_id;
+		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__fd;
+		bpf_map__is_offload_neutral;
+		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_inner_map_fd;
+		bpf_map__set_priv;
+		bpf_map__unpin;
+		bpf_map_delete_elem;
+		bpf_map_get_fd_by_id;
+		bpf_map_get_next_id;
+		bpf_map_get_next_key;
+		bpf_map_lookup_and_delete_elem;
+		bpf_map_lookup_elem;
+		bpf_map_update_elem;
+		bpf_obj_get;
+		bpf_obj_get_info_by_fd;
+		bpf_obj_pin;
+		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_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_prog_test_run;
+		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_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__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;
+	local:
+		*;
+};
-- 
2.17.1

[PATCH bpf-next v2 3/4] libbpf: Verify versioned symbols

[Andrey Ignatov]

Since ABI versioning info is kept separately from the code it's easy to
forget to update it while adding a new API.

Add simple verification that all global symbols exported with LIBBPF_API
are versioned in libbpf.map version script.

The idea is to check that number of global symbols in libbpf-in.o, that
is the input to the linker, matches with number of unique versioned
symbols in libbpf.so, that is the output of the linker. If these numbers
don't match, it may mean some symbol was not versioned and make will
fail.

"Unique" means that if a symbol is present in more than one version of
ABI due to ABI changes, it'll be counted once.

Another option to calculate number of global symbols in the "input"
could be to count number of LIBBPF_ABI entries in C headers but it seems
to be fragile.

Example of output when a symbol is missing in version script:

    ...
    LD       libbpf-in.o
    LINK     libbpf.a
    LINK     libbpf.so
  Warning: Num of global symbols in libbpf-in.o (115) does NOT match
  with num of versioned symbols in libbpf.so (114). Please make sure all
  LIBBPF_API symbols are versioned in libbpf.map.
  make: *** [check_abi] Error 1

Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
 tools/lib/bpf/Makefile | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 22c5ffe22825..34d9c3619c96 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -147,6 +147,11 @@ BPF_IN    := $(OUTPUT)libbpf-in.o
 LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
 VERSION_SCRIPT := libbpf.map
 
+GLOBAL_SYM_COUNT = $(shell readelf -s $(BPF_IN) | \
+			   awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {s++} END{print s}')
+VERSIONED_SYM_COUNT = $(shell readelf -s $(OUTPUT)libbpf.so | \
+			      grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort -u | wc -l)
+
 CMD_TARGETS = $(LIB_FILE)
 
 CXX_TEST_TARGET = $(OUTPUT)test_libbpf
@@ -159,7 +164,7 @@ TARGETS = $(CMD_TARGETS)
 
 all: fixdep all_cmd
 
-all_cmd: $(CMD_TARGETS)
+all_cmd: $(CMD_TARGETS) check
 
 $(BPF_IN): force elfdep bpfdep
 	@(test -f ../../include/uapi/linux/bpf.h -a -f ../../../include/uapi/linux/bpf.h && ( \
@@ -186,6 +191,18 @@ $(OUTPUT)libbpf.a: $(BPF_IN)
 $(OUTPUT)test_libbpf: test_libbpf.cpp $(OUTPUT)libbpf.a
 	$(QUIET_LINK)$(CXX) $^ -lelf -o $@
 
+check: check_abi
+
+check_abi: $(OUTPUT)libbpf.so
+	@if [ "$(GLOBAL_SYM_COUNT)" != "$(VERSIONED_SYM_COUNT)" ]; then	 \
+		echo "Warning: Num of global symbols in $(BPF_IN)"	 \
+		     "($(GLOBAL_SYM_COUNT)) does NOT match with num of"	 \
+		     "versioned symbols in $^ ($(VERSIONED_SYM_COUNT))." \
+		     "Please make sure all LIBBPF_API symbols are"	 \
+		     "versioned in $(VERSION_SCRIPT)." >&2;		 \
+		exit 1;							 \
+	fi
+
 define do_install
 	if [ ! -d '$(DESTDIR_SQ)$2' ]; then		\
 		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2';	\
-- 
2.17.1

[PATCH bpf-next v2 4/4] libbpf: Document API and ABI conventions

[Andrey Ignatov]

Document API and ABI for libbpf: naming convention, symbol visibility,
ABI versioning.

This is just a starting point. Documentation can be significantly
extended in the future to cover more topics.

ABI versioning section touches only a few basic points with a link to
more comprehensive documentation from Ulrich Drepper. This section can
be extended in the future when there is better understanding what works
well and what not so well in libbpf development process and production
usage.

Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
 tools/lib/bpf/README.rst | 139 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)
 create mode 100644 tools/lib/bpf/README.rst

diff --git a/tools/lib/bpf/README.rst b/tools/lib/bpf/README.rst
new file mode 100644
index 000000000000..2ced9e061c4b
--- /dev/null
+++ b/tools/lib/bpf/README.rst
@@ -0,0 +1,139 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+libbpf API naming convention
+============================
+
+libbpf API provides access to a few logically separated groups of
+functions and types. Every group has its own naming convention
+described here. It's recommended to follow these conventions whenever a
+new function or type is added to keep libbpf API clean and consistent.
+
+All types and functions provided by libbpf API should have one of the
+following prefixes: ``bpf_``, ``btf_``, ``libbpf_``.
+
+System call wrappers
+--------------------
+
+System call wrappers are simple wrappers for commands supported by
+sys_bpf system call. These wrappers should go to ``bpf.h`` header file
+and map one-on-one to corresponding commands.
+
+For example ``bpf_map_lookup_elem`` wraps ``BPF_MAP_LOOKUP_ELEM``
+command of sys_bpf, ``bpf_prog_attach`` wraps ``BPF_PROG_ATTACH``, etc.
+
+Objects
+-------
+
+Another class of types and functions provided by libbpf API is "objects"
+and functions to work with them. Objects are high-level abstractions
+such as BPF program or BPF map. They're represented by corresponding
+structures such as ``struct bpf_object``, ``struct bpf_program``,
+``struct bpf_map``, etc.
+
+Structures are forward declared and access to their fields should be
+provided via corresponding getters and setters rather than directly.
+
+These objects are associated with corresponding parts of ELF object that
+contains compiled BPF programs.
+
+For example ``struct bpf_object`` represents ELF object itself created
+from an ELF file or from a buffer, ``struct bpf_program`` represents a
+program in ELF object and ``struct bpf_map`` is a map.
+
+Functions that work with an object have names built from object name,
+double underscore and part that describes function purpose.
+
+For example ``bpf_object__open`` consists of the name of corresponding
+object, ``bpf_object``, double underscore and ``open`` that defines the
+purpose of the function to open ELF file and create ``bpf_object`` from
+it.
+
+Another example: ``bpf_program__load`` is named for corresponding
+object, ``bpf_program``, that is separated from other part of the name
+by double underscore.
+
+All objects and corresponding functions other than BTF related should go
+to ``libbpf.h``. BTF types and functions should go to ``btf.h``.
+
+Auxiliary functions
+-------------------
+
+Auxiliary functions and types that don't fit well in any of categories
+described above should have ``libbpf_`` prefix, e.g.
+``libbpf_get_error`` or ``libbpf_prog_type_by_name``.
+
+libbpf ABI
+==========
+
+libbpf can be both linked statically or used as DSO. To avoid possible
+conflicts with other libraries an application is linked with, all
+non-static libbpf symbols should have one of the prefixes mentioned in
+API documentation above. See API naming convention to choose the right
+name for a new symbol.
+
+Symbol visibility
+-----------------
+
+libbpf follow the model when all global symbols have visibility "hidden"
+by default and to make a symbol visible it has to be explicitly
+attributed with ``LIBBPF_API`` macro. For example:
+
+.. code-block:: c
+
+        LIBBPF_API int bpf_prog_get_fd_by_id(__u32 id);
+
+This prevents from accidentally exporting a symbol, that is not supposed
+to be a part of ABI what, in turn, improves both libbpf developer- and
+user-experiences.
+
+ABI versionning
+---------------
+
+To make future ABI extensions possible libbpf ABI is versioned.
+Versioning is implemented by ``libbpf.map`` version script that is
+passed to linker.
+
+Version name is ``LIBBPF_`` prefix + three-component numeric version,
+starting from ``0.0.1``.
+
+Every time ABI is being changed, e.g. because a new symbol is added or
+semantic of existing symbol is changed, ABI version should be bumped.
+
+For example, if current state of ``libbpf.map`` is:
+
+.. code-block::
+        LIBBPF_0.0.1 {
+        	global:
+                        bpf_func_a;
+                        bpf_func_b;
+        	local:
+        		\*;
+        };
+
+, and a new symbol ``bpf_func_c`` is being introduced, then
+``libbpf.map`` should be changed like this:
+
+.. code-block::
+        LIBBPF_0.0.1 {
+        	global:
+                        bpf_func_a;
+                        bpf_func_b;
+        	local:
+        		\*;
+        };
+        LIBBPF_0.0.2 {
+                global:
+                        bpf_func_c;
+        } LIBBPF_0.0.1;
+
+, where new version ``LIBBPF_0.0.2`` depends on the previous
+``LIBBPF_0.0.1``.
+
+Format of version script and ways to handle ABI changes, including
+incompatible ones, described in details in [1].
+
+Links
+=====
+
+[1] https://www.akkadia.org/drepper/dsohowto.pdf
+    (Chapter 3. Maintaining APIs and ABIs).
-- 
2.17.1

Re: [PATCH bpf-next v2 0/4] libbpf: ABI versioning and documentation

[Alexei Starovoitov]

On Fri, Nov 23, 2018 at 04:44:31PM -0800, Andrey Ignatov wrote:
> This patch set adds ABI versioning and documentation to libbpf.
> 
> Patch 1 renames btf_get_from_id to btf__get_from_id to follow naming
> convention.
> Patch 2 adds version script and has more details on ABI versioning.
> Patch 3 adds simple check that all global symbols are versioned.
> Patch 4 documents a few aspects of libbpf API and ABI in dev process.
> 
> v1->v2:
> * add patch from Martin KaFai Lau <kafai@fb.com> to rename btf_get_from_id;
> * add documentation for libbpf API and ABI.

All looks great to me.
Thank you for adding the doc.
Applied to bpf-next.

We need to discuss the release model and version bumps.
For example I don't think it's necessary to bump the version
and update libbpf.map every time the new function is added.
I mean we can synchronize release of libbpf with the release of the kernel
or release it every N weeks.
So if we add new api functions during this release we can simply
add them to libbpf.map while keeping the version as 0.0.1

I'd also consider the first 0.0.1 release to be experimental
while we're figuring out the right process.
For the next kernel/libbpf release I propose to bump it to 1.0.0

Another idea is to version it just like kernel and make libbpf version
always equal kernel version.
But I think that would be an overkill. libbpf isn't tightly coupled to
the kernel. Like we just merged the patch (prog_name/map_name probing
that allows new libbpf to work with older kernel.

Re: [PATCH bpf-next v2 0/4] libbpf: ABI versioning and documentation

[Daniel Borkmann]

On 11/27/2018 04:06 AM, Alexei Starovoitov wrote:
> On Fri, Nov 23, 2018 at 04:44:31PM -0800, Andrey Ignatov wrote:
>> This patch set adds ABI versioning and documentation to libbpf.
>>
>> Patch 1 renames btf_get_from_id to btf__get_from_id to follow naming
>> convention.
>> Patch 2 adds version script and has more details on ABI versioning.
>> Patch 3 adds simple check that all global symbols are versioned.
>> Patch 4 documents a few aspects of libbpf API and ABI in dev process.
>>
>> v1->v2:
>> * add patch from Martin KaFai Lau <kafai@fb.com> to rename btf_get_from_id;
>> * add documentation for libbpf API and ABI.
> 
> All looks great to me.
> Thank you for adding the doc.
> Applied to bpf-next.
> 
> We need to discuss the release model and version bumps.
> For example I don't think it's necessary to bump the version
> and update libbpf.map every time the new function is added.
> I mean we can synchronize release of libbpf with the release of the kernel
> or release it every N weeks.

+1, synchronizing with release of the kernel seems a natural fit
given it's part of the kernel tree. Every N weeks might not even
work since changes are not in Linus' tree at that point, I think
this would probably just lead to confusion.

> So if we add new api functions during this release we can simply
> add them to libbpf.map while keeping the version as 0.0.1
> 
> I'd also consider the first 0.0.1 release to be experimental
> while we're figuring out the right process.
> For the next kernel/libbpf release I propose to bump it to 1.0.0
> 
> Another idea is to version it just like kernel and make libbpf version
> always equal kernel version.
> But I think that would be an overkill. libbpf isn't tightly coupled to
> the kernel. Like we just merged the patch (prog_name/map_name probing
> that allows new libbpf to work with older kernel.

Right, though we might end up bumping version each kernel release
anyway. This would allow for more automation on that part if we
could simply say that version looks like libbpf.so.4.20 or
libbpf.so.4.21, etc, without us forgetting to send an extra patch
every release just to bump version. Whether it's version -0.0.1,
-1.0.0, -4.20 or say -2018.11.27 is probably mostly matter of
preference but for the former two we would have to additionally
establish some convention / process on when to bump which part
of the three version component. I'd rather prefer if we could
have kernel-like convention where a bump in major version is "just"
yet another release but has no other special meaning attached to
it in which case we could just adapt the same numbering scheme.

Thanks,
Daniel

Re: [PATCH bpf-next v2 0/4] libbpf: ABI versioning and documentation

[Andrey Ignatov]

Daniel Borkmann <daniel@iogearbox.net> [Tue, 2018-11-27 03:36 -0800]:
> On 11/27/2018 04:06 AM, Alexei Starovoitov wrote:
> > On Fri, Nov 23, 2018 at 04:44:31PM -0800, Andrey Ignatov wrote:
> >> This patch set adds ABI versioning and documentation to libbpf.
> >>
> >> Patch 1 renames btf_get_from_id to btf__get_from_id to follow naming
> >> convention.
> >> Patch 2 adds version script and has more details on ABI versioning.
> >> Patch 3 adds simple check that all global symbols are versioned.
> >> Patch 4 documents a few aspects of libbpf API and ABI in dev process.
> >>
> >> v1->v2:
> >> * add patch from Martin KaFai Lau <kafai@fb.com> to rename btf_get_from_id;
> >> * add documentation for libbpf API and ABI.
> > 
> > All looks great to me.
> > Thank you for adding the doc.
> > Applied to bpf-next.
> > 
> > We need to discuss the release model and version bumps.
> > For example I don't think it's necessary to bump the version
> > and update libbpf.map every time the new function is added.
> > I mean we can synchronize release of libbpf with the release of the kernel
> > or release it every N weeks.
> 
> +1, synchronizing with release of the kernel seems a natural fit
> given it's part of the kernel tree. Every N weeks might not even
> work since changes are not in Linus' tree at that point, I think
> this would probably just lead to confusion.

Sorry for delay folks, didn't have a chance to answer earlier.

After some thinking I agree that bumping ABI version every time, a new
symbol is being added, is probably overkill.

Though I think all new symbols added in a new release should go to
corresponding new ABI version, i.e. during release preparation we may
have a version X and all new symbols go to this version X. Once release
happens and X is released, no new symbols should be added to X and
instead a new X + 1 ABI version should be used.

If we simply add new symbols to existing ABI version (without
coordinating it with release) I can imaging a problem when application
is build and dynamically linked with libbpf on one host B, but run on a
different host R, both hosts have exactly same ABI version X of libbpf
DSO, but libbpf on B is newer and has some symbols (used by application)
that are not available in libbpf on R. In this scenario when application
is started on host R, dynamic linker will get DT_VERNEED from it,
identify that libbpf of version X is needed, will find that libbpf on
the host has this same version in DT_VERDEFNUM and will assume that all
symbols of that version are available, but will actually fail later on
symbol lookup for any "new" symbol.

It seems we all agree that ABI version bump should be synchronized with
some release.

> > So if we add new api functions during this release we can simply
> > add them to libbpf.map while keeping the version as 0.0.1

Yeah, "during" is very important IMO, but not after release happened.


> > I'd also consider the first 0.0.1 release to be experimental
> > while we're figuring out the right process.
> > For the next kernel/libbpf release I propose to bump it to 1.0.0
> > 
> > Another idea is to version it just like kernel and make libbpf version
> > always equal kernel version.
> > But I think that would be an overkill. libbpf isn't tightly coupled to
> > the kernel. Like we just merged the patch (prog_name/map_name probing
> > that allows new libbpf to work with older kernel.
> 
> Right, though we might end up bumping version each kernel release
> anyway. This would allow for more automation on that part if we
> could simply say that version looks like libbpf.so.4.20 or
> libbpf.so.4.21, etc, without us forgetting to send an extra patch

I think there is a confusion here. libbpf.so.4.20 and libbpf.so.4.21 are
two different file names, what usually means different SONAME. Renaming
a library this way is another way of versioning, yeah, but it's the
coarsest grained one that has many drawbacks and is basically the last
resort when other methods don't work.

ABI versioning added in this patch series is the way to avoid file
renaming and provide fine grained control over symbol versioning
instead.

That's being said the DSO file name should not be changed every time we
bump ABI version in the version script.

> every release just to bump version. Whether it's version -0.0.1,
> -1.0.0, -4.20 or say -2018.11.27 is probably mostly matter of
> preference but for the former two we would have to additionally
> establish some convention / process on when to bump which part
> of the three version component. I'd rather prefer if we could
> have kernel-like convention where a bump in major version is "just"
> yet another release but has no other special meaning attached to
> it in which case we could just adapt the same numbering scheme.

I don't have strong opinion on how specifically the version string
should look like.

One idea I have though is about "current" version that is not released
yet and where new symbols may be added. What about naming it
EXPERIMENTAL or something like this? This way we can provide a clear
contract that any experimental symbol can break before it's released
what gives us a room to fix recently added symbols if needed. Once it's
decided to make a new release all new symbols that are stable enough can
be moved from EXPERIMENTAL to a new stable ABI version and kept
compatible. That adds maintenance burden though.

Also we're discussing mostly adding new symbols what is compatible ABI
change. On the other hand if an existing symbol is being changed in an
incompatible way, we have no choice and should bump ABI version, making
the new semantic available in new version, keeping the old semantic in
original version and setting what version of the symbol will be used by
default.  That's the whole other story that we can probably figure out
the first time we have such ABI change.


-- 
Andrey Ignatov