On Mon, 18 Aug 2025 15:43:44 +0200
Jiri Olsa <olsaj...@gmail.com> wrote:
> On Sun, Aug 17, 2025 at 10:46:02AM +0800, Menglong Dong wrote:
>
> SNIP
>
> > +/* Node insertion and deletion requires the fprobe_mutex */
> > +static int insert_fprobe_node(struct fprobe_hlist_node *node)
> > +{
> > lockdep_assert_held(&fprobe_mutex);
> >
> > - next = find_first_fprobe_node(ip);
> > - if (next) {
> > - hlist_add_before_rcu(&node->hlist, &next->hlist);
> > - return;
> > - }
> > - head = &fprobe_ip_table[hash_ptr((void *)ip, FPROBE_IP_HASH_BITS)];
> > - hlist_add_head_rcu(&node->hlist, head);
> > + return rhltable_insert(&fprobe_ip_table, &node->hlist,
> > fprobe_rht_params);
> > }
> >
> > /* Return true if there are synonims */
> > @@ -92,9 +92,11 @@ static bool delete_fprobe_node(struct fprobe_hlist_node
> > *node)
> > /* Avoid double deleting */
> > if (READ_ONCE(node->fp) != NULL) {
> > WRITE_ONCE(node->fp, NULL);
> > - hlist_del_rcu(&node->hlist);
> > + rhltable_remove(&fprobe_ip_table, &node->hlist,
> > + fprobe_rht_params);
> > }
> > - return !!find_first_fprobe_node(node->addr);
> > + return !!rhltable_lookup(&fprobe_ip_table, &node->addr,
> > + fprobe_rht_params);
>
> I think rhltable_lookup needs rcu lock
Good catch! Hmm, previously we guaranteed that the find_first_fprobe_node()
must be called under rcu read locked or fprobe_mutex locked, so that the
node list should not be changed. But according to the comment of
rhltable_lookup(), we need to lock the rcu_read_lock() around that.
>
> > }
> >
> > /* Check existence of the fprobe */
> > @@ -249,9 +251,10 @@ static inline int __fprobe_kprobe_handler(unsigned
> > long ip, unsigned long parent
> > static int fprobe_entry(struct ftrace_graph_ent *trace, struct fgraph_ops
> > *gops,
> > struct ftrace_regs *fregs)
> > {
> > - struct fprobe_hlist_node *node, *first;
> > unsigned long *fgraph_data = NULL;
> > unsigned long func = trace->func;
> > + struct fprobe_hlist_node *node;
> > + struct rhlist_head *head, *pos;
> > unsigned long ret_ip;
> > int reserved_words;
> > struct fprobe *fp;
> > @@ -260,14 +263,11 @@ static int fprobe_entry(struct ftrace_graph_ent
> > *trace, struct fgraph_ops *gops,
> > if (WARN_ON_ONCE(!fregs))
> > return 0;
> >
> > - first = node = find_first_fprobe_node(func);
> > - if (unlikely(!first))
> > - return 0;
> > -
> > + head = rhltable_lookup(&fprobe_ip_table, &func, fprobe_rht_params);
>
> ditto
So this was pointed in the previous thread. In fprobe_entry(), the
preempt is disabled already. Thus we don't need locking rcu.
Thank you,
>
> jirka
>
>
> > reserved_words = 0;
> > - hlist_for_each_entry_from_rcu(node, hlist) {
> > + rhl_for_each_entry_rcu(node, pos, head, hlist) {
> > if (node->addr != func)
> > - break;
> > + continue;
> > fp = READ_ONCE(node->fp);
> > if (!fp || !fp->exit_handler)
> > continue;
> > @@ -278,13 +278,12 @@ static int fprobe_entry(struct ftrace_graph_ent
> > *trace, struct fgraph_ops *gops,
> > reserved_words +=
> > FPROBE_HEADER_SIZE_IN_LONG +
> > SIZE_IN_LONG(fp->entry_data_size);
> > }
> > - node = first;
> > if (reserved_words) {
> > fgraph_data = fgraph_reserve_data(gops->idx, reserved_words *
> > sizeof(long));
> > if (unlikely(!fgraph_data)) {
> > - hlist_for_each_entry_from_rcu(node, hlist) {
> > + rhl_for_each_entry_rcu(node, pos, head, hlist) {
> > if (node->addr != func)
> > - break;
> > + continue;
> > fp = READ_ONCE(node->fp);
> > if (fp && !fprobe_disabled(fp))
> > fp->nmissed++;
> > @@ -299,12 +298,12 @@ static int fprobe_entry(struct ftrace_graph_ent
> > *trace, struct fgraph_ops *gops,
> > */
> > ret_ip = ftrace_regs_get_return_address(fregs);
> > used = 0;
> > - hlist_for_each_entry_from_rcu(node, hlist) {
> > + rhl_for_each_entry_rcu(node, pos, head, hlist) {
> > int data_size;
> > void *data;
> >
> > if (node->addr != func)
> > - break;
> > + continue;
> > fp = READ_ONCE(node->fp);
> > if (!fp || fprobe_disabled(fp))
> > continue;
> > @@ -448,25 +447,21 @@ static int fprobe_addr_list_add(struct
> > fprobe_addr_list *alist, unsigned long ad
> > return 0;
> > }
> >
> > -static void fprobe_remove_node_in_module(struct module *mod, struct
> > hlist_head *head,
> > - struct fprobe_addr_list *alist)
> > +static void fprobe_remove_node_in_module(struct module *mod, struct
> > fprobe_hlist_node *node,
> > + struct fprobe_addr_list *alist)
> > {
> > - struct fprobe_hlist_node *node;
> > int ret = 0;
> >
> > - hlist_for_each_entry_rcu(node, head, hlist,
> > - lockdep_is_held(&fprobe_mutex)) {
> > - if (!within_module(node->addr, mod))
> > - continue;
> > - if (delete_fprobe_node(node))
> > - continue;
> > - /*
> > - * If failed to update alist, just continue to update hlist.
> > - * Therefore, at list user handler will not hit anymore.
> > - */
> > - if (!ret)
> > - ret = fprobe_addr_list_add(alist, node->addr);
> > - }
> > + if (!within_module(node->addr, mod))
> > + return;
> > + if (delete_fprobe_node(node))
> > + return;
> > + /*
> > + * If failed to update alist, just continue to update hlist.
> > + * Therefore, at list user handler will not hit anymore.
> > + */
> > + if (!ret)
> > + ret = fprobe_addr_list_add(alist, node->addr);
> > }
> >
> > /* Handle module unloading to manage fprobe_ip_table. */
> > @@ -474,8 +469,9 @@ static int fprobe_module_callback(struct notifier_block
> > *nb,
> > unsigned long val, void *data)
> > {
> > struct fprobe_addr_list alist = {.size = FPROBE_IPS_BATCH_INIT};
> > + struct fprobe_hlist_node *node;
> > + struct rhashtable_iter iter;
> > struct module *mod = data;
> > - int i;
> >
> > if (val != MODULE_STATE_GOING)
> > return NOTIFY_DONE;
> > @@ -486,8 +482,16 @@ static int fprobe_module_callback(struct
> > notifier_block *nb,
> > return NOTIFY_DONE;
> >
> > mutex_lock(&fprobe_mutex);
> > - for (i = 0; i < FPROBE_IP_TABLE_SIZE; i++)
> > - fprobe_remove_node_in_module(mod, &fprobe_ip_table[i], &alist);
> > + rhltable_walk_enter(&fprobe_ip_table, &iter);
> > + 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,
> > @@ -727,8 +731,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);
> >
> > @@ -824,3 +836,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>
