Re: [PATCH] selftests/bpf: improve test coverage for kfunc call

[Hari Bathini <hbathini@linux.ibm.com>]

Hi Alexei,

Thanks for the review.

On 09/03/26 10:37 pm, Alexei Starovoitov wrote:
> On Tue, Mar 3, 2026 at 5:15 AM Hari Bathini <hbathini@linux.ibm.com> wrote:
>>
>>
>> +SEC("tc")
>> +int kfunc_call_test5(struct __sk_buff *skb)
>> +{
>> +       struct bpf_sock *sk = skb->sk;
>> +       int ret;
>> +       u32 val32;
>> +       u16 val16;
>> +       u8 val8;
>> +
>> +       if (!sk)
>> +               return -1;
>> +
>> +       sk = bpf_sk_fullsock(sk);
>> +       if (!sk)
>> +               return -1;
>> +
>> +       ret = bpf_kfunc_call_test5(0xFF, 0xFFFF, 0xFFFFFFFF);
> 
> maybe add a comment with bpf asm to highlight what this is ?
> 
> Also 0xFFFFffffULL ?
> 8 "F"s in a row is harder on the eyes.
> and ULL to make it explicit ?

True. Will do that.

>> +       if (ret)
>> +               return ret;
>> +
>> +       val32 = bpf_get_prandom_u32();
>> +       val16 = val32 & 0xFFFF;
>> +       val8 = val32 & 0xFF;
>> +       ret = bpf_kfunc_call_test5(val8, val16, val32);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = bpf_kfunc_call_test5(val8 * 0xFF, val16 * 0xFFFF, val32 * 0xFFFFFFFF);
> 
> I'm struggling to decipher it. Pls add a comment with asm to explain.
> I think the last multiplication is still done in 32-bit domain ?
> or not? 0xFFFFFFFF is a 64-bit constant in C. I think...

Sure. Let me add comments to convey the intention of the bpf programs
to avoid ambiguity..

> 
> Also we have 4 ISA versions. test_progs-no_alu32 and test_progs
> compile it differently.
> Maybe let's add another version of this test but fully in asm ?
> Keep the C version too.
> 
>> +       if (ret)
>> +               return ret;
>> +
>> +       return 0;
>> +}
>> +
>>   SEC("tc")
>>   int kfunc_call_test4(struct __sk_buff *skb)
>>   {
>> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
>> index e62c6b78657f..de4897ddcff1 100644
>> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
>> @@ -766,6 +766,33 @@ __bpf_kfunc long noinline bpf_kfunc_call_test4(signed char a, short b, int c, lo
>>          return (long)a + (long)b + (long)c + d;
>>   }
>>
>> +__bpf_kfunc int bpf_kfunc_call_test5(u8 a, u16 b, u32 c)
>> +{
>> +       /* Make val as volatile to avoid compiler optimizations on the below checks */
>> +       volatile long val = a;
> 
> Pls add a comment that this is zero extended in C.
> 
>> +
>> +       /* Check zero-extension */
>> +       if (val != (unsigned long)a)
>> +               return 1;
>> +       /* Check no sign-extension */
>> +       if (val < 0)
>> +               return 2;
>> +
>> +       val = b;
>> +       if (val != (unsigned long)b)
>> +               return 3;
>> +       if (val < 0)
>> +               return 4;
>> +
>> +       val = c;
>> +       if (val != (unsigned long)c)
>> +               return 5;
>> +       if (val < 0)
>> +               return 6;
>> +
>> +       return 0;
>> +}
> 
> Overall this looks very useful.
> I would expand with another test where a,b,c are s8,s16,s32.

Slightly different approach but kfunc_call_test4/bpf_kfunc_call_test4
cover signed arguments already?

> Please resend with [PATCH bpf-next] in the subject, so that CIs
> can pick it up correctly.
My bad. Will add the suffix while sending v2..

- Hari