Date:   Thu, 16 Mar 2023 11:45:44 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Florent Revest <revest@chromium.org>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org,
        catalin.marinas@arm.com, will@kernel.org, mhiramat@kernel.org,
        mark.rutland@arm.com, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, kpsingh@kernel.org, jolsa@kernel.org,
        xukuohai@huaweicloud.com, lihuafei1@huawei.com
Subject: Re: [PATCH v2 04/10] ftrace: Store direct called addresses in their
 ops
Message-ID: <20230316114544.5db09039@gandalf.local.home>
In-Reply-To: <CABRcYmLLbRGZXWwEpyLW1YFz87tTPA8pCL7oLd4K6Hp9Etr5LA@mail.gmail.com>
References: <20230207182135.2671106-1-revest@chromium.org>
        <20230207182135.2671106-5-revest@chromium.org>
        <20230315194334.58eb56ab@gandalf.local.home>
        <CABRcYmLLbRGZXWwEpyLW1YFz87tTPA8pCL7oLd4K6Hp9Etr5LA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 16 Mar 2023 16:40:48 +0100
Florent Revest <revest@chromium.org> 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@FVFF77S0Q05N.cambridge.arm.com/#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. ;-)

-- Steve