Hi Steven, > On Jul 14, 2022, at 7:50 PM, Song Liu <songliubraving@xxxxxx> wrote: > > > >> On Jul 14, 2022, at 7:46 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote: >> >> On Fri, 15 Jul 2022 02:04:33 +0000 >> Song Liu <songliubraving@xxxxxx> wrote: >> >>>> What I'm suggesting is that a DIRECT ops will never set IPMODIFY. >>> >>> Aha, this the point I misunderstood. I thought DIRECT ops would always >>> set IPMODIFY (as it does now). >> >> My fault. I was probably not being clear when I was suggesting that >> DIRECT should *act* like an IPMODIFY, but never explicitly stated that >> it should not set the IPMODIFY flag. >> >> The only reason it does today was to make it easy to act like an >> IPMODIFY (because it set the flag). But I'm now suggesting to get rid >> of that and just make DIRECT act like an IPMDOFIY as there can only be >> one of them on a function, but now we have some cases where DIRECT can >> work with IPMODIFY via the callbacks. > > Thanks for the clarification. I think we are finally on the same page on > this. :) A quick update and ask for feedback/clarification. Based on my understanding, you recommended calling ops_func() from __ftrace_hash_update_ipmodify() and in ops_func() the direct trampoline may make changes to the trampoline. Did I get this right? I am going towards this direction, but hit some issue. Specifically, in __ftrace_hash_update_ipmodify(), ftrace_lock is already locked, so the direct trampoline cannot easily make changes with modify_ftrace_direct_multi(), which locks both direct_mutex and ftrace_mutex. One solution would be have no-lock version of all the functions called by modify_ftrace_direct_multi(), but that's a lot of functions and the code will be pretty ugly. The alternative would be the logic in v2: __ftrace_hash_update_ipmodify() returns -EAGAIN, and we make changes to the direct trampoline in other places: 1) if DIRECT ops attached first, the trampoline is updated in prepare_direct_functions_for_ipmodify(), see 3/5 of v2; 2) if IPMODIFY ops attached first, the trampoline is updated in bpf_trampoline_update(), see "goto again" path in 5/5 of v2. Overall, I think this way is still cleaner. What do you think about this? Thanks, Song
