Re: [PATCH v2 04/10] ftrace: Store direct called addresses in their ops

[Florent Revest]

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!