On Mon, Aug 18, 2025 at 9:43 PM Jiri Olsa [off-list ref] wrote:

On Sun, Aug 17, 2025 at 10:46:02AM +0800, Menglong Dong wrote:

SNIP

quoted

+/* 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

Hi, this is the write side part, which is protected by fprobe_mutex.
Do we need to use rcu_read_lock() in the write side when accessing
the protected list?

quoted

 }

 /* 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);

The whole function graphic handler is protected by preempt_disable,
which indicates rcu_read_lock. So we don't need to use rcu_read_lock()
here again:

https://lore.kernel.org/bpf/20250816235023.4dabfbc13a46a859de61cf4d@kernel.org/ (local)

Thanks!
Menglong Dong

ditto

jirka

quoted

      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