On Sat, Aug 16, 2025 at 10:50 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> Hi Menglong,
>
> Sorry, one more thing.
>
> > @@ -260,14 +263,12 @@ 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;
> > -
> > + rcu_read_lock();
>
> Actually, we don't need these rcu_read_lock() in this function, because
> the caller function_graph_enter_regs() uses ftrace_test_recursion_trylock()
> which disables preemption. Thus we don't need to do this again here.
Yeah, I understand it now. I wondered about this part
for a long time, as I didn't see any RCU lock in the
fprobe_entry(). Thanks for the explanation ;)
I'll send a V5 now.
>
> > + head = rhltable_lookup(&fprobe_ip_table, &func, fprobe_rht_params);
> > 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,17 +279,19 @@ 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;
> > + rcu_read_unlock();
> > 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) {
> > + rcu_read_lock();
>
> Ditto.
>
> > + 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++;
> > }
> > + rcu_read_unlock();
> > return 0;
> > }
> > }
>
> Thank you,
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
