[PATCH] Remove mfinfo[64] used by get_wchan()

[PATCH] Remove mfinfo[64] used by get_wchan()

[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 results in a smaller bss and a better code:

   text    data     bss     dec     hex filename
2543808  254148  139296 2937252  2cd1a4 vmlinux-new-get-wchan
2544080  254148  143392 2941620  2ce2b4 vmlinux~old

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/process.c |  132 +++++++++++++++++---------------------------
 1 files changed, 50 insertions(+), 82 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 951bf9c..6d050fa 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -273,13 +273,15 @@ #endif
 	return do_fork(flags | CLONE_VM | CLONE_UNTRACED, 0, &regs, 0, NULL, NULL);
 }
 
-static struct mips_frame_info {
-	void *func;
-	unsigned long func_size;
-	int frame_size;
-	int pc_offset;
-} *schedule_frame, mfinfo[64];
-static int mfinfo_num;
+/*
+ *
+ */
+struct mips_frame_info {
+	void		*func;
+	unsigned long	func_size;
+	int		frame_size;
+	int		pc_offset;
+};
 
 static inline int is_ra_save_ins(union mips_instruction *ip)
 {
@@ -340,45 +342,31 @@ static int get_frame_info(struct mips_fr
 	return -1;
 }
 
+static struct mips_frame_info schedule_mfi __read_mostly;
+
 static int __init frame_info_init(void)
 {
-	int i;
+	unsigned long size = 0;
+
 #ifdef CONFIG_KALLSYMS
+	unsigned long ofs;
 	char *modname;
 	char namebuf[KSYM_NAME_LEN + 1];
-	unsigned long start, size, ofs;
-	extern char __sched_text_start[], __sched_text_end[];
-	extern char __lock_text_start[], __lock_text_end[];
-
-	start = (unsigned long)__sched_text_start;
-	for (i = 0; i < ARRAY_SIZE(mfinfo); i++) {
-		if (start == (unsigned long)schedule)
-			schedule_frame = &mfinfo[i];
-		if (!kallsyms_lookup(start, &size, &ofs, &modname, namebuf))
-			break;
-		mfinfo[i].func = (void *)(start + ofs);
-		mfinfo[i].func_size = size;
-		start += size - ofs;
-		if (start >= (unsigned long)__lock_text_end)
-			break;
-		if (start == (unsigned long)__sched_text_end)
-			start = (unsigned long)__lock_text_start;
-	}
-#else
-	mfinfo[0].func = schedule;
-	schedule_frame = &mfinfo[0];
+
+	kallsyms_lookup((unsigned long)schedule, &size, &ofs, &modname, namebuf);
 #endif
-	for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++)
-		get_frame_info(mfinfo + i);
+	schedule_mfi.func = schedule;
+	schedule_mfi.func_size = size;
+
+	get_frame_info(&schedule_mfi);
 
 	/*
 	 * Without schedule() frame info, result given by
 	 * thread_saved_pc() and get_wchan() are not reliable.
 	 */
-	if (schedule_frame->pc_offset < 0)
+	if (schedule_mfi.pc_offset < 0)
 		printk("Can't analyze schedule() prologue at %p\n", schedule);
 
-	mfinfo_num = i;
 	return 0;
 }
 
@@ -394,58 +382,11 @@ unsigned long thread_saved_pc(struct tas
 	/* New born processes are a special case */
 	if (t->reg31 == (unsigned long) ret_from_fork)
 		return t->reg31;
-
-	if (!schedule_frame || schedule_frame->pc_offset < 0)
+	if (schedule_mfi.pc_offset < 0)
 		return 0;
-	return ((unsigned long *)t->reg29)[schedule_frame->pc_offset];
+	return ((unsigned long *)t->reg29)[schedule_mfi.pc_offset];
 }
 
-/* get_wchan - a maintenance nightmare^W^Wpain in the ass ...  */
-unsigned long get_wchan(struct task_struct *p)
-{
-	unsigned long stack_page;
-	unsigned long pc;
-#ifdef CONFIG_KALLSYMS
-	unsigned long frame;
-#endif
-
-	if (!p || p == current || p->state == TASK_RUNNING)
-		return 0;
-
-	stack_page = (unsigned long)task_stack_page(p);
-	if (!stack_page || !mfinfo_num)
-		return 0;
-
-	pc = thread_saved_pc(p);
-#ifdef CONFIG_KALLSYMS
-	if (!in_sched_functions(pc))
-		return pc;
-
-	frame = p->thread.reg29 + schedule_frame->frame_size;
-	do {
-		int i;
-
-		if (frame < stack_page || frame > stack_page + THREAD_SIZE - 32)
-			return 0;
-
-		for (i = mfinfo_num - 1; i >= 0; i--) {
-			if (pc >= (unsigned long) mfinfo[i].func)
-				break;
-		}
-		if (i < 0)
-			break;
-
-		if (mfinfo[i].pc_offset < 0)
-			break;
-		pc = ((unsigned long *)frame)[mfinfo[i].pc_offset];
-		if (!mfinfo[i].frame_size)
-			break;
-		frame += mfinfo[i].frame_size;
-	} while (in_sched_functions(pc));
-#endif
-
-	return pc;
-}
 
 #ifdef CONFIG_KALLSYMS
 /* used by show_backtrace() */
