Remove mfinfo[64] used by get_wchan()

This array was used to 'cache' some frame info about scheduler functions to speed up get_wchan(). This array was 1Ko size and was only used when CONFIG_KALLSYMS was set but declared for all configs.

Rather than make the array statement conditional, this patches removes this array and its uses. Indeed the common case doesn't seem to use this array and get_wchan() is not a critical path anyways.

arch/mips/kernel/process.c +7 -0

+#endif
+
+	if (!task || task == current || task->state == TASK_RUNNING)
+		goto out;
+	if (!stack_page)
+		goto out;
+

Quoted 9 lines · Franck Bui-Huu

This array was used to 'cache' some frame info about scheduler functions to speed up get_wchan(). This array was 1Ko size and was only used when CONFIG_KALLSYMS was set but declared for all configs.

Rather than make the array statement conditional, this patches removes this array and its uses. Indeed the common case doesn't seem to use this array and get_wchan() is not a critical path anyways.

It looks good basically, but a few fixes are required.

arch/mips/kernel/process.c static int get_frame_info(struct mips_fr · line 349

Quoted 4 lines

static int __init frame_info_init(void)
{

arch/mips/kernel/process.c +1 -1

-	int i;
+	unsigned long size = 0;

You must pass some non-zero size even if CONFIG_KALLSYMS was not set. Otherwise schedule_mfi will not be initialized as expected. Actually, this is not a problem of this patch, but we missed this point on previous cleanups for the get_frame_info()...

arch/mips/kernel/process.c unsigned long unwind_stack(struct task_s · line 443

Quoted 3 lines

arch/mips/kernel/process.c +3 -0

+unsigned long get_wchan(struct task_struct *task)
+{
+	unsigned long stack_page = (unsigned long)task_stack_page(task);

This should be done after "if (!task ..." check.

arch/mips/kernel/process.c unsigned long unwind_stack(struct task_s · line 446

Quoted 3 lines

arch/mips/kernel/process.c +3 -0

+	unsigned long pc = 0;
+#ifdef CONFIG_KALLSYMS
+	unsigned long sp = task->thread.reg29;

Same. And you missed one stack level.

	sp = task->thread.reg29 + schedule_mfi.frame_size;

Quoted 6 lines

arch/mips/kernel/process.c static int get_frame_info(struct mips_fr · line 349

Quoted 1 line

arch/mips/kernel/process.c +1 -0

+	unsigned long size = 0;

You must pass some non-zero size even if CONFIG_KALLSYMS was not set. Otherwise schedule_mfi will not be initialized as expected. Actually, this is not a problem of this patch, but we missed this point on previous cleanups for the get_frame_info()...

or maybe we can just fix get_frame_info() and make it more robust ?

Quoted 5 lines · Franck Bui-Huu

Quoted 5 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	unsigned long size = 0;

You must pass some non-zero size even if CONFIG_KALLSYMS was not set. Otherwise schedule_mfi will not be initialized as expected. Actually, this is not a problem of this patch, but we missed this point on previous cleanups for the get_frame_info()...

Quoted 1 line

or maybe we can just fix get_frame_info() and make it more robust ?

Maybe. But info->func_size == 0 is valid input when it was called via show_backtrace. If an exception occured on a first instruction of a function, get_frame_info() should return 1. So it would be easy to give some appropriate (128?) size here.

Does something like this seem correct ? If an exception occured on a first instruction of a function, show_backtrace() will call get_frame_info() with info->func_size != 0 but very small. In this case it returns 1.

Quoted 3 lines · Franck Bui-Huu

Does something like this seem correct ? If an exception occured on a first instruction of a function, show_backtrace() will call get_frame_info() with info->func_size != 0 but very small. In this case it returns 1.

Why get_frame_info() will be called with info->func_size != 0 ? The offset of a _first_ instruction is 0, so "ofs" of this line in unwind_stack() will be 0.

info.func_size = ofs; /* analyze from start to ofs */

Quoted 6 lines

Why get_frame_info() will be called with info->func_size != 0 ? The offset of a _first_ instruction is 0, so "ofs" of this line in unwind_stack() will be 0.

info.func_size = ofs; /* analyze from start to ofs */

because in unwind_stack(), before the line you showed, we do:

	if (!kallsyms_lookup(pc, &size, &ofs, &modname, namebuf))
		return 0;
	if (ofs == 0)
		return 0;

Maybe we should do instead:

	if (!kallsyms_lookup(pc, &size, &ofs, &modname, namebuf))
		return 0;
	/* return ra if an exception occured at the first instruction */
	if (ofs == 0)
		return ra;

And in any cases, if we pass info->func_size = 0 to get_frame_info(), then it will consider the function size as unknown.

Quoted 4 lines · Franck Bui-Huu

Quoted 4 lines

Why get_frame_info() will be called with info->func_size != 0 ? The offset of a _first_ instruction is 0, so "ofs" of this line in unwind_stack() will be 0.

Quoted 8 lines

Quoted 2 lines

info.func_size = ofs; /* analyze from start to ofs */

because in unwind_stack(), before the line you showed, we do:

	if (!kallsyms_lookup(pc, &size, &ofs, &modname, namebuf))
		return 0;
	if (ofs == 0)
		return 0;

Oh I missed it.

Quoted 7 lines

Maybe we should do instead:

	if (!kallsyms_lookup(pc, &size, &ofs, &modname, namebuf))
		return 0;
	/* return ra if an exception occured at the first instruction */
	if (ofs == 0)
		return ra;

Sure. I should be a right fix. This part must be fixed anyway.

Quoted 2 lines

And in any cases, if we pass info->func_size = 0 to get_frame_info(), then it will consider the function size as unknown.

I see. You're right.

ok, I'm going to send a new patchset. Thanks for your feedbacks.