linux-kernel - Re: [PATCH bpf-next v2 01/18] bpf: add function hash table for tracing-multi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAADnVQLz7KwQVhpmPQ2X345UvpdHR+rj4NApi2EBO5kXO4_Mqw@mail.gmail.com>
Date: Tue, 15 Jul 2025 09:22:28 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Menglong Dong <menglong8.dong@...il.com>
Cc: Steven Rostedt <rostedt@...dmis.org>, Jiri Olsa <jolsa@...nel.org>, bpf <bpf@...r.kernel.org>, 
	Menglong Dong <dongml2@...natelecom.cn>, Martin KaFai Lau <martin.lau@...ux.dev>, 
	Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2 01/18] bpf: add function hash table for tracing-multi

On Tue, Jul 15, 2025 at 2:07 AM Menglong Dong <menglong8.dong@...il.com> wrote:
>
> On Tue, Jul 15, 2025 at 11:13 AM Menglong Dong <menglong8.dong@...il.com> wrote:
> >
> > On Tue, Jul 15, 2025 at 10:49 AM Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > > On Mon, Jul 14, 2025 at 7:38 PM Menglong Dong <menglong8.dong@...il.com> wrote:
> [......]
> > >
> > > That doesn't sound right.
> > > When everything is always_inline the compiler can inline the callback hashfn.
> > > Without always inline do use see ht->p.hashfn in the assembly?
> > > If so, the compiler is taking this path:
> > >         if (!__builtin_constant_p(params.key_len))
> > >                 hash = ht->p.hashfn(key, ht->key_len, hash_rnd);
> > >
> > > which is back to const params.
> >
> > I think the compiler thinks the bpf_global_caller is complex enough and
> > refuses to inline it for me, and a call to __rhashtable_lookup() happens.
> > When I add always_inline to __rhashtable_lookup(), the compiler makes
> > a call to rht_key_get_hash(), which is annoying. And I'm sure the params.key_len
> > is const, and the function call is not for the ht->p.hashfn.
> >
> > >
> > > > In fact, I think rhashtable is not good enough in our case, which
> > > > has high performance requirements. With rhashtable, the insn count
> > > > is 35 to finish the hash lookup. With the hash table here, it needs only
> > > > 17 insn, which means the rhashtable introduces ~5% overhead.
> > >
> > > I feel you're not using rhashtable correctly.
> > > Try disasm of xdp_unreg_mem_model().
> > > The inlined lookup is quite small.
> >
> > Okay, I'll disasm it and have a look. In my case, it does consume 35 insn
> > after I disasm it.
>
> You might not believe it when I say this, the rhashtable lookup in my
> kernel is not inlined in xdp_unreg_mem_model(), and following is the
> disasm:
>
> disassemble xdp_unreg_mem_model
> Dump of assembler code for function xdp_unreg_mem_model:
>    0xffffffff81e68760 <+0>:     call   0xffffffff8127f9d0 <__fentry__>
>    0xffffffff81e68765 <+5>:     push   %rbx
>    0xffffffff81e68766 <+6>:     sub    $0x10,%rsp
>    [......]
>
>    /* we can see that the function call to __rhashtable_lookup happens
> in this line.  */
>    0xffffffff81e687ba <+90>:    call   0xffffffff81e686c0 <__rhashtable_lookup>
>    0xffffffff81e687bf <+95>:    test   %rax,%rax
>    0xffffffff81e687c2 <+98>:    je     0xffffffff81e687cb
> <xdp_unreg_mem_model+107>
>    [......]
>
> The gcc that I'm using is:
> gcc --version
> gcc (Debian 12.2.0-14+deb12u1) 12.2.0
>
> I think there may be something wrong with the rhashtable, which needs some
> fixing?

Try multiple compilers.
gcc 12 is quite old.
Making software design decisions based on one specific compiler is just wrong.