On Thu, Aug 14, 2025 at 11:40 PM Masami Hiramatsu <mhira...@kernel.org> wrote:
>
> On Thu, 31 Jul 2025 17:24:24 +0800
> Menglong Dong <menglong8.d...@gmail.com> wrote:
>
> > For now, all the kernel functions who are hooked by the fprobe will be
> > added to the hash table "fprobe_ip_table". The key of it is the function
> > address, and the value of it is "struct fprobe_hlist_node".
> >
> > The budget of the hash table is FPROBE_IP_TABLE_SIZE, which is 256. And
> > this means the overhead of the hash table lookup will grow linearly if
> > the count of the functions in the fprobe more than 256. When we try to
> > hook all the kernel functions, the overhead will be huge.
> >
> > Therefore, replace the hash table with rhltable to reduce the overhead.
> >
>
> Hi Menglong,
>
> Thanks for update, I have just some nitpicks.
>
> > Signed-off-by: Menglong Dong <dong...@chinatelecom.cn>
> > ---
> > v3:
> > - some format optimization
> > - handle the error that returned from rhltable_insert in
> > insert_fprobe_node
> > ---
> > include/linux/fprobe.h | 3 +-
> > kernel/trace/fprobe.c | 154 +++++++++++++++++++++++------------------
> > 2 files changed, 90 insertions(+), 67 deletions(-)
> >
> > diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> > index 702099f08929..f5d8982392b9 100644
> > --- a/include/linux/fprobe.h
> > +++ b/include/linux/fprobe.h
> > @@ -7,6 +7,7 @@
> > #include <linux/ftrace.h>
> > #include <linux/rcupdate.h>
> > #include <linux/refcount.h>
> > +#include <linux/rhashtable.h>
>
> nit: can you also include this header file in fprobe.c ?
OK!
>
> > #include <linux/slab.h>
> >
[......]
> >
> > mutex_lock(&fprobe_mutex);
> > - for (i = 0; i < FPROBE_IP_TABLE_SIZE; i++)
> > - fprobe_remove_node_in_module(mod, &fprobe_ip_table[i],
> > &alist);
> > + rhashtable_walk_enter(&fprobe_ip_table.ht, &iter);
>
> nit: Use rhltable_walk_enter() instead.
OK!
I'll send the V4 later with these nitpicks fixed.
Thanks!
Menglong Dong
>
> Others looks good to me.
>
> Thank you,
>
> > + do {
> > + rhashtable_walk_start(&iter);
> > +
> > + while ((node = rhashtable_walk_next(&iter)) && !IS_ERR(node))
> > + fprobe_remove_node_in_module(mod, node, &alist);
> > +
> > + rhashtable_walk_stop(&iter);
> > + } while (node == ERR_PTR(-EAGAIN));
> > + rhashtable_walk_exit(&iter);
> >
> > if (alist.index < alist.size && alist.index > 0)
> > ftrace_set_filter_ips(&fprobe_graph_ops.ops,
> > @@ -722,8 +729,16 @@ int register_fprobe_ips(struct fprobe *fp, unsigned
> > long *addrs, int num)
> > ret = fprobe_graph_add_ips(addrs, num);
> > if (!ret) {
> > add_fprobe_hash(fp);
> > - for (i = 0; i < hlist_array->size; i++)
> > - insert_fprobe_node(&hlist_array->array[i]);
> > + for (i = 0; i < hlist_array->size; i++) {
> > + ret = insert_fprobe_node(&hlist_array->array[i]);
> > + if (ret)
> > + break;
> > + }
> > + /* fallback on insert error */
> > + if (ret) {
> > + for (i--; i >= 0; i--)
> > + delete_fprobe_node(&hlist_array->array[i]);
> > + }
> > }
> > mutex_unlock(&fprobe_mutex);
> >
> > @@ -819,3 +834,10 @@ int unregister_fprobe(struct fprobe *fp)
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(unregister_fprobe);
> > +
> > +static int __init fprobe_initcall(void)
> > +{
> > + rhltable_init(&fprobe_ip_table, &fprobe_rht_params);
> > + return 0;
> > +}
> > +late_initcall(fprobe_initcall);
> > --
> > 2.50.1
> >
>
>
> --
> Masami Hiramatsu (Google) <mhira...@kernel.org>
