From: Nicholas Piggin <npiggin@gmail.com>
To: linuxppc-dev@lists.ozlabs.org,
Michael Ellerman <mpe@ellerman.id.au>,
"Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
Subject: Re: [RFC PATCH] powerpc/64/ftrace: mprofile-kernel patch out mflr
Date: Fri, 17 May 2019 19:12:45 +1000 [thread overview]
Message-ID: <1558084179.fz93ts375u.astroid@bobo.none> (raw)
In-Reply-To: <1557989161.cjlaryiij4.naveen@linux.ibm.com>
Naveen N. Rao's on May 17, 2019 4:22 am:
> Nicholas Piggin wrote:
>> Naveen N. Rao's on May 14, 2019 6:32 pm:
>>> Michael Ellerman wrote:
>>>> "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> writes:
>>>>> Michael Ellerman wrote:
>>>>>> Nicholas Piggin <npiggin@gmail.com> writes:
>>>>>>> The new mprofile-kernel mcount sequence is
>>>>>>>
>>>>>>> mflr r0
>>>>>>> bl _mcount
>>>>>>>
>>>>>>> Dynamic ftrace patches the branch instruction with a noop, but leaves
>>>>>>> the mflr. mflr is executed by the branch unit that can only execute one
>>>>>>> per cycle on POWER9 and shared with branches, so it would be nice to
>>>>>>> avoid it where possible.
>>>>>>>
>>>>>>> This patch is a hacky proof of concept to nop out the mflr. Can we do
>>>>>>> this or are there races or other issues with it?
>>>>>>
>>>>>> There's a race, isn't there?
>>>>>>
>>>>>> We have a function foo which currently has tracing disabled, so the mflr
>>>>>> and bl are nop'ed out.
>>>>>>
>>>>>> CPU 0 CPU 1
>>>>>> ==================================
>>>>>> bl foo
>>>>>> nop (ie. not mflr)
>>>>>> -> interrupt
>>>>>> something else enable tracing for foo
>>>>>> ... patch mflr and branch
>>>>>> <- rfi
>>>>>> bl _mcount
>>>>>>
>>>>>> So we end up in _mcount() but with r0 not populated.
>>>>>
>>>>> Good catch! Looks like we need to patch the mflr with a "b +8" similar
>>>>> to what we do in __ftrace_make_nop().
>>>>
>>>> Would that actually make it any faster though? Nick?
>>>
>>> Ok, how about doing this as a 2-step process?
>>> 1. patch 'mflr r0' with a 'b +8'
>>> synchronize_rcu_tasks()
>>> 2. convert 'b +8' to a 'nop'
>>
>> Good idea. Well the mflr r0 is harmless, so you can leave that in.
>> You just need to ensure it's not removed before the bl is. So nop
>> the bl _mcount, then synchronize_rcu_tasks(), then nop the mflr?
>
> The problem actually seems to be when we try to patch in the branch to
> _mcount(), rather than when we are patching in the nop instructions
> (i.e., the race is when we try to enable the function tracer, rather
> than while disabling it).
>
> When we disable ftrace, we only need to ensure we patch out the branch
> to _mcount() before patching out the preceding 'mflr r0'. I don't think
> we need a synchronize_rcu_tasks() in that case.
That's probably right.
> While enabling ftrace, we will first need to patch the preceding 'mflr
> r0' (which would now be a 'nop') with 'b +8', then use
> synchronize_rcu_tasks() and finally patch in 'bl _mcount()' followed by
> 'mflr r0'.
>
> I think that's what you meant, just that my reference to patching 'mflr
> r0' with a 'b +8' should have called out that the mflr would have been
> nop'ed out.
I meant that we don't need the b +8 anywhere, because the mflr r0
is harmless. Enabling ftrace just needs to patch in 'mflr r0', and
then synchronize_rcu_tasks(), and then patch in 'bl _mcount'. I think?
Thanks,
Nick
next prev parent reply other threads:[~2019-05-17 9:14 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-13 1:59 [RFC PATCH] powerpc/64/ftrace: mprofile-kernel patch out mflr Nicholas Piggin
2019-04-15 9:42 ` Naveen N. Rao
2019-05-13 3:25 ` Michael Ellerman
2019-05-13 6:47 ` Naveen N. Rao
2019-05-13 11:34 ` Michael Ellerman
2019-05-14 8:32 ` Naveen N. Rao
2019-05-15 12:20 ` Michael Ellerman
2019-05-16 5:49 ` Nicholas Piggin
2019-05-16 18:22 ` Naveen N. Rao
2019-05-17 9:12 ` Nicholas Piggin [this message]
2019-05-17 17:25 ` Naveen N. Rao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1558084179.fz93ts375u.astroid@bobo.none \
--to=npiggin@gmail.com \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=naveen.n.rao@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).