Skip to contentSkip to conversation

kallsyms_lookup always requires buffers

RFC In review Franck Bui-Huu 3 participants 5 messages Aug 22, 2006 – Aug 23, 2006 MIPS

lore
Share & tools
lore

Franck Bui-Huu

author
#0 lore raw

RFC 1/1 kallsyms_lookup always requires buffers

Hi,

Some uses of kallsyms_lookup() do not need to find out the name of a symbol and its module's name it belongs. This is specially true in arch specific code, which needs to unwind the stack to show the back trace during opps (mips is an example). In this specific case, we just need to retreive the function's size and the offset of the active intruction inside it.

This simple patch adds a new entry "kallsyms_lookup_gently()". The name actually sucks but I can't figure out a coherent name with the rest of the file. If someone could give me a better idea... This new entry does exactly the same as kallsyms_lookup() but does not require any buffers to store any names. It returns 0 if it fails otherwise 1.

Do you think this can be usefull ?

thanks
Franck

-- >8 --

include/linux/kallsyms.h +11 -0
index 849043c..30f8d8d 100644
@@ -12,6 +12,10 @@ #ifdef CONFIG_KALLSYMS
 /* Lookup the address for a symbol. Returns 0 if not found. */
 unsigned long kallsyms_lookup_name(const char *name);
 
+extern int kallsyms_lookup_gently(unsigned long addr,
+				  unsigned long *symbolsize,
+				  unsigned long *offset);
+
 /* Lookup an address.  modname is set to NULL if it's in the kernel. */
 const char *kallsyms_lookup(unsigned long addr,
 			    unsigned long *symbolsize,

@@ -28,6 +32,13 @@ static inline unsigned long kallsyms_loo
 	return 0;
 }
 
