Date:   Thu, 24 Sep 2020 08:56:17 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Andrii Nakryiko <andriin@...com>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, ast@...com, daniel@...earbox.net
Cc:     andrii.nakryiko@...il.com, kernel-team@...com,
        Andrii Nakryiko <andriin@...com>,
        Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: RE: [PATCH bpf-next 5/9] libbpf: allow modification of BTF and add
 btf__add_str API

Andrii Nakryiko wrote:
> Allow internal BTF representation to switch from default read-only mode, in
> which raw BTF data is a single non-modifiable block of memory with BTF header,
> types, and strings layed out sequentially and contiguously in memory, into
> a writable representation with types and strings data split out into separate
> memory regions, that can be dynamically expanded.
> 
> Such writable internal representation is transparent to users of libbpf APIs,
> but allows to append new types and strings at the end of BTF, which is
> a typical use case when generating BTF programmatically. All the basic
> guarantees of BTF types and strings layout is preserved, i.e., user can get
> `struct btf_type *` pointer and read it directly. Such btf_type pointers might
> be invalidated if BTF is modified, so some care is required in such mixed
> read/write scenarios.
> 
> Switch from read-only to writable configuration happens automatically the
> first time when user attempts to modify BTF by either adding a new type or new
> string. It is still possible to get raw BTF data, which is a single piece of
> memory that can be persisted in ELF section or into a file as raw BTF. Such
> raw data memory is also still owned by BTF and will be freed either when BTF
> object is freed or if another modification to BTF happens, as any modification
> invalidates BTF raw representation.
> 
> This patch adds the first BTF writing API: btf__add_str(), which allows to
> add arbitrary strings to BTF string section. All the added strings are
> automatically deduplicated. This is achieved by maintaining an additional
> string lookup index for all unique strings. Such index is built when BTF is
> switched to modifiable mode. If at that time BTF strings section contained
> duplicate strings, they are not de-duplicated. This is done specifically to
> not modify the existing content of BTF (types, their string offsets, etc),
> which can cause confusion and is especially important property if there is
> struct btf_ext associated with struct btf. By following this "imperfect
> deduplication" process, btf_ext is kept consitent and correct. If
> deduplication of strings is necessary, it can be forced by doing BTF
> deduplication, at which point all the strings will be eagerly deduplicated and
> all string offsets both in struct btf and struct btf_ext will be updated.
> 
> Signed-off-by: Andrii Nakryiko <andriin@...com>
> ---

[...]

> +/* Ensure BTF is ready to be modified (by splitting into a three memory
> + * regions for header, types, and strings). Also invalidate cached
> + * raw_data, if any.
> + */
> +static int btf_ensure_modifiable(struct btf *btf)
> +{
> +	void *hdr, *types, *strs, *strs_end, *s;
> +	struct hashmap *hash = NULL;
> +	long off;
> +	int err;
> +
> +	if (btf_is_modifiable(btf)) {
> +		/* any BTF modification invalidates raw_data */
> +		if (btf->raw_data) {

I missed why this case is needed? Just being cautious? It looks like
we get btf->hdr != btf->raw_data (aka btf_is_modifiable) below, but
by the tiime we do this set it looks like we will always null btf->raw_data
as well. Again doesn't appear harmful just seeing if I missed a path.

> +			free(btf->raw_data);
> +			btf->raw_data = NULL;
> +		}
> +		return 0;
> +	}
> +
> +	/* split raw data into three memory regions */
> +	hdr = malloc(btf->hdr->hdr_len);
> +	types = malloc(btf->hdr->type_len);
> +	strs = malloc(btf->hdr->str_len);
> +	if (!hdr || !types || !strs)
> +		goto err_out;
> +
> +	memcpy(hdr, btf->hdr, btf->hdr->hdr_len);
> +	memcpy(types, btf->types_data, btf->hdr->type_len);
> +	memcpy(strs, btf->strs_data, btf->hdr->str_len);
> +
> +	/* build lookup index for all strings */
> +	hash = hashmap__new(strs_hash_fn, strs_hash_equal_fn, btf);
> +	if (IS_ERR(hash)) {
> +		err = PTR_ERR(hash);
> +		hash = NULL;
> +		goto err_out;
> +	}
> +

[...]

Thanks,
John

Powered by blists - more mailing lists