On Thu, Mar 16, 2023 at 4:45 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote: > > On Thu, 16 Mar 2023 16:40:48 +0100 > Florent Revest <revest@xxxxxxxxxxxx> wrote: > > > > > @@ -5466,6 +5467,7 @@ __modify_ftrace_direct(struct ftrace_ops *ops, unsigned long addr) > > > > entry->direct = addr; > > > > } > > > > } > > > > + WRITE_ONCE(ops->direct_call, addr); > > > > > > I'm curious about the use of WRITE_ONCE(). It should not go outside the > > > mutex barrier. > > > > This WRITE_ONCE was originally suggested by Mark here: > > https://lore.kernel.org/all/Y9vW99htjOphDXqY@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/#t > > > > My understanding is that it's not so much about avoiding re-ordering > > but rather about avoiding store tearing since a ftrace_caller > > trampoline could concurrently read ops->direct_call. Does that make > > sense ? > > Yes, but a comment needs to be added: > > /* Prevent store tearing on some archs */ > WRITE_ONCE(ops->direct_call, addr); > > Or something to that affect. Otherwise I can see it confusing others in the > future. And probably me too, as I'll forget why it was a WRITE_ONCE() by > next month. ;-) Definitely :) I was myself confused after a few weeks of adding it so I'll add a clarifying comment. Thanks!
