Re: [PATCH v2 bpf-next 1/5] bpf: add in-kernel split BTF support
On Fri, Nov 6, 2020 at 5:28 PM Song Liu wrote:
>
>
>
> > On Nov 6, 2020, at 3:02 PM, Andrii Nakryiko wrote:
> >
> > Adjust in-kernel BTF implementation to support a split BTF mode of
> > operation.
> > Changes are mostly mirroring libbpf split BTF changes, with the exception of
> > start_id being 0 for in-kernel implementation due to simpler read-only mode.
> >
> > Otherwise, for split BTF logic, most of the logic of jumping to base BTF,
> > where necessary, is encapsulated in few helper functions. Type numbering and
> > string offset in a split BTF are logically continuing where base BTF ends,
> > so
> > most of the high-level logic is kept without changes.
> >
> > Type verification and size resolution is only doing an added resolution of
> > new
> > split BTF types and relies on already cached size and type resolution
> > results
> > in the base BTF.
> >
> > Signed-off-by: Andrii Nakryiko
>
> [...]
>
> >
> > @@ -600,8 +618,15 @@ static const struct btf_kind_operations
> > *btf_type_ops(const struct btf_type *t)
> >
> > static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
> > {
> > - return BTF_STR_OFFSET_VALID(offset) &&
> > - offset < btf->hdr.str_len;
> > + if (!BTF_STR_OFFSET_VALID(offset))
> > + return false;
> > +again:
> > + if (offset < btf->start_str_off) {
> > + btf = btf->base_btf;
> > + goto again;
>
> Can we do a while loop instead of "goto again;"?
yep, not sure why I went with goto...
while (offset < btf->start_str_off)
btf = btf->base_btf;
Shorter.
>
> > + }
> > + offset -= btf->start_str_off;
> > + return offset < btf->hdr.str_len;
> > }
> >
> > static bool __btf_name_char_ok(char c, bool first, bool dot_ok)
> > @@ -615,10 +640,25 @@ static bool __btf_name_char_ok(char c, bool first,
> > bool dot_ok)
> > return true;
> > }
> >
> > +static const char *btf_str_by_offset(const struct btf *btf, u32 offset)
> > +{
> > +again:
> > + if (offset < btf->start_str_off) {
> > + btf = btf->base_btf;
> > + goto again;
> > + }
>
> Maybe add a btf_find_base_btf(btf, offset) helper for this logic?
No strong feelings about this, but given it's a two-line loop might
not be worth it. I'd also need a pretty verbose
btf_find_base_btf_for_str_offset() and
btf_find_base_btf_for_type_id(). I feel like loop might be less
distracting actually.
>
> > +
> > + offset -= btf->start_str_off;
> > + if (offset < btf->hdr.str_len)
> > + return >strings[offset];
> > +
> > + return NULL;
> > +}
> > +
>
> [...]
>
> > }
> >
> > const char *btf_name_by_offset(const struct btf *btf, u32 offset)
> > {
> > - if (offset < btf->hdr.str_len)
> > - return >strings[offset];
> > -
> > - return NULL;
> > + return btf_str_by_offset(btf, offset);
> > }
>
> IIUC, btf_str_by_offset() and btf_name_by_offset() are identical. Can we
> just keep btf_name_by_offset()?
btf_str_by_offset() is static, so should be inlinable, while
btf_name_by_offset() is a global function, I was worrying about
regressing performance for __btf_name_valid() and
__btf_name_by_offset(). Premature optimization you think?
>
> >
> > const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id)
> > {
> > - if (type_id > btf->nr_types)
> > - return NULL;
> > +again:
> > + if (type_id < btf->start_id) {
> > + btf = btf->base_btf;
> > + goto again;
> > + }
>
> ditto, goto again..
>
> [...]
>
>
Re: [PATCH v2 bpf-next 1/5] bpf: add in-kernel split BTF support
> On Nov 6, 2020, at 3:02 PM, Andrii Nakryiko wrote:
>
> Adjust in-kernel BTF implementation to support a split BTF mode of operation.
> Changes are mostly mirroring libbpf split BTF changes, with the exception of
> start_id being 0 for in-kernel implementation due to simpler read-only mode.
>
> Otherwise, for split BTF logic, most of the logic of jumping to base BTF,
> where necessary, is encapsulated in few helper functions. Type numbering and
> string offset in a split BTF are logically continuing where base BTF ends, so
> most of the high-level logic is kept without changes.
>
> Type verification and size resolution is only doing an added resolution of new
> split BTF types and relies on already cached size and type resolution results
> in the base BTF.
>
> Signed-off-by: Andrii Nakryiko
[...]
>
> @@ -600,8 +618,15 @@ static const struct btf_kind_operations
> *btf_type_ops(const struct btf_type *t)
>
> static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
> {
> - return BTF_STR_OFFSET_VALID(offset) &&
> - offset < btf->hdr.str_len;
> + if (!BTF_STR_OFFSET_VALID(offset))
> + return false;
> +again:
> + if (offset < btf->start_str_off) {
> + btf = btf->base_btf;
> + goto again;
Can we do a while loop instead of "goto again;"?
> + }
> + offset -= btf->start_str_off;
> + return offset < btf->hdr.str_len;
> }
>
> static bool __btf_name_char_ok(char c, bool first, bool dot_ok)
> @@ -615,10 +640,25 @@ static bool __btf_name_char_ok(char c, bool first, bool
> dot_ok)
> return true;
> }
>
> +static const char *btf_str_by_offset(const struct btf *btf, u32 offset)
> +{
> +again:
> + if (offset < btf->start_str_off) {
> + btf = btf->base_btf;
> + goto again;
> + }
Maybe add a btf_find_base_btf(btf, offset) helper for this logic?
> +
> + offset -= btf->start_str_off;
> + if (offset < btf->hdr.str_len)
> + return >strings[offset];
> +
> + return NULL;
> +}
> +
[...]
> }
>
> const char *btf_name_by_offset(const struct btf *btf, u32 offset)
> {
> - if (offset < btf->hdr.str_len)
> - return >strings[offset];
> -
> - return NULL;
> + return btf_str_by_offset(btf, offset);
> }
IIUC, btf_str_by_offset() and btf_name_by_offset() are identical. Can we
just keep btf_name_by_offset()?
>
> const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id)
> {
> - if (type_id > btf->nr_types)
> - return NULL;
> +again:
> + if (type_id < btf->start_id) {
> + btf = btf->base_btf;
> + goto again;
> + }
ditto, goto again..
[...]
[PATCH v2 bpf-next 1/5] bpf: add in-kernel split BTF support
Adjust in-kernel BTF implementation to support a split BTF mode of operation.
Changes are mostly mirroring libbpf split BTF changes, with the exception of
start_id being 0 for in-kernel implementation due to simpler read-only mode.
Otherwise, for split BTF logic, most of the logic of jumping to base BTF,
where necessary, is encapsulated in few helper functions. Type numbering and
string offset in a split BTF are logically continuing where base BTF ends, so
most of the high-level logic is kept without changes.
Type verification and size resolution is only doing an added resolution of new
split BTF types and relies on already cached size and type resolution results
in the base BTF.
Signed-off-by: Andrii Nakryiko
---
kernel/bpf/btf.c | 182 +--
1 file changed, 130 insertions(+), 52 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index ed7d02e8bc93..f61944a3873b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -204,12 +204,17 @@ struct btf {
const char *strings;
void *nohdr_data;
struct btf_header hdr;
- u32 nr_types;
+ u32 nr_types; /* includes VOID for base BTF */
u32 types_size;
u32 data_size;
refcount_t refcnt;
u32 id;
struct rcu_head rcu;
+
+ /* split BTF support */
+ struct btf *base_btf;
+ u32 start_id; /* first type ID in this BTF (0 for base BTF) */
+ u32 start_str_off; /* first string offset (0 for base BTF) */
};
enum verifier_phase {
@@ -450,14 +455,27 @@ static bool btf_type_is_datasec(const struct btf_type *t)
return BTF_INFO_KIND(t->info) == BTF_KIND_DATASEC;
}
+static u32 btf_nr_types_total(const struct btf *btf)
+{
+ u32 total = 0;
+
+ while (btf) {
+ total += btf->nr_types;
+ btf = btf->base_btf;
+ }
+
+ return total;
+}
+
s32 btf_find_by_name_kind(const struct btf *btf, const char *name, u8 kind)
{
const struct btf_type *t;
const char *tname;
- u32 i;
+ u32 i, total;
- for (i = 1; i <= btf->nr_types; i++) {
- t = btf->types[i];
+ total = btf_nr_types_total(btf);
+ for (i = 1; i < total; i++) {
+ t = btf_type_by_id(btf, i);
if (BTF_INFO_KIND(t->info) != kind)
continue;
@@ -600,8 +618,15 @@ static const struct btf_kind_operations
*btf_type_ops(const struct btf_type *t)
static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
{
- return BTF_STR_OFFSET_VALID(offset) &&
- offset < btf->hdr.str_len;
+ if (!BTF_STR_OFFSET_VALID(offset))
+ return false;
+again:
+ if (offset < btf->start_str_off) {
+ btf = btf->base_btf;
+ goto again;
+ }
+ offset -= btf->start_str_off;
+ return offset < btf->hdr.str_len;
}
static bool __btf_name_char_ok(char c, bool first, bool dot_ok)
@@ -615,10 +640,25 @@ static bool __btf_name_char_ok(char c, bool first, bool
dot_ok)
return true;
}
+static const char *btf_str_by_offset(const struct btf *btf, u32 offset)
+{
+again:
+ if (offset < btf->start_str_off) {
+ btf = btf->base_btf;
+ goto again;
+ }
+
+ offset -= btf->start_str_off;
+ if (offset < btf->hdr.str_len)
+ return >strings[offset];
+
+ return NULL;
+}
+
static bool __btf_name_valid(const struct btf *btf, u32 offset, bool dot_ok)
{
/* offset must be valid */
- const char *src = >strings[offset];
+ const char *src = btf_str_by_offset(btf, offset);
const char *src_limit;
if (!__btf_name_char_ok(*src, true, dot_ok))
@@ -651,27 +691,31 @@ static bool btf_name_valid_section(const struct btf *btf,
u32 offset)
static const char *__btf_name_by_offset(const struct btf *btf, u32 offset)
{
+ const char *name;
+
if (!offset)
return "(anon)";
- else if (offset < btf->hdr.str_len)
- return >strings[offset];
- else
- return "(invalid-name-offset)";
+
+ name = btf_str_by_offset(btf, offset);
+ return name ?: "(invalid-name-offset)";
}
const char *btf_name_by_offset(const struct btf *btf, u32 offset)
{
- if (offset < btf->hdr.str_len)
- return >strings[offset];
-
- return NULL;
+ return btf_str_by_offset(btf, offset);
}
const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id)
{
- if (type_id > btf->nr_types)
- return NULL;
+again:
+ if (type_id < btf->start_id) {
+ btf = btf->base_btf;
+ goto again;
+ }
+ type_id -= btf->start_id;
+ if (type_id >= btf->nr_types)
+ return NULL;
return btf->types[type_id];
}
@@ -1391,17 +1435,13 @@ static int btf_add_type(struct btf_verifier_env *env,
struct btf_type *t)