+static inline const int kallsyms_lookup_gently(unsigned long addr,
+					       unsigned long *symbolsize,
+					       unsigned long *offset)
+{
+	return 0;
+}
+
 static inline const char *kallsyms_lookup(unsigned long addr,
 					  unsigned long *symbolsize,
 					  unsigned long *offset,
kernel/kallsyms.c +86 -44

155 lines not on this page · raw message

kernel/module.c +2 -1
index 2a19cd4..0e3e6ab 100644
@@ -2012,7 +2012,8 @@ const char *module_address_lookup(unsign
 	list_for_each_entry(mod, &modules, list) {
 		if (within(addr, mod->module_init, mod->init_size)
 		    || within(addr, mod->module_core, mod->core_size)) {
-			*modname = mod->name;
+			if (modname)
+				*modname = mod->name;
 			return get_ksymbol(mod, addr, size, offset);
 		}
 	}
#1 lore raw

Review

Quoted 3 lines
(quoted diff) +2 -0
+}
+EXPORT_SYMBOL_GPL(kallsyms_lookup_gently);

Hi,

there don't seem to be modular users so please don't export it since that export just eats up useless space. (we have way too many of those already)

(Also I suggest you submit at least one user with your patch but that's another matter)

Greetings,
Arjan van de Ven

author
#2 lore raw

Hi

Quoted 10 lines · Arjan van de Ven

Review

Quoted 2 lines
(quoted diff) +2 -0
+}
+EXPORT_SYMBOL_GPL(kallsyms_lookup_gently);

Hi,

there don't seem to be modular users so please don't export it since that export just eats up useless space. (we have way too many of those already)

true.

Quoted 3 lines

(Also I suggest you submit at least one user with your patch but that's another matter)

mips could be one of the future users, see patch below.

thanks
Franck

-- >8 --

arch/mips/kernel/process.c +2 -6
index 951bf9c..eb80db5 100644
@@ -344,8 +344,6 @@ static int __init frame_info_init(void)
 {
 	int i;
 #ifdef CONFIG_KALLSYMS
-	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[];

@@ -354,7 +352,7 @@ #ifdef CONFIG_KALLSYMS
 	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))
+		if (!kallsyms_lookup_gently(start, &size, &ofs))
 			break;
 		mfinfo[i].func = (void *)(start + ofs);
 		mfinfo[i].func_size = size;

@@ -454,8 +452,6 @@ unsigned long unwind_stack(struct task_s
 {
 	unsigned long stack_page;
 	struct mips_frame_info info;
-	char *modname;
-	char namebuf[KSYM_NAME_LEN + 1];
 	unsigned long size, ofs;
 	int leaf;
 

@@ -463,7 +459,7 @@ unsigned long unwind_stack(struct task_s
 	if (!stack_page)
 		return 0;
 
-	if (!kallsyms_lookup(pc, &size, &ofs, &modname, namebuf))
+	if (!kallsyms_lookup_gently(pc, &size, &ofs))
 		return 0;
 	if (ofs == 0)
 		return 0;
#3 lore raw
In reply to Franck Bui-Huu
Quoted 9 lines · Franck Bui-Huu

Hi,

Some uses of kallsyms_lookup() do not need to find out the name of a symbol and its module's name it belongs. This is specially true in arch specific code, which needs to unwind the stack to show the back trace during opps (mips is an example). In this specific case, we just need to retreive the function's size and the offset of the active intruction inside it.

Quoted 3 lines

This simple patch adds a new entry "kallsyms_lookup_gently()". The name actually sucks but I can't figure out a coherent name with the rest of the file. If someone could give me a better idea...

kallsyms_lookup_size_offset() ?

Granted it is a little bigger, but it tells exactly what it does and there are not that many users that we have to write this name all that often.

Quoted 5 lines

This new entry does exactly the same as kallsyms_lookup() but does not require any buffers to store any names. It returns 0 if it fails otherwise 1.

Do you think this can be usefull ?

You tell me, since you're proposing the change ;)

Quoted 34 lines
include/linux/kallsyms.h +11 -0
index 849043c..30f8d8d 100644
@@ -12,6 +12,10 @@ #ifdef CONFIG_KALLSYMS
 /* Lookup the address for a symbol. Returns 0 if not found. */
 unsigned long kallsyms_lookup_name(const char *name);
 
+extern int kallsyms_lookup_gently(unsigned long addr,
+				  unsigned long *symbolsize,
+				  unsigned long *offset);
+
 /* Lookup an address.  modname is set to NULL if it's in the kernel. */
 const char *kallsyms_lookup(unsigned long addr,
 			    unsigned long *symbolsize,

@@ -28,6 +32,13 @@ static inline unsigned long kallsyms_loo
 	return 0;
 }
 
+static inline const int kallsyms_lookup_gently(unsigned long addr,
+					       unsigned long *symbolsize,
+					       unsigned long *offset)
+{
+	return 0;
+}
+
 static inline const char *kallsyms_lookup(unsigned long addr,
 					  unsigned long *symbolsize,
 					  unsigned long *offset,
kernel/kallsyms.c +0 -0
index ab16a5a..c398753 100644

kernel/kallsyms.c static inline int is_kernel(unsigned lon · line 72

Quoted 4 lines
kernel/kallsyms.c +1 -0
@@ -69,6 +69,15 @@ static inline int is_kernel(unsigned lon
 	return in_gate_area_no_task(addr);
 }
 
+static int is_kernel_addr(unsigned long addr)

Maybe change the name to kallsyms_is_kernel_addr, because this function does more things than what the generic name implies.

Quoted 8 lines
kernel/kallsyms.c +8 -0
+{
+	if (all_var)
+		return is_kernel(addr);
+
+	return is_kernel_text(addr) || is_kernel_inittext(addr) ||
+		is_kernel_extratext(addr);
+}
+

[...]

Quoted 5 lines
kernel/kallsyms.c +5 -0
+/*
+ * Lookup an address but don't bother to find any names.
+ */
+int kallsyms_lookup_gently(unsigned long addr, unsigned long *symbolsize,
+			   unsigned long *offset)

kernel/kallsyms.c unsigned long kallsyms_lookup_name(const · line 237

Quoted 10 lines
kernel/kallsyms.c +10 -0
+{
+	int rv;
+
+	if (is_kernel_addr(addr))
+		rv = !!get_symbol_pos(addr, symbolsize, offset);
+	else
+		rv = !!module_address_lookup(addr, symbolsize, offset, NULL);
+
+	return rv;
+}

<minor nitpick>

Why not just:

Quoted 4 lines

if (is_kernel_addr(addr))

	return !!get_symbol_pos(addr, symbolsize, offset);

return !!module_address_lookup(addr, symbolsize, offset, NULL);

and just get rid of "rv" completely?

</minor nitpick>

Quoted 1 line

+EXPORT_SYMBOL_GPL(kallsyms_lookup_gently);

I agree with Arjan here. If kallsyms_lookup wasn't exported, I don't see a reason to export this either.

Everything else seems fine.

Signature · 4 lines
Paulo Marques - www.grupopie.com

"The face of a child can say it all, especially the
mouth part of the face."
author
#4 lore raw

Hi

Quoted 6 lines · Paulo Marques
Quoted 4 lines · Franck Bui-Huu

This simple patch adds a new entry "kallsyms_lookup_gently()". The name actually sucks but I can't figure out a coherent name with the rest of the file. If someone could give me a better idea...

Quoted 6 lines

kallsyms_lookup_size_offset() ?

Granted it is a little bigger, but it tells exactly what it does and there are not that many users that we have to write this name all that often.

ok.

Quoted 8 lines
Quoted 5 lines

This new entry does exactly the same as kallsyms_lookup() but does not require any buffers to store any names. It returns 0 if it fails otherwise 1.

Do you think this can be usefull ?

You tell me, since you're proposing the change ;)

:)
actually the question was rather "does this change look sensible to you ?"

Quoted 5 lines
Quoted 1 line

+static int is_kernel_addr(unsigned long addr)

Maybe change the name to kallsyms_is_kernel_addr, because this function does more things than what the generic name implies.

it sounds like a public function name. Maybe is_ksym_addr() is better ?

Quoted 20 lines
Quoted 5 lines

+ * Lookup an address but don't bother to find any names.
+ */
+int kallsyms_lookup_gently(unsigned long addr, unsigned long *symbolsize,

kernel/kallsyms.c +1 -0
+               unsigned long *offset)

kernel/kallsyms.c unsigned long kallsyms_lookup_name(const · line 237

Quoted 10 lines
kernel/kallsyms.c +10 -0
+{
+    int rv;
+
+    if (is_kernel_addr(addr))
+        rv = !!get_symbol_pos(addr, symbolsize, offset);
+    else
+        rv = !!module_address_lookup(addr, symbolsize, offset, NULL);
+
+    return rv;
+}

<minor nitpick>

Why not just:

Quoted 9 lines
Quoted 4 lines

if (is_kernel_addr(addr))
return !!get_symbol_pos(addr, symbolsize, offset);

return !!module_address_lookup(addr, symbolsize, offset, NULL);

and just get rid of "rv" completely?

</minor nitpick>

No problem.

Quoted 5 lines
Quoted 1 line

+EXPORT_SYMBOL_GPL(kallsyms_lookup_gently);

I agree with Arjan here. If kallsyms_lookup wasn't exported, I don't see a reason to export this either.

Already removed.

thanks
Franck