Date: Thu, 28 Sep 2017 09:22:20 +0200
From: Ingo Molnar <mingo@kernel.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: mingo@redhat.com, x86@kernel.org, Steven Rostedt <rostedt@goodmis.org>,
        linux-kernel@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>,
        Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        "H . Peter Anvin" <hpa@zytor.com>,
        "Paul E . McKenney" <paulmck@linux.vnet.ibm.com>,
        Alexei Starovoitov <ast@kernel.org>, Alexei Starovoitov <ast@fb.com>
Subject: Re: [PATCH -tip v3 7/7] kprobes: Use synchronize_rcu_tasks() for
 optprobe with CONFIG_PREEMPT
Message-ID: <20170928072220.iv5subwl4u4zcqcp@gmail.com>
References: <150581509713.32348.1905525476438163954.stgit@devbox>
 <150581537997.32348.14125457458719053753.stgit@devbox>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <150581537997.32348.14125457458719053753.stgit@devbox>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2620
Lines: 73


* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> To enable jump optimized probe with CONFIG_PREEMPT, use
> synchronize_rcu_tasks() to wait for all tasks preempted
> on trampoline code back on track.

This sentence does not parse. It's missing a verb, but I'm not sure.

> Since the jump optimized kprobes can replace multiple
> instructions, there can be tasks which are preempted
> on the 2nd (or 3rd) instructions. If the kprobe
> replaces those instructions by a jump instruction,
> when those tasks back to the preempted place, it is
> a middle of the jump instruction and causes a kernel
> panic.


Again, sentence appears to be missing a verb and also an adjective I think.

> To avoid such tragedies in advance, kprobe optimizer
> prepare a detour route using normal kprobe (e.g.
> int3 breakpoint on x86), and wait for the tasks which
> is interrrupted on such place by synchronize_sched()
> when CONFIG_PREEMPT=n.

s/tragedies/mishaps

Part after the first comma does not parse.

Also the way to refer to kprobes is "kprobes" and "normal kprobes".
Use 'kprobe' only when talking about a specific kprobe instance or such.
You use this correctly later on in the changelog ...

> If CONFIG_PREEMPT=y, things be more complicated, because

s/be/are or s/be/get

> such interrupted thread can be preempted (other thread
> can be scheduled in interrupt handler.) So, kprobes

full stop in the wrong place.

> optimizer has to wait for those tasks scheduled normally.

missing verb.

> In this case we can use synchronize_rcu_tasks() which
> ensures that all preempted tasks back on track and
> schedule it.

More careful changelogs please.

> +	 * are done. Because optprobe may modify multiple instructions,
> +	 * there is a chance that the Nth instruction is interrupted. In that
> +	 * case, running interrupt can return to the Nth byte of jump
> +	 * instruction. This can be avoided by waiting for returning of
> +	 * such interrupts, since (until here) the first byte of the optimized
> +	 * probe is already replaced with normal kprobe (sw breakpoint) and
> +	 * all threads which reach to the probed address will hit it and
> +	 * bypass the copied instructions (instead of executing the original.)
> +	 * With CONFIG_PREEMPT, such interrupts can be preepmted. To wait
> +	 * for such thread, we will use synchronize_rcu_tasks() which ensures
> +	 * all preeempted tasks are scheduled normally (not preempted).
> +	 * So we can ensure there is no threads preempted at probed address.

What? Interrupts cannot be preempted.

Also, "To wait for such threads", or "To wait for such a thread".

Thanks,

	Ingo