From: Nicholas Piggin <npiggin@gmail.com> To: Masami Hiramatsu <mhiramat@kernel.org>, Ingo Molnar <mingo@kernel.org>, Michael Ellerman <mpe@ellerman.id.au>, "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>, Steven Rostedt <rostedt@goodmis.org> Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel Date: Wed, 19 Jun 2019 20:41:41 +1000 [thread overview] Message-ID: <1560939496.ovo51ph4i4.astroid@bobo.none> (raw) In-Reply-To: <1560935530.70niyxru6o.naveen@linux.ibm.com> Naveen N. Rao's on June 19, 2019 7:53 pm: > Nicholas Piggin wrote: >> Michael Ellerman's on June 19, 2019 3:14 pm: >>> Hi Naveen, >>> >>> Sorry I meant to reply to this earlier .. :/ > > No problem. Thanks for the questions. > >>> >>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes: >>>> With -mprofile-kernel, gcc emits 'mflr r0', followed by 'bl _mcount' to >>>> enable function tracing and profiling. So far, with dynamic ftrace, we >>>> used to only patch out the branch to _mcount(). However, 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. >>>> >>>> We cannot simply nop out the mflr either. When enabling function >>>> tracing, there can be a race if tracing is enabled when some thread was >>>> interrupted after executing a nop'ed out mflr. In this case, the thread >>>> would execute the now-patched-in branch to _mcount() without having >>>> executed the preceding mflr. >>>> >>>> To solve this, we now enable function tracing in 2 steps: patch in the >>>> mflr instruction, use synchronize_rcu_tasks() to ensure all existing >>>> threads make progress, and then patch in the branch to _mcount(). We >>>> override ftrace_replace_code() with a powerpc64 variant for this >>>> purpose. >>> >>> According to the ISA we're not allowed to patch mflr at runtime. See the >>> section on "CMODX". >> >> According to "quasi patch class" engineering note, we can patch >> anything with a preferred nop. But that's written as an optional >> facility, which we don't have a feature to test for. >> > > Hmm... I wonder what the implications are. We've been patching in a > 'trap' for kprobes for a long time now, along with having to patch back > the original instruction (which can be anything), when the probe is > removed. Will have to check what implementations support "quasi patch class" instructions. IIRC recent POWER processors are okay. May have to add a feature test though. >>> >>> I'm also not convinced the ordering between the two patches is >>> guaranteed by the ISA, given that there's possibly no isync on the other >>> CPU. >> >> Will they go through a context synchronizing event? >> >> synchronize_rcu_tasks() should ensure a thread is scheduled away, but >> I'm not actually sure it guarantees CSI if it's kernel->kernel. Could >> do a smp_call_function to do the isync on each CPU to be sure. > > Good point. Per > Documentation/RCU/Design/Requirements/Requirements.html#Tasks RCU: > "The solution, in the form of Tasks RCU, is to have implicit read-side > critical sections that are delimited by voluntary context switches, that > is, calls to schedule(), cond_resched(), and synchronize_rcu_tasks(). In > addition, transitions to and from userspace execution also delimit > tasks-RCU read-side critical sections." > > I suppose transitions to/from userspace, as well as calls to schedule() > result in context synchronizing instruction being executed. But, if some > tasks call cond_resched() and synchronize_rcu_tasks(), we probably won't > have a CSI executed. > > Also: > "In CONFIG_PREEMPT=n kernels, trampolines cannot be preempted, so these > APIs map to call_rcu(), synchronize_rcu(), and rcu_barrier(), > respectively." > > In this scenario as well, I think we won't have a CSI executed in case > of cond_resched(). > > Should we enhance patch_instruction() to handle that? Well, not sure. Do we have many post-boot callers of it? Should they take care of their own synchronization requirements? Thanks, Nick
WARNING: multiple messages have this Message-ID (diff)
From: Nicholas Piggin <npiggin@gmail.com> To: Masami Hiramatsu <mhiramat@kernel.org>, Ingo Molnar <mingo@kernel.org>, Michael Ellerman <mpe@ellerman.id.au>, "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>, Steven Rostedt <rostedt@goodmis.org> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel Date: Wed, 19 Jun 2019 20:41:41 +1000 [thread overview] Message-ID: <1560939496.ovo51ph4i4.astroid@bobo.none> (raw) In-Reply-To: <1560935530.70niyxru6o.naveen@linux.ibm.com> Naveen N. Rao's on June 19, 2019 7:53 pm: > Nicholas Piggin wrote: >> Michael Ellerman's on June 19, 2019 3:14 pm: >>> Hi Naveen, >>> >>> Sorry I meant to reply to this earlier .. :/ > > No problem. Thanks for the questions. > >>> >>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes: >>>> With -mprofile-kernel, gcc emits 'mflr r0', followed by 'bl _mcount' to >>>> enable function tracing and profiling. So far, with dynamic ftrace, we >>>> used to only patch out the branch to _mcount(). However, 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. >>>> >>>> We cannot simply nop out the mflr either. When enabling function >>>> tracing, there can be a race if tracing is enabled when some thread was >>>> interrupted after executing a nop'ed out mflr. In this case, the thread >>>> would execute the now-patched-in branch to _mcount() without having >>>> executed the preceding mflr. >>>> >>>> To solve this, we now enable function tracing in 2 steps: patch in the >>>> mflr instruction, use synchronize_rcu_tasks() to ensure all existing >>>> threads make progress, and then patch in the branch to _mcount(). We >>>> override ftrace_replace_code() with a powerpc64 variant for this >>>> purpose. >>> >>> According to the ISA we're not allowed to patch mflr at runtime. See the >>> section on "CMODX". >> >> According to "quasi patch class" engineering note, we can patch >> anything with a preferred nop. But that's written as an optional >> facility, which we don't have a feature to test for. >> > > Hmm... I wonder what the implications are. We've been patching in a > 'trap' for kprobes for a long time now, along with having to patch back > the original instruction (which can be anything), when the probe is > removed. Will have to check what implementations support "quasi patch class" instructions. IIRC recent POWER processors are okay. May have to add a feature test though. >>> >>> I'm also not convinced the ordering between the two patches is >>> guaranteed by the ISA, given that there's possibly no isync on the other >>> CPU. >> >> Will they go through a context synchronizing event? >> >> synchronize_rcu_tasks() should ensure a thread is scheduled away, but >> I'm not actually sure it guarantees CSI if it's kernel->kernel. Could >> do a smp_call_function to do the isync on each CPU to be sure. > > Good point. Per > Documentation/RCU/Design/Requirements/Requirements.html#Tasks RCU: > "The solution, in the form of Tasks RCU, is to have implicit read-side > critical sections that are delimited by voluntary context switches, that > is, calls to schedule(), cond_resched(), and synchronize_rcu_tasks(). In > addition, transitions to and from userspace execution also delimit > tasks-RCU read-side critical sections." > > I suppose transitions to/from userspace, as well as calls to schedule() > result in context synchronizing instruction being executed. But, if some > tasks call cond_resched() and synchronize_rcu_tasks(), we probably won't > have a CSI executed. > > Also: > "In CONFIG_PREEMPT=n kernels, trampolines cannot be preempted, so these > APIs map to call_rcu(), synchronize_rcu(), and rcu_barrier(), > respectively." > > In this scenario as well, I think we won't have a CSI executed in case > of cond_resched(). > > Should we enhance patch_instruction() to handle that? Well, not sure. Do we have many post-boot callers of it? Should they take care of their own synchronization requirements? Thanks, Nick
next prev parent reply other threads:[~2019-06-19 10:47 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-18 14:46 [PATCH 0/7] powerpc/ftrace: Patch out -mprofile-kernel instructions Naveen N. Rao 2019-06-18 14:46 ` Naveen N. Rao 2019-06-18 14:47 ` [PATCH 1/7] ftrace: Expose flags used for ftrace_replace_code() Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-18 14:47 ` [PATCH 2/7] x86/ftrace: Fix use of flags in ftrace_replace_code() Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-18 14:47 ` [PATCH 3/7] ftrace: Expose __ftrace_replace_code() Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-18 14:47 ` [PATCH 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-19 5:14 ` Michael Ellerman 2019-06-19 7:10 ` Nicholas Piggin 2019-06-19 7:10 ` Nicholas Piggin 2019-06-19 9:53 ` Naveen N. Rao 2019-06-19 9:53 ` Naveen N. Rao 2019-06-19 10:41 ` Nicholas Piggin [this message] 2019-06-19 10:41 ` Nicholas Piggin 2019-06-19 17:14 ` Naveen N. Rao 2019-06-19 17:14 ` Naveen N. Rao 2019-06-18 14:47 ` [PATCH 5/7] powerpc/ftrace: Update ftrace_location() for powerpc -mprofile-kernel Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-18 15:45 ` Steven Rostedt 2019-06-18 15:45 ` Steven Rostedt 2019-06-18 18:11 ` Naveen N. Rao 2019-06-18 18:11 ` Naveen N. Rao 2019-06-18 18:23 ` Naveen N. Rao 2019-06-18 18:23 ` Naveen N. Rao 2019-06-18 18:32 ` Steven Rostedt 2019-06-18 18:32 ` Steven Rostedt 2019-06-19 7:56 ` Naveen N. Rao 2019-06-19 7:56 ` Naveen N. Rao 2019-06-19 9:28 ` Steven Rostedt 2019-06-19 9:28 ` Steven Rostedt 2019-06-18 14:47 ` [PATCH 6/7] kprobes/ftrace: Use ftrace_location() when [dis]arming probes Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-21 14:41 ` Masami Hiramatsu 2019-06-21 14:41 ` Masami Hiramatsu 2019-06-18 14:47 ` [PATCH 7/7] powerpc/kprobes: Allow probing on any ftrace address Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-21 14:50 ` Masami Hiramatsu 2019-06-21 14:50 ` Masami Hiramatsu 2019-06-22 3:49 ` Joe Perches 2019-06-22 3:49 ` Joe Perches 2019-06-26 9:39 ` Naveen N. Rao 2019-06-26 9:39 ` 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=1560939496.ovo51ph4i4.astroid@bobo.none \ --to=npiggin@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mhiramat@kernel.org \ --cc=mingo@kernel.org \ --cc=mpe@ellerman.id.au \ --cc=naveen.n.rao@linux.vnet.ibm.com \ --cc=rostedt@goodmis.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.