On Mon, Nov 28, 2022 at 10:53 AM Stanislav Fomichev [off-list ref] wrote:

 s

On Fri, Nov 25, 2022 at 9:53 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Stanislav Fomichev [off-list ref] writes:

quoted

There is an ndo handler per kfunc, the verifier replaces a call to the
generic kfunc with a call to the per-device one.

For XDP, we define a new kfunc set (xdp_metadata_kfunc_ids) which
implements all possible metatada kfuncs. Not all devices have to
implement them. If kfunc is not supported by the target device,
the default implementation is called instead.

BTW, this "the default implementation is called instead" bit is not
included in this version... :)

fixup_xdp_kfunc_call should return 0 when the device doesn't have a
kfunc defined and should fallback to the default kfunc implementation,
right?
Or am I missing something?

quoted

[...]

quoted

+#ifdef CONFIG_DEBUG_INFO_BTF
+BTF_SET8_START(xdp_metadata_kfunc_ids)
+#define XDP_METADATA_KFUNC(name, str) BTF_ID_FLAGS(func, str, 0)
+XDP_METADATA_KFUNC_xxx
+#undef XDP_METADATA_KFUNC
+BTF_SET8_END(xdp_metadata_kfunc_ids)
+
+static const struct btf_kfunc_id_set xdp_metadata_kfunc_set = {
+     .owner = THIS_MODULE,
+     .set   = &xdp_metadata_kfunc_ids,
+};
+
+u32 xdp_metadata_kfunc_id(int id)
+{
+     return xdp_metadata_kfunc_ids.pairs[id].id;
+}
+EXPORT_SYMBOL_GPL(xdp_metadata_kfunc_id);

So I was getting some really weird values when testing (always getting a
timestamp value of '1'), and it turns out to be because this way of
looking up the ID doesn't work: The set is always sorted by the BTF ID,
not the order it was defined. Which meant that the mapping code got the
functions mixed up, and would call a different one instead (so the
timestamp value I was getting was really the return value of
rx_hash_enabled()).

I fixed it by building a secondary lookup table as below; feel free to
incorporate that (or if you can come up with a better way, go ahead!).

Interesting, will take a closer look. I took this pattern from
BTF_SOCK_TYPE_xxx, which means that 'sorting by btf-id' is something
BTF_SET8_START specific...
But if it's sorted, probably easier to do a bsearch over this table
than to build another one?

Ah, I see, there is no place to store an index :-( Maybe the following
is easier still?

diff --git a/net/core/xdp.c b/net/core/xdp.c
index e43f7d4ef4cf..8240805bfdb7 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c

@@ -743,9 +743,15 @@ static const struct btf_kfunc_id_set

xdp_metadata_kfunc_set = {
        .set   = &xdp_metadata_kfunc_ids,
 };

+BTF_ID_LIST(xdp_metadata_kfunc_ids_unsorted)
+#define XDP_METADATA_KFUNC(name, str) BTF_ID(func, str)
+XDP_METADATA_KFUNC_xxx
+#undef XDP_METADATA_KFUNC
+
 u32 xdp_metadata_kfunc_id(int id)
 {
-       return xdp_metadata_kfunc_ids.pairs[id].id;
+       /* xdp_metadata_kfunc_ids is sorted and can't be used */
+       return xdp_metadata_kfunc_ids_unsorted[id];
 }
 EXPORT_SYMBOL_GPL(xdp_metadata_kfunc_id);

quoted

-Toke

diff --git a/net/core/xdp.c b/net/core/xdp.c
index e43f7d4ef4cf..dc0a9644dacc 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c

@@ -738,6 +738,15 @@ XDP_METADATA_KFUNC_xxx
 #undef XDP_METADATA_KFUNC
 BTF_SET8_END(xdp_metadata_kfunc_ids)

+static struct xdp_metadata_kfunc_map {
+       const char *fname;
+       u32 btf_id;
+} xdp_metadata_kfunc_lookup_map[MAX_XDP_METADATA_KFUNC] = {
+#define XDP_METADATA_KFUNC(name, str) { .fname = __stringify(str) },
+XDP_METADATA_KFUNC_xxx
+#undef XDP_METADATA_KFUNC
+};
+
 static const struct btf_kfunc_id_set xdp_metadata_kfunc_set = {
        .owner = THIS_MODULE,
        .set   = &xdp_metadata_kfunc_ids,

@@ -745,13 +754,41 @@ static const struct btf_kfunc_id_set xdp_metadata_kfunc_set = {

 u32 xdp_metadata_kfunc_id(int id)
 {
-       return xdp_metadata_kfunc_ids.pairs[id].id;
+       return xdp_metadata_kfunc_lookup_map[id].btf_id;
 }
 EXPORT_SYMBOL_GPL(xdp_metadata_kfunc_id);

 static int __init xdp_metadata_init(void)
 {
-       return register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &xdp_metadata_kfunc_set);
+       const struct btf *btf;
+       int i, j, ret;
+
+       ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &xdp_metadata_kfunc_set);
+       if (ret)
+               return ret;
+
+       btf = bpf_get_btf_vmlinux();
+
+       for (i = 0; i < MAX_XDP_METADATA_KFUNC; i++) {
+               u32 btf_id = xdp_metadata_kfunc_ids.pairs[i].id;
+               const struct btf_type *t;
+               const char *name;
+
+               t = btf_type_by_id(btf, btf_id);
+               if (WARN_ON_ONCE(!t || !t->name_off))
+                       continue;
+
+               name = btf_name_by_offset(btf, t->name_off);
+
+               for (j = 0; j < MAX_XDP_METADATA_KFUNC; j++) {
+                       if (!strcmp(name, xdp_metadata_kfunc_lookup_map[j].fname)) {
+                               xdp_metadata_kfunc_lookup_map[j].btf_id = btf_id;
+                               break;
+                       }
+               }
+       }
+
+       return 0;
 }
 late_initcall(xdp_metadata_init);
 #else /* CONFIG_DEBUG_INFO_BTF */