On Thu, Dec 1, 2022 at 6:08 AM Larysa Zaremba [off-list ref] wrote:

On Wed, Nov 30, 2022 at 12:17:39PM -0800, Stanislav Fomichev wrote:

quoted

On Wed, Nov 30, 2022 at 11:06 AM Stanislav Fomichev [off-list ref] wrote:

quoted

On Wed, Nov 30, 2022 at 9:38 AM Larysa Zaremba [off-list ref] wrote:

quoted

On Mon, Nov 21, 2022 at 10:25:46AM -0800, Stanislav Fomichev wrote:

quoted

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9528a066cfa5..315876fa9d30 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -15171,6 +15171,25 @@ static int fixup_call_args(struct bpf_verifier_env *env)
      return err;
 }

+static int fixup_xdp_kfunc_call(struct bpf_verifier_env *env, u32 func_id)
+{
+     struct bpf_prog_aux *aux = env->prog->aux;
+     void *resolved = NULL;

First I would like to say I really like the kfunc hints impementation.

I am currently trying to test possible performace benefits of the unrolled
version in the ice driver. I was working on top of the RFC v2,
when I noticed a problem that also persists in this newer version.

For debugging purposes, I have put the following logs in this place in code.

printk(KERN_ERR "func_id=%u\n", func_id);
printk(KERN_ERR "XDP_METADATA_KFUNC_RX_TIMESTAMP_SUPPORTED=%u\n",
       xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_TIMESTAMP_SUPPORTED));
printk(KERN_ERR "XDP_METADATA_KFUNC_RX_TIMESTAMP=%u\n",
       xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_TIMESTAMP));
printk(KERN_ERR "XDP_METADATA_KFUNC_RX_HASH_SUPPORTED=%u\n",
       xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_HASH_SUPPORTED));
printk(KERN_ERR "XDP_METADATA_KFUNC_RX_HASH=%u\n",
       xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_HASH));

Loading the program, which uses bpf_xdp_metadata_rx_timestamp_supported()
and bpf_xdp_metadata_rx_timestamp(), has resulted in such messages:

[  412.611888] func_id=108131
[  412.611891] XDP_METADATA_KFUNC_RX_TIMESTAMP_SUPPORTED=108126
[  412.611892] XDP_METADATA_KFUNC_RX_TIMESTAMP=108128
[  412.611892] XDP_METADATA_KFUNC_RX_HASH_SUPPORTED=108130
[  412.611893] XDP_METADATA_KFUNC_RX_HASH=108131
[  412.611894] func_id=108130
[  412.611894] XDP_METADATA_KFUNC_RX_TIMESTAMP_SUPPORTED=108126
[  412.611895] XDP_METADATA_KFUNC_RX_TIMESTAMP=108128
[  412.611895] XDP_METADATA_KFUNC_RX_HASH_SUPPORTED=108130
[  412.611895] XDP_METADATA_KFUNC_RX_HASH=108131

As you can see, I've got 108131 and 108130 IDs in program,
while 108126 and 108128 would be more reasonable.
It's hard to proceed with the implementation, when IDs cannot be sustainably
compared.

Thanks for the report!
Toke has reported a similar issue in [0], have you tried his patch?
I've also tried to address it in v3 [1], could you retry on top of it?
I'll try to insert your printk in my local build to see what happens
with btf ids on my side. Will get back to you..

0: https://lore.kernel.org/bpf/87mt8e2a69.fsf@toke.dk/ (local)
1: https://lore.kernel.org/bpf/20221129193452.3448944-3-sdf@google.com/T/#u (local)

Nope, even if I go back to v2, I still can't reproduce locally.
Somehow in my setup they are sorted properly :-/
Would appreciate it if you can test the v3 patch and confirm whether
it's fixed on your side or not.

I've tested v3 and it looks like the isssue was resolved.
Thanks a lot!

Great, thank you for verifying!

quoted

quoted

quoted

Furthermore, dumped vmlinux BTF shows the IDs is in the exactly reversed
order:

[108126] FUNC 'bpf_xdp_metadata_rx_hash' type_id=108125 linkage=static
[108128] FUNC 'bpf_xdp_metadata_rx_hash_supported' type_id=108127 linkage=static
[108130] FUNC 'bpf_xdp_metadata_rx_timestamp' type_id=108129 linkage=static
[108131] FUNC 'bpf_xdp_metadata_rx_timestamp_supported' type_id=108127 linkage=static

quoted

+
+     if (func_id == xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_TIMESTAMP_SUPPORTED))
+             resolved = aux->xdp_netdev->netdev_ops->ndo_xdp_rx_timestamp_supported;
+     else if (func_id == xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_TIMESTAMP))
+             resolved = aux->xdp_netdev->netdev_ops->ndo_xdp_rx_timestamp;
+     else if (func_id == xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_HASH_SUPPORTED))
+             resolved = aux->xdp_netdev->netdev_ops->ndo_xdp_rx_hash_supported;
+     else if (func_id == xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_HASH))
+             resolved = aux->xdp_netdev->netdev_ops->ndo_xdp_rx_hash;
+
+     if (resolved)
+             return BPF_CALL_IMM(resolved);
+     return 0;
+}
+

My working tree (based on this version) is available on github [0]. Situation
is also described in the last commit message.
I would be great, if you could check, whether this behaviour can be reproduced
on your setup.

[0] https://github.com/walking-machine/linux/tree/hints-v2