Re: [RFC PATCH 10/10] x86/unwind: add undwarf unwinder

[Peter Zijlstra]

On Thu, Jun 01, 2017 at 12:44:16AM -0500, Josh Poimboeuf wrote:

> +static struct undwarf *undwarf_lookup(unsigned long ip)
> +{
> +	struct undwarf *undwarf;
> +	struct module *mod;
> +
> +	/* Look in vmlinux undwarf section: */
> +	undwarf = __undwarf_lookup(__undwarf_start, __undwarf_end - __undwarf_start, ip);
> +	if (undwarf)
> +		return undwarf;
> +
> +	/* Look in module undwarf sections: */
> +	preempt_disable();
> +	mod = __module_address(ip);
> +	if (!mod || !mod->arch.undwarf)
> +		goto module_out;
> +	undwarf = __undwarf_lookup(mod->arch.undwarf, mod->arch.num_undwarves, ip);
> +
> +module_out:
> +	preempt_enable();
> +	return undwarf;
> +}

> +bool unwind_next_frame(struct unwind_state *state)
> +{
> +	struct undwarf *undwarf;
> +	unsigned long cfa;
> +	bool indirect = false;
> +	enum stack_type prev_type = state->stack_info.type;
> +	unsigned long ip_p, prev_sp = state->sp;
> +
> +	if (unwind_done(state))
> +		return false;
> +
> +	/* Have we reached the end? */
> +	if (state->regs && user_mode(state->regs))
> +		goto done;
> +
> +	/* Look up the instruction address in the .undwarf table: */
> +	undwarf = undwarf_lookup(state->ip);
> +	if (!undwarf || undwarf->cfa_reg == UNDWARF_REG_UNDEFINED)
> +		goto done;
> +

	....

> +}
> +EXPORT_SYMBOL_GPL(unwind_next_frame);
> +
> +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> +		    struct pt_regs *regs, unsigned long *first_frame)
> +{

	...

> +	while (!unwind_done(state) &&
> +	       (!on_stack(&state->stack_info, first_frame, sizeof(long)) ||
> +			state->sp <= (unsigned long)first_frame))
> +		unwind_next_frame(state);
> +}

So we do that lookup for every single frame. That's going to hurt.

Would it make sense to cache the last 'module' in an attempt to at least
avoid that lookup again? Something like so:

---
--- a/arch/x86/include/asm/unwind.h
+++ b/arch/x86/include/asm/unwind.h
@@ -15,6 +15,7 @@ struct unwind_state {
 #if defined(CONFIG_UNDWARF_UNWINDER)
 	unsigned long sp, bp, ip;
 	struct pt_regs *regs;
+	struct module *mod;
 #elif defined(CONFIG_FRAME_POINTER)
 	bool got_irq;
 	unsigned long *bp, *orig_sp, ip;
--- a/arch/x86/kernel/unwind_undwarf.c
+++ b/arch/x86/kernel/unwind_undwarf.c
@@ -62,26 +62,45 @@ static struct undwarf *__undwarf_lookup(
 	return NULL;
 }
 
-static struct undwarf *undwarf_lookup(unsigned long ip)
+static struct undwarf *undwarf_lookup(struct unwind_state *state)
 {
+	struct module *mod = state->mod;
+	unsigned long ip = state->ip;
 	struct undwarf *undwarf;
-	struct module *mod;
+	unsigned int num;
 
-	/* Look in vmlinux undwarf section: */
-	undwarf = __undwarf_lookup(__undwarf_start, __undwarf_end - __undwarf_start, ip);
-	if (undwarf)
-		return undwarf;
+	if (mod) {
+		if (within_module(ip, mod)) {
+			undwarf = mod->arch.undwarf;
+			num	= mod->arch.num_undwarves;
+			goto lookup;
+		}
+		mod = NULL;
+	}
+
+	if (core_kernel_text(ip)) {
+		undwarf = __undwarf_start;
+		num	= __undwarf_end - __undwarf_start;
+		goto lookup;
+	}
 
-	/* Look in module undwarf sections: */
+	/*
+	 * Shut up the warning from __module_address(), regardless the undwarf
+	 * pointer can disappear from under us.
+	 */
 	preempt_disable();
 	mod = __module_address(ip);
+	preempt_enable();
+
 	if (!mod || !mod->arch.undwarf)
-		goto module_out;
-	undwarf = __undwarf_lookup(mod->arch.undwarf, mod->arch.num_undwarves, ip);
+		return NULL;
 
-module_out:
-	preempt_enable();
-	return undwarf;
+	undwarf	= mod->arch.undwarf;
+	num	= mod->arch.num_undwarves;
+
+lookup:
+	state->mod = mod;
+	return __undwarf_lookup(undwarf, num, ip);
 }
 
 static bool stack_access_ok(struct unwind_state *state, unsigned long addr,
@@ -168,7 +187,7 @@ bool unwind_next_frame(struct unwind_sta
 		goto done;
 
 	/* Look up the instruction address in the .undwarf table: */
-	undwarf = undwarf_lookup(state->ip);
+	undwarf = undwarf_lookup(state);
 	if (!undwarf || undwarf->cfa_reg == UNDWARF_REG_UNDEFINED)
 		goto done;