On Thu, Nov 6, 2025 at 8:43 AM Alexei Starovoitov <[email protected]> wrote: > > On Wed, Nov 5, 2025 at 3:38 PM Eduard Zingerman <[email protected]> wrote: > > > > On Wed, 2025-11-05 at 15:33 -0800, Alexei Starovoitov wrote: > > > On Wed, Nov 5, 2025 at 2:52 PM Eduard Zingerman <[email protected]> wrote: > > > > > > > > On Wed, 2025-11-05 at 14:45 -0800, Alexei Starovoitov wrote: > > > > > On Wed, Nov 5, 2025 at 12:14 PM Hoyeon Lee <[email protected]> > > > > > wrote: > > > > > > > > > > > > The netif_receive_skb BPF program used in snprintf_btf test still > > > > > > uses > > > > > > a custom __strncmp. This is unnecessary as the bpf_strncmp helper is > > > > > > available and provides the same functionality. > > > > > > > > > > > > This commit refactors the test to use the bpf_strncmp helper, > > > > > > removing > > > > > > the redundant custom implementation. > > > > > > > > > > > > Signed-off-by: Hoyeon Lee <[email protected]> > > > > > > --- > > > > > > .../selftests/bpf/progs/netif_receive_skb.c | 15 > > > > > > +-------------- > > > > > > 1 file changed, 1 insertion(+), 14 deletions(-) > > > > > > > > > > > > diff --git a/tools/testing/selftests/bpf/progs/netif_receive_skb.c > > > > > > b/tools/testing/selftests/bpf/progs/netif_receive_skb.c > > > > > > index 9e067dcbf607..186b8c82b9e6 100644 > > > > > > --- a/tools/testing/selftests/bpf/progs/netif_receive_skb.c > > > > > > +++ b/tools/testing/selftests/bpf/progs/netif_receive_skb.c > > > > > > @@ -31,19 +31,6 @@ struct { > > > > > > __type(value, char[STRSIZE]); > > > > > > } strdata SEC(".maps"); > > > > > > > > > > > > -static int __strncmp(const void *m1, const void *m2, size_t len) > > > > > > -{ > > > > > > - const unsigned char *s1 = m1; > > > > > > - const unsigned char *s2 = m2; > > > > > > - int i, delta = 0; > > > > > > - > > > > > > - for (i = 0; i < len; i++) { > > > > > > - delta = s1[i] - s2[i]; > > > > > > - if (delta || s1[i] == 0 || s2[i] == 0) > > > > > > - break; > > > > > > - } > > > > > > - return delta; > > > > > > -} > > > > > > > > > > > > #if __has_builtin(__builtin_btf_type_id) > > > > > > #define TEST_BTF(_str, _type, _flags, _expected, ...) > > > > > > \ > > > > > > @@ -69,7 +56,7 @@ static int __strncmp(const void *m1, const void > > > > > > *m2, size_t len) > > > > > > &_ptr, sizeof(_ptr), > > > > > > _hflags); \ > > > > > > if (ret) > > > > > > \ > > > > > > break; > > > > > > \ > > > > > > - _cmp = __strncmp(_str, _expectedval, > > > > > > EXPECTED_STRSIZE); \ > > > > > > + _cmp = bpf_strncmp(_str, EXPECTED_STRSIZE, > > > > > > _expectedval); \ > > > > > > > > > > Though it's equivalent, the point of the test is to be heavy > > > > > for the verifier with open coded __strncmp(). > > > > > > > > > > pw-bot: cr > > > > > > > > I double checked that before acking, the test was added as a part of > > > > [1]. > > > > So it seems to be focused on bpf_snprintf_btf(), not on scalability. > > > > And it's not that heavy in terms of instructions budget: > > > > > > > > File Program Verdict Insns States > > > > ----------------------- ----------------------- ------- ----- ------ > > > > netif_receive_skb.bpf.o trace_netif_receive_skb success 18152 629 > > > > > > Is this before or after? > > > What is the % decrease in insn_processed? > > > I'd like to better understand the impact of the change. > > > > That's before, after the change it is as follows: > > > > File Program Verdict Insns States > > ----------------------- ----------------------- ------- ----- ------ > > netif_receive_skb.bpf.o trace_netif_receive_skb success 4353 235 > > ----------------------- ----------------------- ------- ----- ------ > > > > So, the overall impact is 18K -> 4K instructions processed. > > It's large enough impact for the verifier. > I agree that the test was mainly focusing on testing > bpf_snprintf_btf(), but it has a nice side effect by testing > bounded loops too. > I prefer to keep it as-is.
Thanks for the clarification. Removing the open-coded __strncmp would drop the bounded-loop coverage that this test currently provides (as a side effect), and that stress on the verifier is still valuable. I'll drop this patch. Thank you all for the discussion and review.