@@ -493,3 +434,30 @@ unsigned long unwind_stack(struct task_s
 	return __kernel_text_address(pc) ? pc : 0;
 }
 #endif
+
+/*
+ * get_wchan - a maintenance nightmare^W^Wpain in the ass ...
+ */
+unsigned long get_wchan(struct task_struct *task)
+{
+	unsigned long stack_page = (unsigned long)task_stack_page(task);
+	unsigned long pc = 0;
+#ifdef CONFIG_KALLSYMS
+	unsigned long sp = task->thread.reg29;
+#endif
+
+	if (!task || task == current || task->state == TASK_RUNNING)
+		goto out;
+	if (!stack_page)
+		goto out;
+
+	pc = thread_saved_pc(task);
+
+#ifdef CONFIG_KALLSYMS
+	while (in_sched_functions(pc))
+		pc = unwind_stack(task, &sp, pc, 0);
+#endif
+
+out:
+	return pc;
+}
-- 
1.4.2.rc4

Re: [PATCH] Remove mfinfo[64] used by get_wchan()

[Atsushi Nemoto]

On Thu, 17 Aug 2006 15:57:28 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> 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.

>  static int __init frame_info_init(void)
>  {
> -	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()...

> +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.

> +	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;

> +#endif
> +
> +	if (!task || task == current || task->state == TASK_RUNNING)
> +		goto out;
> +	if (!stack_page)
> +		goto out;
> +
> +	pc = thread_saved_pc(task);
> +
> +#ifdef CONFIG_KALLSYMS
> +	while (in_sched_functions(pc))
> +		pc = unwind_stack(task, &sp, pc, 0);
> +#endif
> +
> +out:
> +	return pc;
> +}

Thanks for your work.

---
Atsushi Nemoto

Re: [PATCH] Remove mfinfo[64] used by get_wchan()

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Thu, 17 Aug 2006 15:57:28 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> 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.
> 
>>  static int __init frame_info_init(void)
>>  {
>> -	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()...
> 

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

>> +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.
> 
>> +	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;
> 

Absolutely. I'll cook up a new patch and will send it today.

Thanks

		Franck

Re: [PATCH] Remove mfinfo[64] used by get_wchan()

[Atsushi Nemoto]

On Fri, 18 Aug 2006 09:50:57 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> >> +	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 ?

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.

---
Atsushi Nemoto

Re: [PATCH] Remove mfinfo[64] used by get_wchan()

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Fri, 18 Aug 2006 09:50:57 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>>>> +	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 ?
> 
> 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.

If the caller of get_frame_info() set info->func_size = 0, then it doesn't
know the size of the function, and we assume it to 128 instructions.

		Franck

-- >8 --


diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 951bf9c..5b18806 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -311,12 +311,19 @@ static inline int is_sp_move_ins(union m
 static int get_frame_info(struct mips_frame_info *info)
 {
 	union mips_instruction *ip = info->func;
-	int i, max_insns =
-		min(128UL, info->func_size / sizeof(union mips_instruction));
+	unsigned max_insns = info->func_size / sizeof(union mips_instruction);
+	unsigned i;
 
 	info->pc_offset = -1;
 	info->frame_size = 0;
 
+	if (!ip)
+		goto err;
+	
+	if (max_insns == 0)
+		max_insns = 128U;
+	max_insns = min(128U, max_insns);
+
 	for (i = 0; i < max_insns; i++, ip++) {
 
 		if (is_jal_jalr_jr_ins(ip))
@@ -337,6 +344,7 @@ static int get_frame_info(struct mips_fr
 	if (info->pc_offset < 0) /* leaf */
 		return 1;
 	/* prologue seems boggus... */
+err:
 	return -1;
 }

Re: [PATCH] Remove mfinfo[64] used by get_wchan()

[Atsushi Nemoto]

On Fri, 18 Aug 2006 10:50:01 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> 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 */

---
Atsushi Nemoto

Re: [PATCH] Remove mfinfo[64] used by get_wchan()

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Fri, 18 Aug 2006 10:50:01 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> 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 */
> 

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.

		Franck

Re: [PATCH] Remove mfinfo[64] used by get_wchan()

[Atsushi Nemoto]

On Fri, 18 Aug 2006 14:17:29 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> > 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;

Oh I missed it.

> 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.

> 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.

---
Atsushi Nemoto

Re: [PATCH] Remove mfinfo[64] used by get_wchan()

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Fri, 18 Aug 2006 14:17:29 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>>> 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;
> 
> Oh I missed it.
> 
>> 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.
> 
>> 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.

		Franck

Re: [PATCH] Remove mfinfo[64] used by get_wchan()

[Atsushi Nemoto]

On Fri, 18 Aug 2006 16:13:37 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> ok, I'm going to send a new patchset. Thanks for your feedbacks.

Thanks.  The new patchset looks good for me.

Acked-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>