Re: very poor ext3 write performance on big filesystems?
Mark Lord wrote: Theodore Tso wrote: .. The following ld_preload can help in some cases. Mutt has this hack encoded in for maildir directories, which helps. .. Oddly enough, that same spd_readdir() preload craps out here too when used with "rm -r" on largish directories. From looking at the code, I think I've found at least one bug in opendir: ... dnew = realloc(dirstruct->dp, dirstruct->max * sizeof(struct dir_s)); ... Shouldn't this be: "...*sizeof(struct dirent_s));"? -- Paulo Marques - www.grupopie.com "Nostalgia isn't what it used to be." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: very poor ext3 write performance on big filesystems?
Mark Lord wrote: Theodore Tso wrote: .. The following ld_preload can help in some cases. Mutt has this hack encoded in for maildir directories, which helps. .. Oddly enough, that same spd_readdir() preload craps out here too when used with rm -r on largish directories. From looking at the code, I think I've found at least one bug in opendir: ... dnew = realloc(dirstruct-dp, dirstruct-max * sizeof(struct dir_s)); ... Shouldn't this be: ...*sizeof(struct dirent_s));? -- Paulo Marques - www.grupopie.com Nostalgia isn't what it used to be. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Disk schedulers
Lukas Hejtmanek wrote: [...] If I'm scping file (about 500MB) from the network (which is faster than the local disk), any process is totally unable to read anything from the local disk till the scp finishes. It is not caused by low free memory, while scping I have 500MB of free memory (not cached or buffered). If you want to take advantage of all that memory to buffer disk writes, so that the reads can proceed better, you might want to tweak your /proc/sys/vm/dirty_ratio amd /proc/sys/vm/dirty_background_ratio to more appropriate values. (maybe also dirty_writeback_centisecs and dirty_expire_centisecs) You can read all about those tunables in Documentation/filesystems/proc.txt. Just my 2 cents, -- Paulo Marques - www.grupopie.com "Very funny Scotty. Now beam up my clothes." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Disk schedulers
Lukas Hejtmanek wrote: [...] If I'm scping file (about 500MB) from the network (which is faster than the local disk), any process is totally unable to read anything from the local disk till the scp finishes. It is not caused by low free memory, while scping I have 500MB of free memory (not cached or buffered). If you want to take advantage of all that memory to buffer disk writes, so that the reads can proceed better, you might want to tweak your /proc/sys/vm/dirty_ratio amd /proc/sys/vm/dirty_background_ratio to more appropriate values. (maybe also dirty_writeback_centisecs and dirty_expire_centisecs) You can read all about those tunables in Documentation/filesystems/proc.txt. Just my 2 cents, -- Paulo Marques - www.grupopie.com Very funny Scotty. Now beam up my clothes. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: managing kallsyms_addresses
Robin Getz wrote: When the kernel needs to find out what symbol is at a specific address, it uses kallsyms_lookup() This seems to work pretty well - almost. The problem is today, we don't to remove the symbols from the init section when the init section is freed. There is invalid data in kallsyms_addresses. [...] There would be two solutions: - when freeing the init section, remove all the init labels from the kallsyms_addresses, and resort/pack it. This should be doable, but to be worth it, we would need to use different structures for the init symbols, that would be stored in __initdata. Then, ideally we would have separate functions in the __init section to look up symbols in those structures that would only be called until we released the init sections. On the plus side, this would avoid the "repacking" the kallsyms structures to remove the init labels. - if the init section is unloaded, have is_kernel_inittext always return 0. This is by far the simplest solution. I even STR a patch floating around to do this, by I can't seem to locate it now... :( I assume that similar things need to be handled for module init too, but I have not run into that yet. It seems that at least the last solution should be easy enough to implement there. Thoughts? I think that the simplest solution (the second one) is probably the best for now. One thing that did cross my mind though, is stuff like lockdep. If we run a locking sequence that is called from init code, and later a different locking sequence is used when we already freed init data, how can the debug information show the names of the functions that generated the previous locking sequence? It seems that the only "correct" approach would be to store a "before freeing init sections" flag together with the function pointer, and then have a kallsyms interface that could receive this flag to know where to look. In that case, the first solution can not be used at all (because we need to keep the init symbols anyway) and the second solution could be simply implemented by having a default value for the flag that is the "current state" for that flag... -- Paulo Marques - www.grupopie.com "There cannot be a crisis today; my schedule is already full." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: managing kallsyms_addresses
Robin Getz wrote: When the kernel needs to find out what symbol is at a specific address, it uses kallsyms_lookup() This seems to work pretty well - almost. The problem is today, we don't to remove the symbols from the init section when the init section is freed. There is invalid data in kallsyms_addresses. [...] There would be two solutions: - when freeing the init section, remove all the init labels from the kallsyms_addresses, and resort/pack it. This should be doable, but to be worth it, we would need to use different structures for the init symbols, that would be stored in __initdata. Then, ideally we would have separate functions in the __init section to look up symbols in those structures that would only be called until we released the init sections. On the plus side, this would avoid the repacking the kallsyms structures to remove the init labels. - if the init section is unloaded, have is_kernel_inittext always return 0. This is by far the simplest solution. I even STR a patch floating around to do this, by I can't seem to locate it now... :( I assume that similar things need to be handled for module init too, but I have not run into that yet. It seems that at least the last solution should be easy enough to implement there. Thoughts? I think that the simplest solution (the second one) is probably the best for now. One thing that did cross my mind though, is stuff like lockdep. If we run a locking sequence that is called from init code, and later a different locking sequence is used when we already freed init data, how can the debug information show the names of the functions that generated the previous locking sequence? It seems that the only correct approach would be to store a before freeing init sections flag together with the function pointer, and then have a kallsyms interface that could receive this flag to know where to look. In that case, the first solution can not be used at all (because we need to keep the init symbols anyway) and the second solution could be simply implemented by having a default value for the flag that is the current state for that flag... -- Paulo Marques - www.grupopie.com There cannot be a crisis today; my schedule is already full. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] [Kallsyms] Blackfin: Allow kernel symbols in L1 to be found
Bryan Wu wrote: From: Robin Getz <[EMAIL PROTECTED]> [...] --- a/scripts/kallsyms.c +++ b/scripts/kallsyms.c @@ -12,6 +12,8 @@ * (25/Aug/2004) Paulo Marques <[EMAIL PROTECTED]> * Changed the compression method from stem compression to "table lookup" * compression + * (10/Jul/2006) Robin Getz <[EMAIL PROTECTED]> + * Add _stext_l1, _etext_l1 for the L1 memory section in Blackfin. When I wrote this initially, it was a mistake to add a Changelog in the first place, but I didn't know better at the time. If you're going to make changes to this file, please remove all the Changelog, instead of adding more entries to it. The "Changelog" should be kept by the version control system, and not the source code itself. The rest of the patch looks ok, though. -- Paulo Marques - www.grupopie.com "Very funny Scotty. Now beam up my clothes." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] [Kallsyms] Blackfin: Allow kernel symbols in L1 to be found
Bryan Wu wrote: From: Robin Getz [EMAIL PROTECTED] [...] --- a/scripts/kallsyms.c +++ b/scripts/kallsyms.c @@ -12,6 +12,8 @@ * (25/Aug/2004) Paulo Marques [EMAIL PROTECTED] * Changed the compression method from stem compression to table lookup * compression + * (10/Jul/2006) Robin Getz [EMAIL PROTECTED] + * Add _stext_l1, _etext_l1 for the L1 memory section in Blackfin. When I wrote this initially, it was a mistake to add a Changelog in the first place, but I didn't know better at the time. If you're going to make changes to this file, please remove all the Changelog, instead of adding more entries to it. The Changelog should be kept by the version control system, and not the source code itself. The rest of the patch looks ok, though. -- Paulo Marques - www.grupopie.com Very funny Scotty. Now beam up my clothes. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] POWERPC: use KSYM_NAME_LEN
Cyrill Gorcunov wrote:
[Paulo Marques - Wed, Jan 23, 2008 at 06:26:28PM +]
Cyrill Gorcunov wrote:
[...]
case 's':
- getstring(tmp, 64);
+ getstring(tmp, sizeof(tmp));
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
just after that poin in the original code a call to kallsyms_lookup_name
is done - so i think it could be an overflow (of course it depends
on what *exactly* the name is being searched, and Paulo - I didn't
managed to get *the whole picture* of what is going on in this
code - so the thoughs were like: kallsyms_lookup_name could find
a quite long name restricted by KSYM_NAME_LEN (dunno how it could
happens - due to buggy code or due to memory corruption outside,
it does not matter - the only matter - it *could* find that long
name).
Ah, now I understand your confusion: kallsyms_lookup_name doesn't fill
the name. It searches the name and returns the address. It is the
_caller_ that fills the name, not kallsyms_lookup_name.
It is used for stuff like: "give me the address of function foo":
addr = kallsyms_lookup_name("foo");
Anyway - it's just an attempt ;) we always could drop it far-far away ;)
I think that using KSYM_NAME_LEN would be a nice cleanup for xmon, but
it is for the powerpc guys to decide if they want to do it. I just
wanted to point the change in behavior so that it wouldn't go unnoticed.
For all we know, the stack may at that point be close to full and an
extra 64 bytes may tip it over the edge.
This also introduces a change in behavior. It is still a nice cleanup,
though. So, if the powerpc people feel they can spare an extra 64 bytes of
stack here, I guess it's ok.
Thanks a lot for review Paulo!
No problem. I always keep an eye out for kallsyms related stuff.
--
Paulo Marques - www.grupopie.com
"There cannot be a crisis today; my schedule is already full."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] POWERPC: use KSYM_NAME_LEN
Cyrill Gorcunov wrote:
Use KSYM_NAME_LEN instead of numeric value.
The patch series looks like a nice cleanup, except for a few things in
this patch.
Actually because of too small 'tmp' there is
a potential buffer overflow.
I don't think there is. "tmp" is not being passed to kallsyms to be
filled with a symbol name, but it's being used to hold a name written by
the user to lookup an address.
If the powerpc/xmon people feel that 63 characters is enough to hold a
symbol name, it's their problem, but there is no buffer overflow.
Signed-off-by: Cyrill Gorcunov <[EMAIL PROTECTED]>
---
Index: linux-2.6.git/arch/powerpc/xmon/xmon.c
===
--- linux-2.6.git.orig/arch/powerpc/xmon/xmon.c 2008-01-23 19:04:42.0
+0300
+++ linux-2.6.git/arch/powerpc/xmon/xmon.c 2008-01-23 19:12:45.0
+0300
@@ -69,7 +69,7 @@ static unsigned long ndump = 64;
static unsigned long nidump = 16;
static unsigned long ncsum = 4096;
static int termch;
-static char tmpstr[128];
+static char tmpstr[KSYM_NAME_LEN];
This one seems ok, since "tmpstr" is used everywhere to hold symbol names.
#define JMP_BUF_LEN23
static long bus_error_jmp[JMP_BUF_LEN];
@@ -2354,7 +2354,7 @@ scanhex(unsigned long *vp)
}
} else if (c == '$') {
int i;
- for (i=0; i<63; i++) {
+ for (i = 0; i < sizeof(tmpstr) / 2; i++) {
This one is completely out of the blue. Why "sizeof(tmpstr) / 2"?
It would make more sense to use "sizeof(tmpstr) - 1", but either way it
is a change in behavior.
c = inchar();
if (isspace(c)) {
termch = c;
@@ -2467,7 +2467,7 @@ symbol_lookup(void)
{
int type = inchar();
unsigned long addr;
- static char tmp[64];
+ static char tmp[KSYM_NAME_LEN];
switch (type) {
case 'a':
@@ -2476,7 +2476,7 @@ symbol_lookup(void)
termch = 0;
break;
case 's':
- getstring(tmp, 64);
+ getstring(tmp, sizeof(tmp));
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
This also introduces a change in behavior. It is still a nice cleanup,
though. So, if the powerpc people feel they can spare an extra 64 bytes
of stack here, I guess it's ok.
--
Paulo Marques - www.grupopie.com
"As far as we know, our computer has never had an undetected error."
Weisert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] POWERPC: use KSYM_NAME_LEN
Cyrill Gorcunov wrote:
Use KSYM_NAME_LEN instead of numeric value.
The patch series looks like a nice cleanup, except for a few things in
this patch.
Actually because of too small 'tmp' there is
a potential buffer overflow.
I don't think there is. tmp is not being passed to kallsyms to be
filled with a symbol name, but it's being used to hold a name written by
the user to lookup an address.
If the powerpc/xmon people feel that 63 characters is enough to hold a
symbol name, it's their problem, but there is no buffer overflow.
Signed-off-by: Cyrill Gorcunov [EMAIL PROTECTED]
---
Index: linux-2.6.git/arch/powerpc/xmon/xmon.c
===
--- linux-2.6.git.orig/arch/powerpc/xmon/xmon.c 2008-01-23 19:04:42.0
+0300
+++ linux-2.6.git/arch/powerpc/xmon/xmon.c 2008-01-23 19:12:45.0
+0300
@@ -69,7 +69,7 @@ static unsigned long ndump = 64;
static unsigned long nidump = 16;
static unsigned long ncsum = 4096;
static int termch;
-static char tmpstr[128];
+static char tmpstr[KSYM_NAME_LEN];
This one seems ok, since tmpstr is used everywhere to hold symbol names.
#define JMP_BUF_LEN23
static long bus_error_jmp[JMP_BUF_LEN];
@@ -2354,7 +2354,7 @@ scanhex(unsigned long *vp)
}
} else if (c == '$') {
int i;
- for (i=0; i63; i++) {
+ for (i = 0; i sizeof(tmpstr) / 2; i++) {
This one is completely out of the blue. Why sizeof(tmpstr) / 2?
It would make more sense to use sizeof(tmpstr) - 1, but either way it
is a change in behavior.
c = inchar();
if (isspace(c)) {
termch = c;
@@ -2467,7 +2467,7 @@ symbol_lookup(void)
{
int type = inchar();
unsigned long addr;
- static char tmp[64];
+ static char tmp[KSYM_NAME_LEN];
switch (type) {
case 'a':
@@ -2476,7 +2476,7 @@ symbol_lookup(void)
termch = 0;
break;
case 's':
- getstring(tmp, 64);
+ getstring(tmp, sizeof(tmp));
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
This also introduces a change in behavior. It is still a nice cleanup,
though. So, if the powerpc people feel they can spare an extra 64 bytes
of stack here, I guess it's ok.
--
Paulo Marques - www.grupopie.com
As far as we know, our computer has never had an undetected error.
Weisert
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] POWERPC: use KSYM_NAME_LEN
Cyrill Gorcunov wrote:
[Paulo Marques - Wed, Jan 23, 2008 at 06:26:28PM +]
Cyrill Gorcunov wrote:
[...]
case 's':
- getstring(tmp, 64);
+ getstring(tmp, sizeof(tmp));
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
just after that poin in the original code a call to kallsyms_lookup_name
is done - so i think it could be an overflow (of course it depends
on what *exactly* the name is being searched, and Paulo - I didn't
managed to get *the whole picture* of what is going on in this
code - so the thoughs were like: kallsyms_lookup_name could find
a quite long name restricted by KSYM_NAME_LEN (dunno how it could
happens - due to buggy code or due to memory corruption outside,
it does not matter - the only matter - it *could* find that long
name).
Ah, now I understand your confusion: kallsyms_lookup_name doesn't fill
the name. It searches the name and returns the address. It is the
_caller_ that fills the name, not kallsyms_lookup_name.
It is used for stuff like: give me the address of function foo:
addr = kallsyms_lookup_name(foo);
Anyway - it's just an attempt ;) we always could drop it far-far away ;)
I think that using KSYM_NAME_LEN would be a nice cleanup for xmon, but
it is for the powerpc guys to decide if they want to do it. I just
wanted to point the change in behavior so that it wouldn't go unnoticed.
For all we know, the stack may at that point be close to full and an
extra 64 bytes may tip it over the edge.
This also introduces a change in behavior. It is still a nice cleanup,
though. So, if the powerpc people feel they can spare an extra 64 bytes of
stack here, I guess it's ok.
Thanks a lot for review Paulo!
No problem. I always keep an eye out for kallsyms related stuff.
--
Paulo Marques - www.grupopie.com
There cannot be a crisis today; my schedule is already full.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmaped copy too slow?
KOSAKI Motohiro wrote: Hi at one point, I found the large file copy speed was different depending on the copy method. I compared below method - read(2) and write(2). - mmap(2) x2 and memcpy. - mmap(2) and write(2). in addition, effect of fadvice(2) and madvice(2) is checked. to a strange thing, - most faster method is read + write + fadvice. - worst method is mmap + memcpy. One thing you could also try is to pass MAP_POPULATE to mmap so that the page tables are filled in at the time of the mmap, avoiding a lot of page faults later. Just my 2 cents, -- Paulo Marques - www.grupopie.com "All I ask is a chance to prove that money can't make me happy." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmaped copy too slow?
KOSAKI Motohiro wrote: Hi at one point, I found the large file copy speed was different depending on the copy method. I compared below method - read(2) and write(2). - mmap(2) x2 and memcpy. - mmap(2) and write(2). in addition, effect of fadvice(2) and madvice(2) is checked. to a strange thing, - most faster method is read + write + fadvice. - worst method is mmap + memcpy. One thing you could also try is to pass MAP_POPULATE to mmap so that the page tables are filled in at the time of the mmap, avoiding a lot of page faults later. Just my 2 cents, -- Paulo Marques - www.grupopie.com All I ask is a chance to prove that money can't make me happy. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] call sysrq_timer_list_show from a workqueue
Rusty Russell wrote: On Wednesday 09 January 2008 11:21:59 Andrew Morton wrote: [...] And the fact that incoming arg `namebuf' MUST point at a KSYM_NAME_LEN-sized buffer could be better communicated by using a dedicated struct for this, or by giving the arg a type of `char namebuf[KSYM_NAME_LEN]'. Or by adding a comment. Or by just ignoring me and doing something more useful. Or better, rework all the name lookup interfaces, rather than having: Yes, there is some rework we can do here struct module *module_text_address(unsigned long addr); struct module *__module_text_address(unsigned long addr); int is_module_address(unsigned long addr); int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type, char *name, char *module_name, int *exported); char *module_address_lookup(unsigned long addr, unsigned long *symbolsize, unsigned long *offset, char **modname, char *namebuf); int lookup_module_symbol_name(unsigned long addr, char *symname); int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); unsigned long module_kallsyms_lookup_name(const char *name); All of these are somewhat less important, because most users call the kallsyms generic functions which will in turn call these functions if the symbol isn't global. So, they should suffer basically the same transformations as the kallsyms_ counterparts and can be considered almost "internal" to the kallsyms infrastructure. unsigned long kallsyms_lookup_name(const char *name); This one look fine, as there is no duplication in any other function. extern int kallsyms_lookup_size_offset(unsigned long addr, unsigned long *symbolsize, unsigned long *offset); const char *kallsyms_lookup(unsigned long addr, unsigned long *symbolsize, unsigned long *offset, char **modname, char *namebuf); int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); int lookup_symbol_name(unsigned long addr, char *symname); These 4 functions can probably be condensed into just one, by allowing NULL pointer arguments to mean "don't need this result": kallsyms_lookup_size_offset(a,s,o) <=> kallsyms_lookup(a,s,o,NULL,NULL) lookup_symbol_attrs(a,s,o,m,n) <=> kallsyms_lookup(a,s,o,m,n) lookup_symbol_name(a,n) <=> kallsyms_lookup(a,NULL,NULL,NULL,n) extern int sprint_symbol(char *buffer, unsigned long address); extern void __print_symbol(const char *fmt, unsigned long address); These 2 are probably fine. There is a difference in the way the module name is passed, because kallsyms_lookup assumes it can return just a pointer to the module name. However, we should probably change that interface so that the caller provides the buffer to hold the module name, to avoid races. The stop_machine should help already, but returning a pointer that can be stale just a little bit later isn't pretty anyway. I can do a patch for this, but this will touch a few subsystems that use these interfaces (there are not a lot of them, though). The major change would probably be the allocation of a small buffer (56~60 bytes) in some of the callers to hold the module name. -- Paulo Marques - www.grupopie.com "There cannot be a crisis today; my schedule is already full." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] call sysrq_timer_list_show from a workqueue
Rusty Russell wrote: On Wednesday 09 January 2008 11:21:59 Andrew Morton wrote: [...] And the fact that incoming arg `namebuf' MUST point at a KSYM_NAME_LEN-sized buffer could be better communicated by using a dedicated struct for this, or by giving the arg a type of `char namebuf[KSYM_NAME_LEN]'. Or by adding a comment. Or by just ignoring me and doing something more useful. Or better, rework all the name lookup interfaces, rather than having: Yes, there is some rework we can do here struct module *module_text_address(unsigned long addr); struct module *__module_text_address(unsigned long addr); int is_module_address(unsigned long addr); int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type, char *name, char *module_name, int *exported); char *module_address_lookup(unsigned long addr, unsigned long *symbolsize, unsigned long *offset, char **modname, char *namebuf); int lookup_module_symbol_name(unsigned long addr, char *symname); int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); unsigned long module_kallsyms_lookup_name(const char *name); All of these are somewhat less important, because most users call the kallsyms generic functions which will in turn call these functions if the symbol isn't global. So, they should suffer basically the same transformations as the kallsyms_ counterparts and can be considered almost internal to the kallsyms infrastructure. unsigned long kallsyms_lookup_name(const char *name); This one look fine, as there is no duplication in any other function. extern int kallsyms_lookup_size_offset(unsigned long addr, unsigned long *symbolsize, unsigned long *offset); const char *kallsyms_lookup(unsigned long addr, unsigned long *symbolsize, unsigned long *offset, char **modname, char *namebuf); int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); int lookup_symbol_name(unsigned long addr, char *symname); These 4 functions can probably be condensed into just one, by allowing NULL pointer arguments to mean don't need this result: kallsyms_lookup_size_offset(a,s,o) = kallsyms_lookup(a,s,o,NULL,NULL) lookup_symbol_attrs(a,s,o,m,n) = kallsyms_lookup(a,s,o,m,n) lookup_symbol_name(a,n) = kallsyms_lookup(a,NULL,NULL,NULL,n) extern int sprint_symbol(char *buffer, unsigned long address); extern void __print_symbol(const char *fmt, unsigned long address); These 2 are probably fine. There is a difference in the way the module name is passed, because kallsyms_lookup assumes it can return just a pointer to the module name. However, we should probably change that interface so that the caller provides the buffer to hold the module name, to avoid races. The stop_machine should help already, but returning a pointer that can be stale just a little bit later isn't pretty anyway. I can do a patch for this, but this will touch a few subsystems that use these interfaces (there are not a lot of them, though). The major change would probably be the allocation of a small buffer (56~60 bytes) in some of the callers to hold the module name. -- Paulo Marques - www.grupopie.com There cannot be a crisis today; my schedule is already full. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] libusb / in-kernel usb driver criteria (was: USB driver for talking to the Microchip PIC18 boot loader)
Xiaofan Chen wrote: On Dec 30, 2007 11:53 AM, mgross <[EMAIL PROTECTED]> wrote: [...] What is the linux-usb policies on new drivers that could be implemented in user space? When does a kernel driver make sense over a libusb one? That would be interesting to know. I myself have been faced with this question before, and I think we should try to clarify this by adding a document with some guidelines to Documentation/usb. So, to get the ball rolling, here are some factors that IMHO help decide in which side to implement a driver: - if the driver ties a hardware device to an existing in-kernel interface (network, block, serial, bluetooth, video4linux, etc.), it should probably be implemented in-kernel. - on the other hand, if the driver doesn't use an existing kernel interface and creates a new user-visible interface that is going to be used by a single userspace application, it should probably be done in userspace. - if it is going to be used by several applications it could still be implemented as a library, but it starts moving into the gray area. - performance might be a reason to move to kernel space, but I don't think it matters for transfer rates below 10Mbytes/sec or so. Anyway, this is just MHO, so feel free to discuss this further. I'm simply volunteering to sum up this thread into a patch to add a Documentation/usb/userspace_drivers.txt (or something like that), so that we can help future developers decide where to write their drivers. -- Paulo Marques - www.grupopie.com "Very funny Scotty. Now beam up my clothes." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] libusb / in-kernel usb driver criteria (was: USB driver for talking to the Microchip PIC18 boot loader)
Xiaofan Chen wrote: On Dec 30, 2007 11:53 AM, mgross [EMAIL PROTECTED] wrote: [...] What is the linux-usb policies on new drivers that could be implemented in user space? When does a kernel driver make sense over a libusb one? That would be interesting to know. I myself have been faced with this question before, and I think we should try to clarify this by adding a document with some guidelines to Documentation/usb. So, to get the ball rolling, here are some factors that IMHO help decide in which side to implement a driver: - if the driver ties a hardware device to an existing in-kernel interface (network, block, serial, bluetooth, video4linux, etc.), it should probably be implemented in-kernel. - on the other hand, if the driver doesn't use an existing kernel interface and creates a new user-visible interface that is going to be used by a single userspace application, it should probably be done in userspace. - if it is going to be used by several applications it could still be implemented as a library, but it starts moving into the gray area. - performance might be a reason to move to kernel space, but I don't think it matters for transfer rates below 10Mbytes/sec or so. Anyway, this is just MHO, so feel free to discuss this further. I'm simply volunteering to sum up this thread into a patch to add a Documentation/usb/userspace_drivers.txt (or something like that), so that we can help future developers decide where to write their drivers. -- Paulo Marques - www.grupopie.com Very funny Scotty. Now beam up my clothes. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [-mm PATCH] kallsyms should prefer non weak symbols
Mathieu Desnoyers wrote: * Paulo Marques ([EMAIL PROTECTED]) wrote: When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol. [...] From: Paulo Marques <[EMAIL PROTECTED]> Signed-off-by: Mathieu Desnoyers <[EMAIL PROTECTED]> Please wait for me to accept the changes before adding signed-off-by. My mistake was that I didn't realized you'd already corrected the original patch when you posted this: http://lkml.org/lkml/2007/11/13/292 Since I thought it was the same patch, it already had your "Signed-off-by". Sorry for the confusion... :( -- Paulo Marques - www.grupopie.com "I haven't lost my mind -- it's backed up on tape somewhere." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [-mm PATCH] kallsyms should prefer non weak symbols
Mathieu Desnoyers wrote: * Paulo Marques ([EMAIL PROTECTED]) wrote: When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol. [...] From: Paulo Marques [EMAIL PROTECTED] Signed-off-by: Mathieu Desnoyers [EMAIL PROTECTED] Please wait for me to accept the changes before adding signed-off-by. My mistake was that I didn't realized you'd already corrected the original patch when you posted this: http://lkml.org/lkml/2007/11/13/292 Since I thought it was the same patch, it already had your Signed-off-by. Sorry for the confusion... :( -- Paulo Marques - www.grupopie.com I haven't lost my mind -- it's backed up on tape somewhere. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[-mm PATCH] kallsyms should prefer non weak symbols
When resolving symbol names from addresses with aliased symbol names,
kallsyms_lookup always returns the first symbol, even if it is a weak
symbol.
This patch changes this by sorting the symbols with the weak symbols
last before feeding them to the kernel. This way the kernel runtime
isn't changed at all, only the kallsyms build system is changed.
Another side effect is that the symbols get sorted by address, too. So,
even if future binutils version have some bug in "nm" that makes it fail
to correctly sort symbols by address, the kernel won't be affected by this.
From: Paulo Marques <[EMAIL PROTECTED]>
Signed-off-by: Mathieu Desnoyers <[EMAIL PROTECTED]>
--
Paulo Marques - www.grupopie.com
"There cannot be a crisis today; my schedule is already full."
--- ./scripts/kallsyms.c.orig 2007-10-30 18:51:28.0 +
+++ ./scripts/kallsyms.c2007-10-30 19:07:58.0 +
@@ -34,7 +34,7 @@
struct sym_entry {
unsigned long long addr;
- unsigned int len;
+ unsigned int len, start_pos;
unsigned char *sym;
};
@@ -202,8 +202,10 @@ static void read_map(FILE *in)
exit (1);
}
}
- if (read_symbol(in, [table_cnt]) == 0)
+ if (read_symbol(in, [table_cnt]) == 0) {
+ table[table_cnt].start_pos = table_cnt;
table_cnt++;
+ }
}
}
@@ -507,6 +509,35 @@ static void optimize_token_table(void)
}
+static int compare_symbols(const void *a, const void *b)
+{
+ struct sym_entry *sa, *sb;
+ int wa, wb;
+
+ sa = (struct sym_entry *) a;
+ sb = (struct sym_entry *) b;
+
+ // sort by address first
+ if (sa->addr > sb->addr)
+ return 1;
+ if (sa->addr < sb->addr)
+ return -1;
+
+ // sort by "weakness" type
+ wa = (sa->sym[0] == 'w') || (sa->sym[0] == 'W');
+ wb = (sb->sym[0] == 'w') || (sb->sym[0] == 'W');
+ if (wa != wb)
+ return wa - wb;
+
+ // sort by initial order, so that other symbols are left undisturbed
+ return sa->start_pos - sb->start_pos;
+}
+
+static void sort_symbols(void)
+{
+ qsort(table, table_cnt, sizeof(struct sym_entry), compare_symbols);
+}
+
int main(int argc, char **argv)
{
if (argc >= 2) {
@@ -527,6 +558,7 @@ int main(int argc, char **argv)
usage();
read_map(stdin);
+ sort_symbols();
optimize_token_table();
write_src();
[-mm PATCH] kallsyms should prefer non weak symbols
When resolving symbol names from addresses with aliased symbol names,
kallsyms_lookup always returns the first symbol, even if it is a weak
symbol.
This patch changes this by sorting the symbols with the weak symbols
last before feeding them to the kernel. This way the kernel runtime
isn't changed at all, only the kallsyms build system is changed.
Another side effect is that the symbols get sorted by address, too. So,
even if future binutils version have some bug in nm that makes it fail
to correctly sort symbols by address, the kernel won't be affected by this.
From: Paulo Marques [EMAIL PROTECTED]
Signed-off-by: Mathieu Desnoyers [EMAIL PROTECTED]
--
Paulo Marques - www.grupopie.com
There cannot be a crisis today; my schedule is already full.
--- ./scripts/kallsyms.c.orig 2007-10-30 18:51:28.0 +
+++ ./scripts/kallsyms.c2007-10-30 19:07:58.0 +
@@ -34,7 +34,7 @@
struct sym_entry {
unsigned long long addr;
- unsigned int len;
+ unsigned int len, start_pos;
unsigned char *sym;
};
@@ -202,8 +202,10 @@ static void read_map(FILE *in)
exit (1);
}
}
- if (read_symbol(in, table[table_cnt]) == 0)
+ if (read_symbol(in, table[table_cnt]) == 0) {
+ table[table_cnt].start_pos = table_cnt;
table_cnt++;
+ }
}
}
@@ -507,6 +509,35 @@ static void optimize_token_table(void)
}
+static int compare_symbols(const void *a, const void *b)
+{
+ struct sym_entry *sa, *sb;
+ int wa, wb;
+
+ sa = (struct sym_entry *) a;
+ sb = (struct sym_entry *) b;
+
+ // sort by address first
+ if (sa-addr sb-addr)
+ return 1;
+ if (sa-addr sb-addr)
+ return -1;
+
+ // sort by weakness type
+ wa = (sa-sym[0] == 'w') || (sa-sym[0] == 'W');
+ wb = (sb-sym[0] == 'w') || (sb-sym[0] == 'W');
+ if (wa != wb)
+ return wa - wb;
+
+ // sort by initial order, so that other symbols are left undisturbed
+ return sa-start_pos - sb-start_pos;
+}
+
+static void sort_symbols(void)
+{
+ qsort(table, table_cnt, sizeof(struct sym_entry), compare_symbols);
+}
+
int main(int argc, char **argv)
{
if (argc = 2) {
@@ -527,6 +558,7 @@ int main(int argc, char **argv)
usage();
read_map(stdin);
+ sort_symbols();
optimize_token_table();
write_src();
Re: [PATCH] Kallsyms Should Prefer Non Weak Symbols
Mathieu Desnoyers wrote: * Mathieu Desnoyers ([EMAIL PROTECTED]) wrote: [...] kallsyms returns the first symbol encountered, even though it is weak, when it should in fact return sys_ni_syscall. Is it a concern for anyone else out there ? Would it make sense to fix it ? I don't know if it is a concern, but if we're going to fix it, we should probably do it in "scripts/kallsyms" by providing a list that is already sorted according to "address, weakness". This way the run-time kernel keeps the current behavior, without any overhead. Something along the lines of the attached patch (just compile tested). However, this is an area where we've had problems in the past with some architectures giving different results between passes, and then any change to the symbol order might make the problem worse and make the build process fail with a "Inconsistent kallsyms data" error message. So, if someone wants to use this, it should go through -mm for a while, first. It applies on top of 2.6.24-rc2-git3. Please use this reply with correct CC list for further discussion. I've been wanting to send this as a proper patch request email, but I just hadn't found the time to do it, and then our mail server just went berserk and I lost 5 days of LKML :P I think the patch is ok as it is, but a nice message explaining what it does and why would be nice for the changelog. So, I'll post a new message with a nice description for inclusion in -mm today. Sorry for the delay, -- Paulo Marques - www.grupopie.com "Very funny Scotty. Now beam up my clothes." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Kallsyms Should Prefer Non Weak Symbols
Mathieu Desnoyers wrote: * Mathieu Desnoyers ([EMAIL PROTECTED]) wrote: [...] kallsyms returns the first symbol encountered, even though it is weak, when it should in fact return sys_ni_syscall. Is it a concern for anyone else out there ? Would it make sense to fix it ? I don't know if it is a concern, but if we're going to fix it, we should probably do it in scripts/kallsyms by providing a list that is already sorted according to address, weakness. This way the run-time kernel keeps the current behavior, without any overhead. Something along the lines of the attached patch (just compile tested). However, this is an area where we've had problems in the past with some architectures giving different results between passes, and then any change to the symbol order might make the problem worse and make the build process fail with a Inconsistent kallsyms data error message. So, if someone wants to use this, it should go through -mm for a while, first. It applies on top of 2.6.24-rc2-git3. Please use this reply with correct CC list for further discussion. I've been wanting to send this as a proper patch request email, but I just hadn't found the time to do it, and then our mail server just went berserk and I lost 5 days of LKML :P I think the patch is ok as it is, but a nice message explaining what it does and why would be nice for the changelog. So, I'll post a new message with a nice description for inclusion in -mm today. Sorry for the delay, -- Paulo Marques - www.grupopie.com Very funny Scotty. Now beam up my clothes. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: kallsyms __print_symbol prints first weak symbol encountered
Mathieu Desnoyers wrote:
Hi,
Hi,
[...]
kallsyms returns the first symbol encountered, even though it is weak,
when it should in fact return sys_ni_syscall.
Is it a concern for anyone else out there ? Would it make sense to fix
it ?
I don't know if it is a concern, but if we're going to fix it, we should
probably do it in "scripts/kallsyms" by providing a list that is already
sorted according to "address, weakness".
This way the run-time kernel keeps the current behavior, without any
overhead. Something along the lines of the attached patch (just compile
tested).
However, this is an area where we've had problems in the past with some
architectures giving different results between passes, and then any
change to the symbol order might make the problem worse and make the
build process fail with a "Inconsistent kallsyms data" error message.
So, if someone wants to use this, it should go through -mm for a while,
first.
--
Paulo Marques - www.grupopie.com
"All I ask is a chance to prove that money can't make me happy."
--- ./scripts/kallsyms.c.orig 2007-10-30 18:51:28.0 +
+++ ./scripts/kallsyms.c2007-10-30 19:07:58.0 +
@@ -34,7 +34,7 @@
struct sym_entry {
unsigned long long addr;
- unsigned int len;
+ unsigned int len, start_pos;
unsigned char *sym;
};
@@ -202,8 +202,10 @@ static void read_map(FILE *in)
exit (1);
}
}
- if (read_symbol(in, [table_cnt]) == 0)
+ if (read_symbol(in, [table_cnt]) == 0) {
+ table[table_cnt].start_pos = table_cnt;
table_cnt++;
+ }
}
}
@@ -507,6 +509,35 @@ static void optimize_token_table(void)
}
+static int compare_symbols(const void *a, const void *b)
+{
+ struct sym_entry *sa, *sb;
+ int wa, wb;
+
+ sa = (struct sym_entry *) a;
+ sb = (struct sym_entry *) b;
+
+ // sort by address first
+ if (sa->addr > sb->addr)
+ return 1;
+ if (sa->addr < sb->addr)
+ return -1;
+
+ // sort by "weakness" type
+ wa = (sa->sym[0] == 'w') || (sa->sym[0] == 'W');
+ wb = (sb->sym[0] == 'w') || (sb->sym[0] == 'W');
+ if (wa != wb)
+ return wa - wb;
+
+ // sort by initial order, so that other symbols are left undisturbed
+ return sa->start_pos - sb->start_pos;
+}
+
+static void sort_symbols(void)
+{
+ qsort(table, table_cnt, sizeof(struct sym_entry), compare_symbols);
+}
+
int main(int argc, char **argv)
{
if (argc >= 2) {
@@ -527,6 +558,7 @@ int main(int argc, char **argv)
usage();
read_map(stdin);
+ sort_symbols();
optimize_token_table();
write_src();
Re: kallsyms __print_symbol prints first weak symbol encountered
Mathieu Desnoyers wrote:
Hi,
Hi,
[...]
kallsyms returns the first symbol encountered, even though it is weak,
when it should in fact return sys_ni_syscall.
Is it a concern for anyone else out there ? Would it make sense to fix
it ?
I don't know if it is a concern, but if we're going to fix it, we should
probably do it in scripts/kallsyms by providing a list that is already
sorted according to address, weakness.
This way the run-time kernel keeps the current behavior, without any
overhead. Something along the lines of the attached patch (just compile
tested).
However, this is an area where we've had problems in the past with some
architectures giving different results between passes, and then any
change to the symbol order might make the problem worse and make the
build process fail with a Inconsistent kallsyms data error message.
So, if someone wants to use this, it should go through -mm for a while,
first.
--
Paulo Marques - www.grupopie.com
All I ask is a chance to prove that money can't make me happy.
--- ./scripts/kallsyms.c.orig 2007-10-30 18:51:28.0 +
+++ ./scripts/kallsyms.c2007-10-30 19:07:58.0 +
@@ -34,7 +34,7 @@
struct sym_entry {
unsigned long long addr;
- unsigned int len;
+ unsigned int len, start_pos;
unsigned char *sym;
};
@@ -202,8 +202,10 @@ static void read_map(FILE *in)
exit (1);
}
}
- if (read_symbol(in, table[table_cnt]) == 0)
+ if (read_symbol(in, table[table_cnt]) == 0) {
+ table[table_cnt].start_pos = table_cnt;
table_cnt++;
+ }
}
}
@@ -507,6 +509,35 @@ static void optimize_token_table(void)
}
+static int compare_symbols(const void *a, const void *b)
+{
+ struct sym_entry *sa, *sb;
+ int wa, wb;
+
+ sa = (struct sym_entry *) a;
+ sb = (struct sym_entry *) b;
+
+ // sort by address first
+ if (sa-addr sb-addr)
+ return 1;
+ if (sa-addr sb-addr)
+ return -1;
+
+ // sort by weakness type
+ wa = (sa-sym[0] == 'w') || (sa-sym[0] == 'W');
+ wb = (sb-sym[0] == 'w') || (sb-sym[0] == 'W');
+ if (wa != wb)
+ return wa - wb;
+
+ // sort by initial order, so that other symbols are left undisturbed
+ return sa-start_pos - sb-start_pos;
+}
+
+static void sort_symbols(void)
+{
+ qsort(table, table_cnt, sizeof(struct sym_entry), compare_symbols);
+}
+
int main(int argc, char **argv)
{
if (argc = 2) {
@@ -527,6 +558,7 @@ int main(int argc, char **argv)
usage();
read_map(stdin);
+ sort_symbols();
optimize_token_table();
write_src();
Re: /proc/kallsyms and symbol size
Stephane Eranian wrote: Hello, Hi, Stephane Many monitoring tools use /proc/kallsyms to build a symbol table for the kernel. This technique has the advantage that it does not require root privileges, nor an up-to-date /boot/System.map, nor a decompressed kernel in /boot. The problem is that /proc/kallsyms does not report the size of the symbols. Yet, the information is available in the kernel as it is used by functions such as __print_symbol(). Having the size is useful to correlate the address obtained is a sample with a symbol name. Most tools use an approximation which assumes symbols are contiguous to estimate the size. That is actually what the kernel does internally, too. It does not keep the size of the symbol, but tries to guess it from the address of the next non-aliased symbol. Since the addresses are sorted, this works fine most of the time. This is done to reduce the size used by the symbol table in the running kernel. Just take a look at "get_symbol_pos" in kernel/kallsyms.c and "get_ksymbol" in kernel/module.c to see exactly how this is done -- Paulo Marques - www.grupopie.com "There cannot be a crisis today; my schedule is already full." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: /proc/kallsyms and symbol size
Stephane Eranian wrote: Hello, Hi, Stephane Many monitoring tools use /proc/kallsyms to build a symbol table for the kernel. This technique has the advantage that it does not require root privileges, nor an up-to-date /boot/System.map, nor a decompressed kernel in /boot. The problem is that /proc/kallsyms does not report the size of the symbols. Yet, the information is available in the kernel as it is used by functions such as __print_symbol(). Having the size is useful to correlate the address obtained is a sample with a symbol name. Most tools use an approximation which assumes symbols are contiguous to estimate the size. That is actually what the kernel does internally, too. It does not keep the size of the symbol, but tries to guess it from the address of the next non-aliased symbol. Since the addresses are sorted, this works fine most of the time. This is done to reduce the size used by the symbol table in the running kernel. Just take a look at get_symbol_pos in kernel/kallsyms.c and get_ksymbol in kernel/module.c to see exactly how this is done -- Paulo Marques - www.grupopie.com There cannot be a crisis today; my schedule is already full. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage. (v3)
Gilboa Davara wrote: Hello all, Hi, Gilboa (1) Problem: I. When CONFIG_4KSTACKS and CONFIG_DEBUG_STACKOVERFLOW are enabled on i386 kernels, do_IRQ calls dump_stack which, down the path, uses print_symbol (display) and sprint_symbol (format) to format and display the function name/address/module. Both function use stack based char array (~350 bytes) that, given the initial state (<512 bytes of stack space) may overrun the stack. II. (Comments - previous patches) Using spinlock protected static storage within these functions might block or even deadlock dump_stack (E.g. Crash within dump_stack itself) (2) Solution: I. Break sprint_symbol into sprint_symbol (API functions; keeps the current interface) and sprint_symbol_helper (helper function with minimal local storage). II. Replace the char array in __print_symbol with two spinlock protected static char arrays; call the __sprint_symbol helper function instead of sprint_symbol. III. Ignore the spinlock if oops_in_progress is set. This is getting more and more convoluted :( The problem with the spinlock isn't just that during a panic, we can not trust the kernel structures enough to use spinlocks. It might well happen that lockdep code might want to use print_symbol (and I think it does, so this is not just theoretical) to dump the stack when someone calls spin_lock_irqsave. But now, because print_symbol itself uses spin_lock_irqsave, we might get into a recursive situation and a produce a deadlock. On the other hand, if you take the other approach of reducing the stack usage by creating a printk_symbol interface, the stack usage would drop from 350 bytes to 128 bytes and your problem would go away entirely. -- Paulo Marques - www.grupopie.com "All I ask is a chance to prove that money can't make me happy." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage.
Steven Rostedt wrote:
On Wed, Sep 19, 2007 at 03:25:15PM +0100, Paulo Marques wrote:
if we change the interface from "print_symbol(fmt, addr)" to
"print_symbol(prefix, addr, int newline)" we can simply do:
printk(prefix);
printk_symbol(addr);
if (newline)
printk("\n");
NACK
I just wrote something that does "print_symbol(" %s)\n", addr);"
Notice the ")" in the output.
We can just change that to "print_symbol(prefix, addr, suffix)" instead.
The concept is basically the same (and I wasn't very fond of that
newline argument either).
As an added bonus we stop needing an extra layer to check that the
string passed is in the right format with a single "%s" in it.
--
Paulo Marques - www.grupopie.com
"Very funny Scotty. Now beam up my clothes."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage.
Gilboa Davara wrote:
Hello Paulo,
Hi, Gilboa
[snip]
[...]
if we change the interface from "print_symbol(fmt, addr)" to
"print_symbol(prefix, addr, int newline)" we can simply do:
printk(prefix);
printk_symbol(addr);
if (newline)
printk("\n");
where "printk_symbol" is a new function that does the same as
sprint_symbol, but does "printk" instead of "sprintf".
This should reduce immensely the stack usage of print_symbol without the
need for locking.
I fully agree.
... Further more, multiple printk_symbols should be combined into a
single, multi-line printk transaction. (To prevent debug printk's from
trashing a BUG() dump_stack).
Usually the developer can separate the output by hand in the unlikely
case of simultaneous concurrent users of printk, so I don't think this
is really a big problem.
Of course this requires changing _all_ callers of print_symbol to use
the new interface, but these are less than 100 ;)
This is my first contribution to the Linux kernel. As such I rather
start small, and work my way up slowly. (Read: solve the immediate stack
over-run now, think about changing the symbol_display interface later)
Yes, but this is a sensitive area, so you can not just implement
something now that can cause regressions, and just fix it later.
Kernel panics are quite rare and the information provided by the stack
dump is _extremely_ precious.
Even more, risking deadlocks caused by something that should only be
used to produce debug information is even worse.
Comments?
I do agree that the current interface needs work.
... But as I said, I rather start slowly and on small scale. (Though I
did find a rather problematic place to start at... ;))
Well, if we agree that this is the way to go, then the way to start
slowly would be to submit a patch that makes both interfaces available
for a while and changes the most stack-critical callers to the new
interface.
Then slowly keep submitting patches to change other callers
progressively until there are no more callers of the old interface. At
that point we can just drop it entirely.
--
Paulo Marques - www.grupopie.com
"God is love. Love is blind. Ray Charles is blind. Ray Charles is God."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage.
Gilboa Davara wrote: Hello Paulo, Hi, Gilboa [snip] [...] if we change the interface from print_symbol(fmt, addr) to print_symbol(prefix, addr, int newline) we can simply do: printk(prefix); printk_symbol(addr); if (newline) printk(\n); where printk_symbol is a new function that does the same as sprint_symbol, but does printk instead of sprintf. This should reduce immensely the stack usage of print_symbol without the need for locking. I fully agree. ... Further more, multiple printk_symbols should be combined into a single, multi-line printk transaction. (To prevent debug printk's from trashing a BUG() dump_stack). Usually the developer can separate the output by hand in the unlikely case of simultaneous concurrent users of printk, so I don't think this is really a big problem. Of course this requires changing _all_ callers of print_symbol to use the new interface, but these are less than 100 ;) This is my first contribution to the Linux kernel. As such I rather start small, and work my way up slowly. (Read: solve the immediate stack over-run now, think about changing the symbol_display interface later) Yes, but this is a sensitive area, so you can not just implement something now that can cause regressions, and just fix it later. Kernel panics are quite rare and the information provided by the stack dump is _extremely_ precious. Even more, risking deadlocks caused by something that should only be used to produce debug information is even worse. Comments? I do agree that the current interface needs work. ... But as I said, I rather start slowly and on small scale. (Though I did find a rather problematic place to start at... ;)) Well, if we agree that this is the way to go, then the way to start slowly would be to submit a patch that makes both interfaces available for a while and changes the most stack-critical callers to the new interface. Then slowly keep submitting patches to change other callers progressively until there are no more callers of the old interface. At that point we can just drop it entirely. -- Paulo Marques - www.grupopie.com God is love. Love is blind. Ray Charles is blind. Ray Charles is God. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage.
Steven Rostedt wrote: On Wed, Sep 19, 2007 at 03:25:15PM +0100, Paulo Marques wrote: if we change the interface from print_symbol(fmt, addr) to print_symbol(prefix, addr, int newline) we can simply do: printk(prefix); printk_symbol(addr); if (newline) printk(\n); NACK I just wrote something that does print_symbol( %s)\n, addr); Notice the ) in the output. We can just change that to print_symbol(prefix, addr, suffix) instead. The concept is basically the same (and I wasn't very fond of that newline argument either). As an added bonus we stop needing an extra layer to check that the string passed is in the right format with a single %s in it. -- Paulo Marques - www.grupopie.com Very funny Scotty. Now beam up my clothes. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage. (v3)
Gilboa Davara wrote: Hello all, Hi, Gilboa (1) Problem: I. When CONFIG_4KSTACKS and CONFIG_DEBUG_STACKOVERFLOW are enabled on i386 kernels, do_IRQ calls dump_stack which, down the path, uses print_symbol (display) and sprint_symbol (format) to format and display the function name/address/module. Both function use stack based char array (~350 bytes) that, given the initial state (512 bytes of stack space) may overrun the stack. II. (Comments - previous patches) Using spinlock protected static storage within these functions might block or even deadlock dump_stack (E.g. Crash within dump_stack itself) (2) Solution: I. Break sprint_symbol into sprint_symbol (API functions; keeps the current interface) and sprint_symbol_helper (helper function with minimal local storage). II. Replace the char array in __print_symbol with two spinlock protected static char arrays; call the __sprint_symbol helper function instead of sprint_symbol. III. Ignore the spinlock if oops_in_progress is set. This is getting more and more convoluted :( The problem with the spinlock isn't just that during a panic, we can not trust the kernel structures enough to use spinlocks. It might well happen that lockdep code might want to use print_symbol (and I think it does, so this is not just theoretical) to dump the stack when someone calls spin_lock_irqsave. But now, because print_symbol itself uses spin_lock_irqsave, we might get into a recursive situation and a produce a deadlock. On the other hand, if you take the other approach of reducing the stack usage by creating a printk_symbol interface, the stack usage would drop from 350 bytes to 128 bytes and your problem would go away entirely. -- Paulo Marques - www.grupopie.com All I ask is a chance to prove that money can't make me happy. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage.
Satyam Sharma wrote:
On Sat, 15 Sep 2007, Gilboa Davara wrote:
printk(fmt, buffer);
+
+ spin_unlock_irqrestore(_lock, flags);
But I still don't much like this :-(
I must say I agree with Satyam here.
Locking in the panic path might leave us without some critical debug
information, which is much more important than all this.
Maybe it would be better to change the print_symbol interface to avoid
having a "char buffer[KSYM_SYMBOL_LEN];" at all.
Most print_symbol callers use something like "yada yada %s" as the
format string, with an optional "\n" in the end.
if we change the interface from "print_symbol(fmt, addr)" to
"print_symbol(prefix, addr, int newline)" we can simply do:
printk(prefix);
printk_symbol(addr);
if (newline)
printk("\n");
where "printk_symbol" is a new function that does the same as
sprint_symbol, but does "printk" instead of "sprintf".
This should reduce immensely the stack usage of print_symbol without the
need for locking.
Of course this requires changing _all_ callers of print_symbol to use
the new interface, but these are less than 100 ;)
Comments?
--
Paulo Marques - www.grupopie.com
"You're just jealous because the voices only talk to me."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage.
Satyam Sharma wrote: On Sat, 15 Sep 2007, Gilboa Davara wrote: printk(fmt, buffer); + + spin_unlock_irqrestore(symbol_lock, flags); But I still don't much like this :-( I must say I agree with Satyam here. Locking in the panic path might leave us without some critical debug information, which is much more important than all this. Maybe it would be better to change the print_symbol interface to avoid having a char buffer[KSYM_SYMBOL_LEN]; at all. Most print_symbol callers use something like yada yada %s as the format string, with an optional \n in the end. if we change the interface from print_symbol(fmt, addr) to print_symbol(prefix, addr, int newline) we can simply do: printk(prefix); printk_symbol(addr); if (newline) printk(\n); where printk_symbol is a new function that does the same as sprint_symbol, but does printk instead of sprintf. This should reduce immensely the stack usage of print_symbol without the need for locking. Of course this requires changing _all_ callers of print_symbol to use the new interface, but these are less than 100 ;) Comments? -- Paulo Marques - www.grupopie.com You're just jealous because the voices only talk to me. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] video: uvesafb: Add X86 dependency.
Paul Mundt wrote: On Tue, Sep 11, 2007 at 12:41:49PM +0100, Paulo Marques wrote: [...] Why do you say that it's x86 specific? Am I missing something? The emulator it uses only runs on x86 and x86_64. Thus, it's x86 specific. The v86d and uvesafb pages seem to be in disagremeent, unless by 'non-x86' it's only implying x86_64. Additionally, it needs the vga I/O routines, as per vgacon. Most platforms don't define these. I just saw the v86d emulator code, and you're right. It mmaps /dev/mem and assumes the video BIOS is mapped there. We should try to change that to mmap something like "/sys/bus/pci/devices/:01:00.0/rom" instead so that it can work on any system with a video card plugged on a PCI bus. It should also be possible for the emulator to translate in/out instructions to appropriate memory mapped I/O equivalents, no? Anyway, I think it is up to Michal to decide if we should remove the kernel support for other archs, or let it stay a bit more while working on solving the v86d side of things. So I'll just step aside now -- Paulo Marques - www.grupopie.com "667: The neighbor of the beast." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] video: uvesafb: Add X86 dependency.
Paul Mundt wrote: uvesafb is x86-specific, reflect that in the Kconfig. Hummm... uvesafb _shouldn't_ be x86 specific. At least according to their page [1] where it says: "works on non-x86 systems". Uvesafb uses a x86 emulator in userspace to run code from the video card ROM, so it should work on any PCI system where we can access the video card ROM and can emulate the hardware used by the ROM code. Why do you say that it's x86 specific? Am I missing something? -- Paulo Marques - www.grupopie.com "You're just jealous because the voices only talk to me." [1] http://dev.gentoo.org/~spock/projects/uvesafb/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] video: uvesafb: Add X86 dependency.
Paul Mundt wrote: uvesafb is x86-specific, reflect that in the Kconfig. Hummm... uvesafb _shouldn't_ be x86 specific. At least according to their page [1] where it says: works on non-x86 systems. Uvesafb uses a x86 emulator in userspace to run code from the video card ROM, so it should work on any PCI system where we can access the video card ROM and can emulate the hardware used by the ROM code. Why do you say that it's x86 specific? Am I missing something? -- Paulo Marques - www.grupopie.com You're just jealous because the voices only talk to me. [1] http://dev.gentoo.org/~spock/projects/uvesafb/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] video: uvesafb: Add X86 dependency.
Paul Mundt wrote: On Tue, Sep 11, 2007 at 12:41:49PM +0100, Paulo Marques wrote: [...] Why do you say that it's x86 specific? Am I missing something? The emulator it uses only runs on x86 and x86_64. Thus, it's x86 specific. The v86d and uvesafb pages seem to be in disagremeent, unless by 'non-x86' it's only implying x86_64. Additionally, it needs the vga I/O routines, as per vgacon. Most platforms don't define these. I just saw the v86d emulator code, and you're right. It mmaps /dev/mem and assumes the video BIOS is mapped there. We should try to change that to mmap something like /sys/bus/pci/devices/:01:00.0/rom instead so that it can work on any system with a video card plugged on a PCI bus. It should also be possible for the emulator to translate in/out instructions to appropriate memory mapped I/O equivalents, no? Anyway, I think it is up to Michal to decide if we should remove the kernel support for other archs, or let it stay a bit more while working on solving the v86d side of things. So I'll just step aside now -- Paulo Marques - www.grupopie.com 667: The neighbor of the beast. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kernel panic with 2.6.23-rc5
Tilman Schmidt wrote: Paulo Marques schrieb: I just tried booting a brand new 2.6.23-rc5 and after a few minutes it just panicked: machine totally frozen, blinking keyboard leds. [...] Maybe someone out there has a good suggestion that I could try before bisecting... A probable candidate would be: http://lkml.org/lkml/2007/9/2/219 I've been running with that patch applied for a few hours now and everything seems to be working fine. Without the patch the kernel would hang in a few minutes, so I guess this fixed it. Thanks for the help, -- Paulo Marques - www.grupopie.com "The Computer made me do it." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kernel panic with 2.6.23-rc5
Tilman Schmidt wrote: Paulo Marques schrieb: I just tried booting a brand new 2.6.23-rc5 and after a few minutes it just panicked: machine totally frozen, blinking keyboard leds. [...] Maybe someone out there has a good suggestion that I could try before bisecting... A probable candidate would be: http://lkml.org/lkml/2007/9/2/219 I've been running with that patch applied for a few hours now and everything seems to be working fine. Without the patch the kernel would hang in a few minutes, so I guess this fixed it. Thanks for the help, -- Paulo Marques - www.grupopie.com The Computer made me do it. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] make kobject dynamic allocation check use kallsyms_lookup()
Dave Hansen wrote:
One of the top ten sysfs problems is that users use statically
allocated kobjects. This patch reminds them that this is a
naughty thing.
One _really_ nice thing this patch does, is us the kallsyms
mechanism to print out exactly which symbol is being complained
about:
The kobject at, or inside 'statickobj.2'@(0xc040d020) is not
dynamically allocated.
This patch replaces the previous implementation's use of a
_sdata symbol in favor of using kallsyms_lookup(). If a
kobject's address is a resolvable symbol, then it isn't
dynamically allocated.
Just a few concerns that I'm not sure of having been addressed:
- doing a kallsyms_lookup() is more expensive then just a simple range
test. This might be a concern if this is called very often. In this case
you could keep the range check and only do the lookup for symbols that
fail that check
- kallsyms_lookup() never finds a symbol if CONFIG_KALLSYMS is not
selected
- more comments below
The one exception to this is init symbols. The patch also
checks to see whether __init memory has been freed and if
it has will allow kobjects in those sections.
Signed-off-by: Dave Hansen <[EMAIL PROTECTED]>
---
lxc-dave/arch/i386/kernel/vmlinux.lds.S |2 --
lxc-dave/include/linux/init.h |1 +
lxc-dave/init/main.c|9 +
lxc-dave/lib/kobject.c | 31 ++-
4 files changed, 32 insertions(+), 11 deletions(-)
diff -puN
lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup
lib/kobject.c
---
lxc/lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup
2007-08-22 14:51:50.0 -0700
+++ lxc-dave/lib/kobject.c 2007-08-22 14:51:50.0 -0700
@@ -139,23 +139,36 @@ static int ptr_in_range(void *ptr, void
return 0;
}
-static void verify_dynamic_kobject_allocation(struct kobject *kobj)
+void verify_dynamic_kobject_allocation(struct kobject *kobj)
{
- if (ptr_in_range(kobj, &_sdata[0], &_edata[0]))
- goto warn;
- if (ptr_in_range(kobj, &__bss_start[0], &__bss_stop[0]))
- goto warn;
- return;
-warn:
+ char *namebuf;
+ const char *ret;
+
+ namebuf = kzalloc(KSYM_NAME_LEN, GFP_KERNEL);
You don't need kzalloc here. kmalloc will do just fine.
+ ret = kallsyms_lookup((unsigned long)kobj, NULL, NULL, NULL,
+ namebuf);
+ /*
+* This is the X86_32-only part of this function.
+* This is here because it is valid to have a kobject
+* in an __init section, but only after those
+* sections have been freed back to the dynamic pool.
+*/
+ if (!initmem_now_dynamic &&
+ ptr_in_range(kobj, __init_begin, __init_end))
+ goto out;
+ if (!ret || !strlen(ret))
The "!strlen(ret)" is not only weird (why not write as "!ret[0] or
!*ret) but is also unnecessary. When kallsyms_lookup fails to find a
symbol it should always return NULL.
+ goto out;
pr_debug(" begin silly warning \n");
pr_debug("This is a janitorial warning, not a kernel bug.\n");
#ifdef CONFIG_DEBUG_KOBJECT
- print_symbol("The kobject at, or inside %s is not dynamically
allocated.\n",
- (unsigned long)kobj);
+ pr_debug("The kobject at, or inside '%s'@(0x%p) is not dynamically
allocated.\n",
+ namebuf, kobj);
#endif
pr_debug("kobjects must be dynamically allocated, not static\n");
/* dump_stack(); */
pr_debug(" end silly warning \n");
+out:
+ kfree(namebuf);
}
#else
[...]
--
Paulo Marques - www.grupopie.com
"You're just jealous because the voices only talk to me."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] make kobject dynamic allocation check use kallsyms_lookup()
Dave Hansen wrote:
One of the top ten sysfs problems is that users use statically
allocated kobjects. This patch reminds them that this is a
naughty thing.
One _really_ nice thing this patch does, is us the kallsyms
mechanism to print out exactly which symbol is being complained
about:
The kobject at, or inside 'statickobj.2'@(0xc040d020) is not
dynamically allocated.
This patch replaces the previous implementation's use of a
_sdata symbol in favor of using kallsyms_lookup(). If a
kobject's address is a resolvable symbol, then it isn't
dynamically allocated.
Just a few concerns that I'm not sure of having been addressed:
- doing a kallsyms_lookup() is more expensive then just a simple range
test. This might be a concern if this is called very often. In this case
you could keep the range check and only do the lookup for symbols that
fail that check
- kallsyms_lookup() never finds a symbol if CONFIG_KALLSYMS is not
selected
- more comments below
The one exception to this is init symbols. The patch also
checks to see whether __init memory has been freed and if
it has will allow kobjects in those sections.
Signed-off-by: Dave Hansen [EMAIL PROTECTED]
---
lxc-dave/arch/i386/kernel/vmlinux.lds.S |2 --
lxc-dave/include/linux/init.h |1 +
lxc-dave/init/main.c|9 +
lxc-dave/lib/kobject.c | 31 ++-
4 files changed, 32 insertions(+), 11 deletions(-)
diff -puN
lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup
lib/kobject.c
---
lxc/lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup
2007-08-22 14:51:50.0 -0700
+++ lxc-dave/lib/kobject.c 2007-08-22 14:51:50.0 -0700
@@ -139,23 +139,36 @@ static int ptr_in_range(void *ptr, void
return 0;
}
-static void verify_dynamic_kobject_allocation(struct kobject *kobj)
+void verify_dynamic_kobject_allocation(struct kobject *kobj)
{
- if (ptr_in_range(kobj, _sdata[0], _edata[0]))
- goto warn;
- if (ptr_in_range(kobj, __bss_start[0], __bss_stop[0]))
- goto warn;
- return;
-warn:
+ char *namebuf;
+ const char *ret;
+
+ namebuf = kzalloc(KSYM_NAME_LEN, GFP_KERNEL);
You don't need kzalloc here. kmalloc will do just fine.
+ ret = kallsyms_lookup((unsigned long)kobj, NULL, NULL, NULL,
+ namebuf);
+ /*
+* This is the X86_32-only part of this function.
+* This is here because it is valid to have a kobject
+* in an __init section, but only after those
+* sections have been freed back to the dynamic pool.
+*/
+ if (!initmem_now_dynamic
+ ptr_in_range(kobj, __init_begin, __init_end))
+ goto out;
+ if (!ret || !strlen(ret))
The !strlen(ret) is not only weird (why not write as !ret[0] or
!*ret) but is also unnecessary. When kallsyms_lookup fails to find a
symbol it should always return NULL.
+ goto out;
pr_debug( begin silly warning \n);
pr_debug(This is a janitorial warning, not a kernel bug.\n);
#ifdef CONFIG_DEBUG_KOBJECT
- print_symbol(The kobject at, or inside %s is not dynamically
allocated.\n,
- (unsigned long)kobj);
+ pr_debug(The kobject at, or inside '%s'@(0x%p) is not dynamically
allocated.\n,
+ namebuf, kobj);
#endif
pr_debug(kobjects must be dynamically allocated, not static\n);
/* dump_stack(); */
pr_debug( end silly warning \n);
+out:
+ kfree(namebuf);
}
#else
[...]
--
Paulo Marques - www.grupopie.com
You're just jealous because the voices only talk to me.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.22-rc6-mm1] kallsyms: make KSYM_NAME_LEN include space for trailing '\0'
Tejun Heo wrote: KSYM_NAME_LEN is peculiar in that it does not include the space for the trailing '\0', forcing all users to use KSYM_NAME_LEN + 1 when allocating buffer. This is nonsense and error-prone. Moreover, when the caller forgets that it's very likely to subtly bite back by corrupting the stack because the last position of the buffer is always cleared to zero. This patch increments KSYM_NAME_LEN by one and updates code accordingly. Nice work. I've been wanting to do that cleanup myself for a long time ;) Acked-by: Paulo Marques <[EMAIL PROTECTED]> -- Paulo Marques - www.grupopie.com "You're just jealous because the voices only talk to me." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.22-rc6-mm1] kallsyms: make KSYM_NAME_LEN include space for trailing '\0'
Tejun Heo wrote: KSYM_NAME_LEN is peculiar in that it does not include the space for the trailing '\0', forcing all users to use KSYM_NAME_LEN + 1 when allocating buffer. This is nonsense and error-prone. Moreover, when the caller forgets that it's very likely to subtly bite back by corrupting the stack because the last position of the buffer is always cleared to zero. This patch increments KSYM_NAME_LEN by one and updates code accordingly. Nice work. I've been wanting to do that cleanup myself for a long time ;) Acked-by: Paulo Marques [EMAIL PROTECTED] -- Paulo Marques - www.grupopie.com You're just jealous because the voices only talk to me. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] remove usage of memmem from scripts/kallsyms.c
Segher Boessenkool wrote: So we could remove the "#define _GNU_SOURCE" at the top of scripts/kallsyms.c too, presumably? If not (i.e. if there are more GNUisms left in that file anyway), then I'm not sure if we really gain by the change. yes, i believe this is true I only tried in on x86 with my toolchain and it works, but I don't know if it is worth the risk of breaking someone's setup for virtually no gain... With the memmem() removed, the code builds (and works) fine on several non-GNU systems. It should be perfectly safe to remove the _GNU_SOURCE. You're right. I went back in history and it was me who introduced the _GNU_SOURCE when I added the memmem too (shame on me). So, if it worked fine before, there is no reason to not work now that memmem is removed. So I can: - send an incremental patch with just that line removed - send a replacement patch - just leave it for now and wait until I work on kallsyms again to silently remove that line together with other changes Andrew, what would you prefer? -- Paulo Marques - www.grupopie.com "All I ask is a chance to prove that money can't make me happy." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] remove usage of memmem from scripts/kallsyms.c
Mike Frysinger wrote: On Tuesday 19 June 2007, Satyam Sharma wrote: So we could remove the "#define _GNU_SOURCE" at the top of scripts/kallsyms.c too, presumably? If not (i.e. if there are more GNUisms left in that file anyway), then I'm not sure if we really gain by the change. yes, i believe this is true I only tried in on x86 with my toolchain and it works, but I don't know if it is worth the risk of breaking someone's setup for virtually no gain... -- Paulo Marques - www.grupopie.com "God is real, unless declared integer." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] remove usage of memmem from scripts/kallsyms.c
Christoph Hellwig wrote: On Tue, Jun 19, 2007 at 05:15:56PM +0100, Paulo Marques wrote: The only in-kernel user of "memmem" is scripts/kallsyms.c and it only uses it to find tokens that are 2 bytes in size. It is trivial to replace it with a simple function that finds 2-byte tokens. This should help users from systems that don't have the memmem GNU extension available. Please add a comment describing why it's there so that it's not ripped out again by the first janitor looking over the code. I don't see why it would seem a good idea to replace a simple find_token function that searches for 2 byte tokens with a call to memmem. So, I think this is not something a janitor would do. The call to memmem was actually a left-over from a previous algorithm that used variable sized tokens. With fixed size, 2 byte tokens, having a specialized function is probably more efficient anyway. -- Paulo Marques - www.grupopie.com "Nostalgia isn't what it used to be." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] remove usage of memmem from scripts/kallsyms.c
Christoph Hellwig wrote: On Tue, Jun 19, 2007 at 05:15:56PM +0100, Paulo Marques wrote: The only in-kernel user of memmem is scripts/kallsyms.c and it only uses it to find tokens that are 2 bytes in size. It is trivial to replace it with a simple function that finds 2-byte tokens. This should help users from systems that don't have the memmem GNU extension available. Please add a comment describing why it's there so that it's not ripped out again by the first janitor looking over the code. I don't see why it would seem a good idea to replace a simple find_token function that searches for 2 byte tokens with a call to memmem. So, I think this is not something a janitor would do. The call to memmem was actually a left-over from a previous algorithm that used variable sized tokens. With fixed size, 2 byte tokens, having a specialized function is probably more efficient anyway. -- Paulo Marques - www.grupopie.com Nostalgia isn't what it used to be. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] remove usage of memmem from scripts/kallsyms.c
Mike Frysinger wrote: On Tuesday 19 June 2007, Satyam Sharma wrote: So we could remove the #define _GNU_SOURCE at the top of scripts/kallsyms.c too, presumably? If not (i.e. if there are more GNUisms left in that file anyway), then I'm not sure if we really gain by the change. yes, i believe this is true I only tried in on x86 with my toolchain and it works, but I don't know if it is worth the risk of breaking someone's setup for virtually no gain... -- Paulo Marques - www.grupopie.com God is real, unless declared integer. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] remove usage of memmem from scripts/kallsyms.c
Segher Boessenkool wrote: So we could remove the #define _GNU_SOURCE at the top of scripts/kallsyms.c too, presumably? If not (i.e. if there are more GNUisms left in that file anyway), then I'm not sure if we really gain by the change. yes, i believe this is true I only tried in on x86 with my toolchain and it works, but I don't know if it is worth the risk of breaking someone's setup for virtually no gain... With the memmem() removed, the code builds (and works) fine on several non-GNU systems. It should be perfectly safe to remove the _GNU_SOURCE. You're right. I went back in history and it was me who introduced the _GNU_SOURCE when I added the memmem too (shame on me). So, if it worked fine before, there is no reason to not work now that memmem is removed. So I can: - send an incremental patch with just that line removed - send a replacement patch - just leave it for now and wait until I work on kallsyms again to silently remove that line together with other changes Andrew, what would you prefer? -- Paulo Marques - www.grupopie.com All I ask is a chance to prove that money can't make me happy. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] remove usage of memmem from scripts/kallsyms.c
The only in-kernel user of "memmem" is scripts/kallsyms.c and it only
uses it to find tokens that are 2 bytes in size. It is trivial to
replace it with a simple function that finds 2-byte tokens.
This should help users from systems that don't have the memmem GNU
extension available.
Signed-off-by: Paulo Marques <[EMAIL PROTECTED]>
--
Paulo Marques - www.grupopie.com
"667: The neighbor of the beast."
--- ./scripts/kallsyms.c.orig 2007-06-08 12:55:49.0 +0100
+++ ./scripts/kallsyms.c2007-06-08 13:19:52.0 +0100
@@ -378,6 +378,17 @@ static void build_initial_tok_table(void
table_cnt = pos;
}
+static void *find_token(unsigned char *str, int len, unsigned char *token)
+{
+ int i;
+
+ for (i = 0; i < len - 1; i++) {
+ if (str[i] == token[0] && str[i+1] == token[1])
+ return [i];
+ }
+ return NULL;
+}
+
/* replace a given token in all the valid symbols. Use the sampled symbols
* to update the counts */
static void compress_symbols(unsigned char *str, int idx)
@@ -391,7 +402,7 @@ static void compress_symbols(unsigned ch
p1 = table[i].sym;
/* find the token on the symbol */
- p2 = memmem(p1, len, str, 2);
+ p2 = find_token(p1, len, str);
if (!p2) continue;
/* decrease the counts for this symbol's tokens */
@@ -410,7 +421,7 @@ static void compress_symbols(unsigned ch
if (size < 2) break;
/* find the token on the symbol */
- p2 = memmem(p1, size, str, 2);
+ p2 = find_token(p1, size, str);
} while (p2);
[PATCH] remove usage of memmem from scripts/kallsyms.c
The only in-kernel user of memmem is scripts/kallsyms.c and it only
uses it to find tokens that are 2 bytes in size. It is trivial to
replace it with a simple function that finds 2-byte tokens.
This should help users from systems that don't have the memmem GNU
extension available.
Signed-off-by: Paulo Marques [EMAIL PROTECTED]
--
Paulo Marques - www.grupopie.com
667: The neighbor of the beast.
--- ./scripts/kallsyms.c.orig 2007-06-08 12:55:49.0 +0100
+++ ./scripts/kallsyms.c2007-06-08 13:19:52.0 +0100
@@ -378,6 +378,17 @@ static void build_initial_tok_table(void
table_cnt = pos;
}
+static void *find_token(unsigned char *str, int len, unsigned char *token)
+{
+ int i;
+
+ for (i = 0; i len - 1; i++) {
+ if (str[i] == token[0] str[i+1] == token[1])
+ return str[i];
+ }
+ return NULL;
+}
+
/* replace a given token in all the valid symbols. Use the sampled symbols
* to update the counts */
static void compress_symbols(unsigned char *str, int idx)
@@ -391,7 +402,7 @@ static void compress_symbols(unsigned ch
p1 = table[i].sym;
/* find the token on the symbol */
- p2 = memmem(p1, len, str, 2);
+ p2 = find_token(p1, len, str);
if (!p2) continue;
/* decrease the counts for this symbol's tokens */
@@ -410,7 +421,7 @@ static void compress_symbols(unsigned ch
if (size 2) break;
/* find the token on the symbol */
- p2 = memmem(p1, size, str, 2);
+ p2 = find_token(p1, size, str);
} while (p2);
Re: Dual-Licensing Linux Kernel with GPL V2 and GPL V3
Alan Cox wrote: But COPYING *is* the entire text and starts with: " GNU GENERAL PUBLIC LICENSE Version 2, June 1991" so there is no confusion about the version. The version of the COPYING file (and the licence document), not of the licence on the code. Wrong. Why do you say "Wrong"? Have you contributed some code to the kernel thinking that the kernel was "v2 or later", only to find out later that it wasn't? A fair bit of the kernel is probably v2 or later but not all of it and that shouldn't really matter as regards the kernel anyway, the GPLv2 only bits (if v2 only is a valid status) anchor it. So we are violently agreeing, then? This sub-thread started by me showing that: $ find -name "*.c" | xargs grep "any later version" | wc -l 3138 $ find -name "*.c" | wc -l 9482 This is a somewhat crude measure but it shows that only about 30% of the kernel is "v2 or later" and those pieces could be used on some other "v2 or later" project (including v3). But the kernel as a whole is v2 and my point was that the claim that there are just a few "v2 only" files was bogus. -- Paulo Marques - www.grupopie.com "As far as we know, our computer has never had an undetected error." Weisert - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Dual-Licensing Linux Kernel with GPL V2 and GPL V3
Bernd Paysan wrote: On Friday 15 June 2007 13:49, Paulo Marques wrote: I've contributed some code for the kernel (unlike yourself, AFAICT), and believe me, I did so under GPL v2. The COPYING file is pretty much self explanatory, so I didn't need to add any explicit license statement to my code. It's not, it's a personal comment from a misunderstanding of the GPL text. It's as valid as the "closed source kernel modules are legal" comment that was there some years ago. These are not changes to the license text. These are just clarifications to help people understand the license. They don't change what the license already said. People seem to forget that the kernel license in COPYING *never had* the "v2 or later" clause. Never. Period. It's there in section 9. The section 9 is meant to explain how you select one version of the license in a program without having to copy the entire license text to it, i.e., in simple programs you can just put the small text, suggested by FSF at the bottom of the gpl, and have the version number there, and that should be enough to reference the entire text. But COPYING *is* the entire text and starts with: " GNU GENERAL PUBLIC LICENSE Version 2, June 1991" so there is no confusion about the version. [...] And people who write kernel code are perfectly aware that the kernel license is GPL v2 only, and always has been (except for the initial linus license). Wrong. Why do you say "Wrong"? Have you contributed some code to the kernel thinking that the kernel was "v2 or later", only to find out later that it wasn't? In case you haven't followed previous discussions, here's a pointer: http://lkml.org/lkml/2006/9/22/176 The major kernel developers (and probably most of the total number of developers) are perfectly aware of the kernel license and chose GPL v2. I'm getting pretty tired of listening to people that just _know_ what I should do with _my_ code. And people who treat kernel developers as morons who can't read a license. We definitely need more Al Viro style comments on this thread ;) -- Paulo Marques - www.grupopie.com "The Mexicans have the Chupacabra. We have Al Viro. If you hear him roar, just _pray_ he's about to dissect somebody elses code than yours.. There is no point in running." Linus Torvalds - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Dual-Licensing Linux Kernel with GPL V2 and GPL V3
Bernd Paysan wrote: On Thursday 14 June 2007 19:20, Paulo Marques wrote: Watching the output of the first grep without "wc -l" shows that, although it is not 100% accurate, it is still ok just to get a rough estimate. So yes, ~6300 files are definitely more than a couple ;) I knew I shouldn't post into the yearly GPL flame-fest... :( Most of them don't say anything, so they are "any GPL" by the author. I've contributed some code for the kernel (unlike yourself, AFAICT), and believe me, I did so under GPL v2. The COPYING file is pretty much self explanatory, so I didn't need to add any explicit license statement to my code. When do you people accept that Linus can't change the GPL, he can only add comments of what he thinks is the case! His interpretation of the GPLv2 might be that not saying anything about the version means "v2 only", but if he does so, he's simply wrong. He was wrong in the module case, as well, and dropped this comment a while ago. He might drop this comment in future, as well. In fact, anybody can drop this comment, as it's just a comment. Linus can't and is not _changing_ the GPL. He can however use whatever license he sees fit for _his_ code just like all the other kernel developers do. People seem to forget that the kernel license in COPYING *never had* the "v2 or later" clause. Never. Period. The only change in license was from the previous hand-made one from Linus into GPL v2 only. And that is perfectly fine since the previous license was even more permissive than GPL v2. The kernel *as a whole* is clearly under GPLv2 only from Linus' comment, which is in fact true, since the common subset of GPL versions from all authors is indeed GPLv2 (by virtue of some files from Al Viro, and maybe some other explicit GPL v2 files). The author must specify the version himself, there simply is no other way. If you don't specify any, it's "any version", because I can license all patches straight from the authors. No, it is not "any version". It is the license specified in COPYING and nothing else. The way the GPLv2 allows you to explicitely specify "any version" is by not saying anything about the version at all. Linus isn't in the positition to change that unless he does a substantial change to the file, and also adds a comment that this file is now GPLv2 only. Man, I sure ain't a lawyer, but people in these discussions seem to not understand the basics at all. And the basics are: "people who write the code decide the license to give it". And that's just it. And people who write kernel code are perfectly aware that the kernel license is GPL v2 only, and always has been (except for the initial linus license). So don't go around saying that because people don't put explicit license statements they don't care about the license. I care very much about the license, and would have never contributed to the kernel if it had a BSD license of some sort. Putting a license statement in _every_ file in the kernel tree would just be idiotic when there is such a clear COPYING file in the root of the kernel tree. -- Paulo Marques - www.grupopie.com "Oh dear, I think you'll find reality's on the blink again." Marvin The Paranoid Android - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Dual-Licensing Linux Kernel with GPL V2 and GPL V3
Bernd Paysan wrote: On Thursday 14 June 2007 19:20, Paulo Marques wrote: Watching the output of the first grep without wc -l shows that, although it is not 100% accurate, it is still ok just to get a rough estimate. So yes, ~6300 files are definitely more than a couple ;) I knew I shouldn't post into the yearly GPL flame-fest... :( Most of them don't say anything, so they are any GPL by the author. I've contributed some code for the kernel (unlike yourself, AFAICT), and believe me, I did so under GPL v2. The COPYING file is pretty much self explanatory, so I didn't need to add any explicit license statement to my code. When do you people accept that Linus can't change the GPL, he can only add comments of what he thinks is the case! His interpretation of the GPLv2 might be that not saying anything about the version means v2 only, but if he does so, he's simply wrong. He was wrong in the module case, as well, and dropped this comment a while ago. He might drop this comment in future, as well. In fact, anybody can drop this comment, as it's just a comment. Linus can't and is not _changing_ the GPL. He can however use whatever license he sees fit for _his_ code just like all the other kernel developers do. People seem to forget that the kernel license in COPYING *never had* the v2 or later clause. Never. Period. The only change in license was from the previous hand-made one from Linus into GPL v2 only. And that is perfectly fine since the previous license was even more permissive than GPL v2. The kernel *as a whole* is clearly under GPLv2 only from Linus' comment, which is in fact true, since the common subset of GPL versions from all authors is indeed GPLv2 (by virtue of some files from Al Viro, and maybe some other explicit GPL v2 files). The author must specify the version himself, there simply is no other way. If you don't specify any, it's any version, because I can license all patches straight from the authors. No, it is not any version. It is the license specified in COPYING and nothing else. The way the GPLv2 allows you to explicitely specify any version is by not saying anything about the version at all. Linus isn't in the positition to change that unless he does a substantial change to the file, and also adds a comment that this file is now GPLv2 only. Man, I sure ain't a lawyer, but people in these discussions seem to not understand the basics at all. And the basics are: people who write the code decide the license to give it. And that's just it. And people who write kernel code are perfectly aware that the kernel license is GPL v2 only, and always has been (except for the initial linus license). So don't go around saying that because people don't put explicit license statements they don't care about the license. I care very much about the license, and would have never contributed to the kernel if it had a BSD license of some sort. Putting a license statement in _every_ file in the kernel tree would just be idiotic when there is such a clear COPYING file in the root of the kernel tree. -- Paulo Marques - www.grupopie.com Oh dear, I think you'll find reality's on the blink again. Marvin The Paranoid Android - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Dual-Licensing Linux Kernel with GPL V2 and GPL V3
Bernd Paysan wrote: On Friday 15 June 2007 13:49, Paulo Marques wrote: I've contributed some code for the kernel (unlike yourself, AFAICT), and believe me, I did so under GPL v2. The COPYING file is pretty much self explanatory, so I didn't need to add any explicit license statement to my code. It's not, it's a personal comment from a misunderstanding of the GPL text. It's as valid as the closed source kernel modules are legal comment that was there some years ago. These are not changes to the license text. These are just clarifications to help people understand the license. They don't change what the license already said. People seem to forget that the kernel license in COPYING *never had* the v2 or later clause. Never. Period. It's there in section 9. The section 9 is meant to explain how you select one version of the license in a program without having to copy the entire license text to it, i.e., in simple programs you can just put the small text, suggested by FSF at the bottom of the gpl, and have the version number there, and that should be enough to reference the entire text. But COPYING *is* the entire text and starts with: GNU GENERAL PUBLIC LICENSE Version 2, June 1991 so there is no confusion about the version. [...] And people who write kernel code are perfectly aware that the kernel license is GPL v2 only, and always has been (except for the initial linus license). Wrong. Why do you say Wrong? Have you contributed some code to the kernel thinking that the kernel was v2 or later, only to find out later that it wasn't? In case you haven't followed previous discussions, here's a pointer: http://lkml.org/lkml/2006/9/22/176 The major kernel developers (and probably most of the total number of developers) are perfectly aware of the kernel license and chose GPL v2. I'm getting pretty tired of listening to people that just _know_ what I should do with _my_ code. And people who treat kernel developers as morons who can't read a license. We definitely need more Al Viro style comments on this thread ;) -- Paulo Marques - www.grupopie.com The Mexicans have the Chupacabra. We have Al Viro. If you hear him roar, just _pray_ he's about to dissect somebody elses code than yours.. There is no point in running. Linus Torvalds - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Dual-Licensing Linux Kernel with GPL V2 and GPL V3
Alan Cox wrote: But COPYING *is* the entire text and starts with: GNU GENERAL PUBLIC LICENSE Version 2, June 1991 so there is no confusion about the version. The version of the COPYING file (and the licence document), not of the licence on the code. Wrong. Why do you say Wrong? Have you contributed some code to the kernel thinking that the kernel was v2 or later, only to find out later that it wasn't? A fair bit of the kernel is probably v2 or later but not all of it and that shouldn't really matter as regards the kernel anyway, the GPLv2 only bits (if v2 only is a valid status) anchor it. So we are violently agreeing, then? This sub-thread started by me showing that: $ find -name *.c | xargs grep any later version | wc -l 3138 $ find -name *.c | wc -l 9482 This is a somewhat crude measure but it shows that only about 30% of the kernel is v2 or later and those pieces could be used on some other v2 or later project (including v3). But the kernel as a whole is v2 and my point was that the claim that there are just a few v2 only files was bogus. -- Paulo Marques - www.grupopie.com As far as we know, our computer has never had an undetected error. Weisert - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Dual-Licensing Linux Kernel with GPL V2 and GPL V3
Al Viro wrote: On Thu, Jun 14, 2007 at 01:01:20PM -0400, Dmitry Torokhov wrote: Multiple authors == need permission from each author with enough contributions to that file to make the contributions in question copyrightable. And in my case (and case of gregkh, and...) that would be considerably more than a couple of files. Really. I would expect that if you contribute to a file that explicitely says "GPL v2 or later" and you do not change that wording then you agree GPL v2 or later for that particular contribution. So for example drivers/net/plip.c could be changed to GPL v3 even though you contributed to it. After you exclude such cases it's still more than a couple of files... FWIW, $ find -name "*.c" | xargs grep "any later version" | wc -l 3138 $ find -name "*.c" | wc -l 9482 Watching the output of the first grep without "wc -l" shows that, although it is not 100% accurate, it is still ok just to get a rough estimate. So yes, ~6300 files are definitely more than a couple ;) -- Paulo Marques - www.grupopie.com "God is love. Love is blind. Ray Charles is blind. Ray Charles is God." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Dual-Licensing Linux Kernel with GPL V2 and GPL V3
Al Viro wrote: On Thu, Jun 14, 2007 at 01:01:20PM -0400, Dmitry Torokhov wrote: Multiple authors == need permission from each author with enough contributions to that file to make the contributions in question copyrightable. And in my case (and case of gregkh, and...) that would be considerably more than a couple of files. Really. I would expect that if you contribute to a file that explicitely says GPL v2 or later and you do not change that wording then you agree GPL v2 or later for that particular contribution. So for example drivers/net/plip.c could be changed to GPL v3 even though you contributed to it. After you exclude such cases it's still more than a couple of files... FWIW, $ find -name *.c | xargs grep any later version | wc -l 3138 $ find -name *.c | wc -l 9482 Watching the output of the first grep without wc -l shows that, although it is not 100% accurate, it is still ok just to get a rough estimate. So yes, ~6300 files are definitely more than a couple ;) -- Paulo Marques - www.grupopie.com God is love. Love is blind. Ray Charles is blind. Ray Charles is God. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: PC speaker
Maciej W. Rozycki wrote: On Tue, 12 Jun 2007, Jan Engelhardt wrote: 4) It assumes the current will be sufficient to burn out the speaker. (I know it will get very hot on older machines, whether it will burn out -- might even depend on the exact speaker model.) Since you can set the x86's crystals frequency from 1193182 to 18 Hz (PIT_TICK_RATE / 1 to PIT_TICK_RATE / 65535) [*], you can never really bust it. But even then, what would a speaker do it was constanly given I am fairly sure you have a choice between a steady low and a steady high level on the speaker output available if you switch the 8254 to the right single-shot mode. In case you have not been into such details -- the 8254 offers six modes of operation, selected for each channel separately, of which only two are periodic. Can we please stop this non-sense thread? Anyone designing the speaker circuit would certainly place a small capacitor in series with the speaker to kill the DC component of the signal. Since the speaker itself wouldn't be able to play very low frequencies anyway, the capacitor wouldn't have to be that big. So I'm pretty sure that on any half-way decent piece of hardware, you won't be able to kill the speaker with software... -- Paulo Marques - www.grupopie.com "Don't hit a man when he's down -- kick him; it's easier." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: PC speaker
Maciej W. Rozycki wrote: On Tue, 12 Jun 2007, Jan Engelhardt wrote: 4) It assumes the current will be sufficient to burn out the speaker. (I know it will get very hot on older machines, whether it will burn out -- might even depend on the exact speaker model.) Since you can set the x86's crystals frequency from 1193182 to 18 Hz (PIT_TICK_RATE / 1 to PIT_TICK_RATE / 65535) [*], you can never really bust it. But even then, what would a speaker do it was constanly given I am fairly sure you have a choice between a steady low and a steady high level on the speaker output available if you switch the 8254 to the right single-shot mode. In case you have not been into such details -- the 8254 offers six modes of operation, selected for each channel separately, of which only two are periodic. Can we please stop this non-sense thread? Anyone designing the speaker circuit would certainly place a small capacitor in series with the speaker to kill the DC component of the signal. Since the speaker itself wouldn't be able to play very low frequencies anyway, the capacitor wouldn't have to be that big. So I'm pretty sure that on any half-way decent piece of hardware, you won't be able to kill the speaker with software... -- Paulo Marques - www.grupopie.com Don't hit a man when he's down -- kick him; it's easier. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch/rfc] implement memmem() locally in kallsyms.c
Mike Frysinger wrote:
This patch basically copies the gnulib version of memmem() into
scripts/kallsyms.c. While a useful function, it isn't in POSIX so some
systems (like Darwin) choose to omit it. How do others feel ?
Well, the only use of memmem in scripts/kallsyms.c is to find tokens of
size 2, so we can use a simplified version instead (and probably get
better code in the process).
How about the attached patch instead?
If you approve it, I'll post it appropriately for inclusion in -mm.
--
Paulo Marques - www.grupopie.com
"God is real, unless declared integer."
--- ./scripts/kallsyms.c.orig 2007-06-08 12:55:49.0 +0100
+++ ./scripts/kallsyms.c2007-06-08 13:19:52.0 +0100
@@ -378,6 +378,17 @@ static void build_initial_tok_table(void
table_cnt = pos;
}
+static void *find_token(unsigned char *str, int len, unsigned char *token)
+{
+ int i;
+
+ for (i = 0; i < len - 1; i++) {
+ if (str[i] == token[0] && str[i+1] == token[1])
+ return [i];
+ }
+ return NULL;
+}
+
/* replace a given token in all the valid symbols. Use the sampled symbols
* to update the counts */
static void compress_symbols(unsigned char *str, int idx)
@@ -391,7 +402,7 @@ static void compress_symbols(unsigned ch
p1 = table[i].sym;
/* find the token on the symbol */
- p2 = memmem(p1, len, str, 2);
+ p2 = find_token(p1, len, str);
if (!p2) continue;
/* decrease the counts for this symbol's tokens */
@@ -410,7 +421,7 @@ static void compress_symbols(unsigned ch
if (size < 2) break;
/* find the token on the symbol */
- p2 = memmem(p1, size, str, 2);
+ p2 = find_token(p1, size, str);
} while (p2);
Re: [patch/rfc] implement memmem() locally in kallsyms.c
Mike Frysinger wrote:
This patch basically copies the gnulib version of memmem() into
scripts/kallsyms.c. While a useful function, it isn't in POSIX so some
systems (like Darwin) choose to omit it. How do others feel ?
Well, the only use of memmem in scripts/kallsyms.c is to find tokens of
size 2, so we can use a simplified version instead (and probably get
better code in the process).
How about the attached patch instead?
If you approve it, I'll post it appropriately for inclusion in -mm.
--
Paulo Marques - www.grupopie.com
God is real, unless declared integer.
--- ./scripts/kallsyms.c.orig 2007-06-08 12:55:49.0 +0100
+++ ./scripts/kallsyms.c2007-06-08 13:19:52.0 +0100
@@ -378,6 +378,17 @@ static void build_initial_tok_table(void
table_cnt = pos;
}
+static void *find_token(unsigned char *str, int len, unsigned char *token)
+{
+ int i;
+
+ for (i = 0; i len - 1; i++) {
+ if (str[i] == token[0] str[i+1] == token[1])
+ return str[i];
+ }
+ return NULL;
+}
+
/* replace a given token in all the valid symbols. Use the sampled symbols
* to update the counts */
static void compress_symbols(unsigned char *str, int idx)
@@ -391,7 +402,7 @@ static void compress_symbols(unsigned ch
p1 = table[i].sym;
/* find the token on the symbol */
- p2 = memmem(p1, len, str, 2);
+ p2 = find_token(p1, len, str);
if (!p2) continue;
/* decrease the counts for this symbol's tokens */
@@ -410,7 +421,7 @@ static void compress_symbols(unsigned ch
if (size 2) break;
/* find the token on the symbol */
- p2 = memmem(p1, size, str, 2);
+ p2 = find_token(p1, size, str);
} while (p2);
Re: [Patch 05/18] fs/logfs/logfs.h
Jörn Engel wrote: --- /dev/null 2007-03-13 19:15:28.862769062 +0100 +++ linux-2.6.21logfs/fs/logfs/logfs.h 2007-06-03 19:34:07.0 +0200 @@ -0,0 +1,398 @@ +/* + * fs/logfs/logfs.h + * + * As should be obvious for Linux kernel code, license is GPLv2 + * + * Copyright (c) 2005-2007 Joern Engel + * + * Private header for logfs. + */ +#ifndef fs_logfs_logfs_h +#define fs_logfs_logfs_h + +#define __CHECK_ENDIAN__ + + +#include +#include +#include Including kallsyms.h is not really needed in this header file, is it? I don't have time / knowledge to review the rest of the code, but I always keep an eye out for kallsyms usage... -- Paulo Marques - www.grupopie.com "Very funny Scotty. Now beam up my clothes." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 05/18] fs/logfs/logfs.h
Jörn Engel wrote: --- /dev/null 2007-03-13 19:15:28.862769062 +0100 +++ linux-2.6.21logfs/fs/logfs/logfs.h 2007-06-03 19:34:07.0 +0200 @@ -0,0 +1,398 @@ +/* + * fs/logfs/logfs.h + * + * As should be obvious for Linux kernel code, license is GPLv2 + * + * Copyright (c) 2005-2007 Joern Engel + * + * Private header for logfs. + */ +#ifndef fs_logfs_logfs_h +#define fs_logfs_logfs_h + +#define __CHECK_ENDIAN__ + + +#include linux/crc32.h +#include linux/fs.h +#include linux/kallsyms.h Including kallsyms.h is not really needed in this header file, is it? I don't have time / knowledge to review the rest of the code, but I always keep an eye out for kallsyms usage... -- Paulo Marques - www.grupopie.com Very funny Scotty. Now beam up my clothes. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 1/1] crypto API: RSA algorithm patch (kernel version 2.6.20.1)
Matt Mackall wrote: [...] +/* Allocate space for the mpi and its data */ +s = (size / 4) + ((size % 4)? 1: 0); Uhhh.. (size + 1) / 4? You mean "(size + 3) / 4", no? Overall, I agree with your comments: this file looks like it needs a lot more CodingStyle ;) Redefining standard kernel interfaces (error numbers, memory allocation, printk, etc.) is not the right way to get code merged. -- Paulo Marques - www.grupopie.com "For every problem there is one solution which is simple, neat, and wrong." H. L. Mencken - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 1/1] crypto API: RSA algorithm patch (kernel version 2.6.20.1)
Matt Mackall wrote: [...] +/* Allocate space for the mpi and its data */ +s = (size / 4) + ((size % 4)? 1: 0); Uhhh.. (size + 1) / 4? You mean (size + 3) / 4, no? Overall, I agree with your comments: this file looks like it needs a lot more CodingStyle ;) Redefining standard kernel interfaces (error numbers, memory allocation, printk, etc.) is not the right way to get code merged. -- Paulo Marques - www.grupopie.com For every problem there is one solution which is simple, neat, and wrong. H. L. Mencken - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] Race between cat /proc/kallsyms and rmmod
Alexey Dobriyan wrote: Iterating code of /proc/kallsyms calls module_get_kallsym() which grabs and drops module_mutex internally and returns "struct module *", module is removed, aforementioned "struct module *" is used in non-trivial way. Steps to reproduce: modprobe/rmmod loop cat /proc/kallsyms >/dev/null loop Copy all needed info under module_mutex. NOTE: this patch keeps module_mutex static. Yes, this patch fixes the "cat /proc/kallsyms" race without changing any "external" interfaces, so I think it should go into mainline in any case. Acked-by: Paulo Marques <[EMAIL PROTECTED]> -- Paulo Marques - www.grupopie.com "All I ask is a chance to prove that money can't make me happy." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Alexey Dobriyan wrote: On Sat, Mar 17, 2007 at 08:37:18PM +1100, Rusty Russell wrote: On Fri, 2007-03-16 at 12:51 +0100, Ingo Molnar wrote: [...] looking at the problem from another angle: wouldnt this be something that would benefit from freeze_processes()/unfreeze_processes(), and hence no locking would be required? Actually, the list manipulation is done with stop_machine for this reason. mmm, my changelog is slightly narrow than it should be. Non-emergency code is traversing modules list. It finds "struct module *". module is removed. "struct module *" is now meaningless, but still dereferenced. How would all this refrigerator stuff would help? It wouldn't, Non-emergency code is traversing modules list. It finds "struct module *". Everything is freezed. Module is removed. Everything is unfreezed. "struct module *" is now meaningless, but still dereferenced. That is why I asked if the refrigerator would preempt processes or not. AFAICS there is no path where the "struct module *" that is returned is used after a voluntary preemption point, so it should be safe. I might be missing something, though. However, this isn't very robust. Since the functions are still returning pointers to module data, some changes in the future might keep the pointer, and use it after a valid freezing point -> oops. Alexey, is preempt enabled in your kernel? Yes. FWIW, CONFIG_PREEMPT=y CONFIG_PREEMPT_BKL=y CONFIG_DEBUG_PREEMPT=y I very much agree with proto-patch which _copies_ all relevant information into caller-supplied structure, keeping module_mutex private. Time to split it sanely. That depends on the roadmap: if we think the freezer approach is the best in the long run, maybe your less intrusive (in the sense that it changes less stuff) patch should go in now (as a quick fix to mainline) so that after we've sorted out the bugs from the freezer in -mm, it will be easier to revert. However, if we think the most reliable solution would be to not return internal module information through pointers and keep all that logic internal to module.c, then the "proto-patch" with some improvements might be the way to go... -- Paulo Marques - www.grupopie.com "God is love. Love is blind. Ray Charles is blind. Ray Charles is God." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Alexey Dobriyan wrote: On Sat, Mar 17, 2007 at 08:37:18PM +1100, Rusty Russell wrote: On Fri, 2007-03-16 at 12:51 +0100, Ingo Molnar wrote: [...] looking at the problem from another angle: wouldnt this be something that would benefit from freeze_processes()/unfreeze_processes(), and hence no locking would be required? Actually, the list manipulation is done with stop_machine for this reason. mmm, my changelog is slightly narrow than it should be. Non-emergency code is traversing modules list. It finds struct module *. module is removed. struct module * is now meaningless, but still dereferenced. How would all this refrigerator stuff would help? It wouldn't, Non-emergency code is traversing modules list. It finds struct module *. Everything is freezed. Module is removed. Everything is unfreezed. struct module * is now meaningless, but still dereferenced. That is why I asked if the refrigerator would preempt processes or not. AFAICS there is no path where the struct module * that is returned is used after a voluntary preemption point, so it should be safe. I might be missing something, though. However, this isn't very robust. Since the functions are still returning pointers to module data, some changes in the future might keep the pointer, and use it after a valid freezing point - oops. Alexey, is preempt enabled in your kernel? Yes. FWIW, CONFIG_PREEMPT=y CONFIG_PREEMPT_BKL=y CONFIG_DEBUG_PREEMPT=y I very much agree with proto-patch which _copies_ all relevant information into caller-supplied structure, keeping module_mutex private. Time to split it sanely. That depends on the roadmap: if we think the freezer approach is the best in the long run, maybe your less intrusive (in the sense that it changes less stuff) patch should go in now (as a quick fix to mainline) so that after we've sorted out the bugs from the freezer in -mm, it will be easier to revert. However, if we think the most reliable solution would be to not return internal module information through pointers and keep all that logic internal to module.c, then the proto-patch with some improvements might be the way to go... -- Paulo Marques - www.grupopie.com God is love. Love is blind. Ray Charles is blind. Ray Charles is God. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] Race between cat /proc/kallsyms and rmmod
Alexey Dobriyan wrote: Iterating code of /proc/kallsyms calls module_get_kallsym() which grabs and drops module_mutex internally and returns struct module *, module is removed, aforementioned struct module * is used in non-trivial way. Steps to reproduce: modprobe/rmmod loop cat /proc/kallsyms /dev/null loop Copy all needed info under module_mutex. NOTE: this patch keeps module_mutex static. Yes, this patch fixes the cat /proc/kallsyms race without changing any external interfaces, so I think it should go into mainline in any case. Acked-by: Paulo Marques [EMAIL PROTECTED] -- Paulo Marques - www.grupopie.com All I ask is a chance to prove that money can't make me happy. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Andrew Morton wrote:
On Fri, 16 Mar 2007 17:16:39 + Paulo Marques <[EMAIL PROTECTED]> wrote:
Does freeze_processes() / unfreeze_processes() solve this by only
freezing processes that have voluntarily scheduled (opposed to just
being preempted)?
It goes much much further than that. Those processes need to actually
perform an explicit call to try_to_freeze().
Ok, I've just done a few tests with the attached patch. It basically
creates a freeze_machine_run function that is equivalent in interface to
stop_machine_run, but uses freeze_processes / thaw_processes to stop the
machine.
This is more of a proof of concept than an actual patch. At the very
least "freeze_machine_run" should be moved to kernel/power/process.c and
declared at include/linux/freezer.h so that it could be treated as a
more general purpose function and not something that is module specific.
Anyway, I then tested it by running a modprobe/rmmod loop while running
a "cat /proc/kallsyms" loop.
On the first run I forgot to remove the mutex_lock(module_mutex) from
the /proc/kallsyms read path and the freezer was unable to freeze the
"cat" process that was waiting for the same mutex that the freezer
process was holding :P
After removing the module_mutex locking from "module_get_kallsym"
everything was going fine (at least I got no oopses) until I got this:
kernel: Stopping user space processes timed out after 20 seconds (1
tasks refusing to freeze):
kernel: kbluetoothd
kernel: Restarting tasks ... <4> Strange, kseriod not stopped
kernel: Strange, pdflush not stopped
kernel: Strange, pdflush not stopped
kernel: Strange, kswapd0 not stopped
kernel: Strange, cifsoplockd not stopped
kernel: Strange, cifsdnotifyd not stopped
kernel: Strange, jfsIO not stopped
kernel: Strange, jfsCommit not stopped
kernel: Strange, jfsCommit not stopped
kernel: Strange, jfsSync not stopped
kernel: Strange, xfslogd/0 not stopped
kernel: Strange, xfslogd/1 not stopped
kernel: Strange, xfsdatad/0 not stopped
kernel: Strange, xfsdatad/1 not stopped
kernel: Strange, kjournald not stopped
kernel: Strange, khubd not stopped
kernel: Strange, khelper not stopped
kernel: Strange, kbluetoothd not stopped
kernel: done.
I repeated the test and did a Alt+SysRq+T to try to find out what
kbluetoothd was doing and got this:
kernel: kbluetoothd D 79A11860 0 19156 1 19142
(NOTLB)
kernel: 9a269e4c 0082 0001 79a11860 79a09860 c7018030
0003
kernel: 9a269e71 78475100 c7ebe000 c6730e40 0001 0001
0001
kernel: 9a2d7570 79a11860 c7018140 1832 42430d03
00ab
kernel: Call Trace:
kernel: [<7845dba3>] wait_for_completion+0x7d/0xb7
kernel: [<781190ba>] default_wake_function+0x0/0xc
kernel: [<781190ba>] default_wake_function+0x0/0xc
kernel: [<7812c759>] call_usermodehelper_keys+0xd1/0xf1
kernel: [<7812c41e>] request_module+0x96/0xd9
kernel: [<783e30fe>] sock_alloc_inode+0x20/0x4e
kernel: [<78172559>] alloc_inode+0x15/0x115
kernel: [<78172d87>] new_inode+0x24/0x81
kernel: [<783e4003>] __sock_create+0x111/0x199
kernel: [<783e40a3>] sock_create+0x18/0x1d
kernel: [<783e40e1>] sys_socket+0x1c/0x43
kernel: [<783e51da>] sys_socketcall+0x247/0x24c
kernel: [<78121b2d>] sys_gettimeofday+0x2c/0x65
kernel: [<78103f10>] sysenter_past_esp+0x5d/0x81
And this was as far as I got...
This actually seems like a better approach than to hold module_mutex
everywhere to account for an operation that should be "rare" (module
loading/unloading). If something like this goes in, there are probably a
few more places inside module.c where we can drop the locking completely.
However, it still has a few gotchas. Apart from the problem above (which
may still be me doing something wrong) it makes module loading /
unloading depend on CONFIG_PM which is somewhat unexpected for the user.
Would it make sense to separate the process freezing / thawing API from
actual power management and create a new config option (CONFIG_FREEZER?)
that was automatically selected by the systems that used it (CONFIG_PM,
CONFIG_MODULES, etc.)? or is that overkill?
--
Paulo Marques - www.grupopie.com
"Nostalgia isn't what it used to be."
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -35,7 +35,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
@@ -618,13 +618,23 @@ static int __try_stop_module(void *_sref
return 0;
}
+static int freeze_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
+{
+ int ret;
+ freeze_processes();
+ ret = fn(data);
+ thaw_processes();
+ return ret;
+}
+
static int try_stop_module(struct module *mod, int flags, int *forced)
{
struct stopref sref = { mod, flags, forced };
- return stop_machin
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Ingo Molnar wrote:
* Paulo Marques <[EMAIL PROTECTED]> wrote:
looking at the problem from another angle: wouldnt this be something
that would benefit from freeze_processes()/unfreeze_processes(), and
hence no locking would be required?
I also considered this, but it seemed a little too "blunt" to stop
everything (including completely unrelated processes and kernel
threads) just to remove a module.
'just to remove a module' is very, very rare, on the timescale of most
kernel ops. Almost no distro does it. Furthermore, because we want to do
CPU-hotplug that way, we really want to make
freeze_processes()/unfreeze_processes() 'instantaneous' to the human -
and it is that already. (if it isnt in some case we can make it so)
Ok. I started to look at this approach and realized that module.c
already does this:
static int __unlink_module(void *_mod)
{
struct module *mod = _mod;
list_del(>list);
return 0;
}
/* Free a module, remove from lists, etc (must hold module mutex). */
static void free_module(struct module *mod)
{
/* Delete from various lists */
stop_machine_run(__unlink_module, mod, NR_CPUS);
However stop_machine_run doesn't seem like the right thing to do,
because users of the "modules" list don't seem to do anything to prevent
preemption. Am I missing something?
Does freeze_processes() / unfreeze_processes() solve this by only
freezing processes that have voluntarily scheduled (opposed to just
being preempted)?
--
Paulo Marques - www.grupopie.com
"The Computer made me do it."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Ingo Molnar wrote:
* Alexey Dobriyan <[EMAIL PROTECTED]> wrote:
[cc'ing folks whose proc files are affected]
kallsyms_lookup() can call module_address_lookup() which iterates over
modules list without module_mutex taken. Comment at the top of
module_address_lookup() says it's for oops resolution so races are
irrelevant, but in some cases it's reachable from regular code:
looking at the problem from another angle: wouldnt this be something
that would benefit from freeze_processes()/unfreeze_processes(), and
hence no locking would be required?
I also considered this, but it seemed a little too "blunt" to stop
everything (including completely unrelated processes and kernel threads)
just to remove a module.
How about something like this instead? (just for review)
It's a little more intrusive, since the interface for symbol lookup is
changed (and it affects the callers), but it makes the caller aware that
it should set "safe_to_lock" if possible.
This way we avoid exposing module_mutex outside of module.c and avoid
returning any data pointer to some data structure that might disappear
before the caller tries to use it.
Some of these changes are actually cleanups, because the callers where
creating a dummy modname variables that they didn't use afterwards.
The thing I like the less about this patch is the increase of stack
usage on some code paths by about 60 bytes.
Anyway, I don't have very strong feelings about this, so if you think it
would be better to use freeze_processes()/unfreeze_processes(), I can
cook up a patch for that too...
--
Paulo Marques - www.grupopie.com
"Very funny Scotty. Now beam up my clothes."
diffstat:
arch/parisc/kernel/unwind.c |3 --
arch/powerpc/xmon/xmon.c| 11 -
arch/ppc/xmon/xmon.c|8 +++---
arch/sh64/kernel/unwind.c |4 +--
arch/x86_64/kernel/traps.c | 10
fs/proc/base.c |3 --
include/linux/kallsyms.h|6 +++-
include/linux/module.h | 44 +++-
kernel/kallsyms.c | 53 +---
kernel/kprobes.c|6 ++--
kernel/lockdep.c|3 --
kernel/module.c | 44 +++-
kernel/time/timer_list.c|3 --
kernel/time/timer_stats.c |3 --
mm/slab.c |6 ++--
15 files changed, 114 insertions(+), 93 deletions(-)
diff --git a/arch/parisc/kernel/unwind.c b/arch/parisc/kernel/unwind.c
--- a/arch/parisc/kernel/unwind.c
+++ b/arch/parisc/kernel/unwind.c
@@ -216,11 +216,10 @@ static void unwind_frame_regs(struct unw
/* Handle some frequent special cases */
{
char symname[KSYM_NAME_LEN+1];
- char *modname;
unsigned long symsize, offset;
kallsyms_lookup(info->ip, , ,
- , symname);
+ NULL, symname, 0);
dbg("info->ip = 0x%lx, name = %s\n", info->ip, symname);
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1218,7 +1218,6 @@ static void get_function_bounds(unsigned
{
unsigned long size, offset;
const char *name;
- char *modname;
*startp = *endp = 0;
if (pc == 0)
@@ -1226,7 +1225,7 @@ static void get_function_bounds(unsigned
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
- name = kallsyms_lookup(pc, , , , tmpstr);
+ name = kallsyms_lookup(pc, , , NULL, tmpstr, 0);
if (name != NULL) {
*startp = pc - offset;
*endp = pc - offset + size;
@@ -2496,7 +2495,7 @@ symbol_lookup(void)
static void xmon_print_symbol(unsigned long address, const char *mid,
const char *after)
{
- char *modname;
+ char modname[MODULE_NAME_LEN + 1];
const char *name = NULL;
unsigned long offset, size;
@@ -2504,8 +2503,8 @@ static void xmon_print_symbol(unsigned l
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
- name = kallsyms_lookup(address, , , ,
- tmpstr);
+ name = kallsyms_lookup(address, , , modname,
+ tmpstr, 0);
sync();
/* wait a little while to see if we get a machine check */
__delay(200);
@@ -2515,7 +2514,7 @@ static void xmon_print_symbol(unsigned l
if (name) {
printf("%s%s+%#lx/%#lx", mid, name, offset, size);
- if (modname)
+
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Ingo Molnar wrote:
* Alexey Dobriyan [EMAIL PROTECTED] wrote:
[cc'ing folks whose proc files are affected]
kallsyms_lookup() can call module_address_lookup() which iterates over
modules list without module_mutex taken. Comment at the top of
module_address_lookup() says it's for oops resolution so races are
irrelevant, but in some cases it's reachable from regular code:
looking at the problem from another angle: wouldnt this be something
that would benefit from freeze_processes()/unfreeze_processes(), and
hence no locking would be required?
I also considered this, but it seemed a little too blunt to stop
everything (including completely unrelated processes and kernel threads)
just to remove a module.
How about something like this instead? (just for review)
It's a little more intrusive, since the interface for symbol lookup is
changed (and it affects the callers), but it makes the caller aware that
it should set safe_to_lock if possible.
This way we avoid exposing module_mutex outside of module.c and avoid
returning any data pointer to some data structure that might disappear
before the caller tries to use it.
Some of these changes are actually cleanups, because the callers where
creating a dummy modname variables that they didn't use afterwards.
The thing I like the less about this patch is the increase of stack
usage on some code paths by about 60 bytes.
Anyway, I don't have very strong feelings about this, so if you think it
would be better to use freeze_processes()/unfreeze_processes(), I can
cook up a patch for that too...
--
Paulo Marques - www.grupopie.com
Very funny Scotty. Now beam up my clothes.
diffstat:
arch/parisc/kernel/unwind.c |3 --
arch/powerpc/xmon/xmon.c| 11 -
arch/ppc/xmon/xmon.c|8 +++---
arch/sh64/kernel/unwind.c |4 +--
arch/x86_64/kernel/traps.c | 10
fs/proc/base.c |3 --
include/linux/kallsyms.h|6 +++-
include/linux/module.h | 44 +++-
kernel/kallsyms.c | 53 +---
kernel/kprobes.c|6 ++--
kernel/lockdep.c|3 --
kernel/module.c | 44 +++-
kernel/time/timer_list.c|3 --
kernel/time/timer_stats.c |3 --
mm/slab.c |6 ++--
15 files changed, 114 insertions(+), 93 deletions(-)
diff --git a/arch/parisc/kernel/unwind.c b/arch/parisc/kernel/unwind.c
--- a/arch/parisc/kernel/unwind.c
+++ b/arch/parisc/kernel/unwind.c
@@ -216,11 +216,10 @@ static void unwind_frame_regs(struct unw
/* Handle some frequent special cases */
{
char symname[KSYM_NAME_LEN+1];
- char *modname;
unsigned long symsize, offset;
kallsyms_lookup(info-ip, symsize, offset,
- modname, symname);
+ NULL, symname, 0);
dbg(info-ip = 0x%lx, name = %s\n, info-ip, symname);
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1218,7 +1218,6 @@ static void get_function_bounds(unsigned
{
unsigned long size, offset;
const char *name;
- char *modname;
*startp = *endp = 0;
if (pc == 0)
@@ -1226,7 +1225,7 @@ static void get_function_bounds(unsigned
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
- name = kallsyms_lookup(pc, size, offset, modname, tmpstr);
+ name = kallsyms_lookup(pc, size, offset, NULL, tmpstr, 0);
if (name != NULL) {
*startp = pc - offset;
*endp = pc - offset + size;
@@ -2496,7 +2495,7 @@ symbol_lookup(void)
static void xmon_print_symbol(unsigned long address, const char *mid,
const char *after)
{
- char *modname;
+ char modname[MODULE_NAME_LEN + 1];
const char *name = NULL;
unsigned long offset, size;
@@ -2504,8 +2503,8 @@ static void xmon_print_symbol(unsigned l
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
- name = kallsyms_lookup(address, size, offset, modname,
- tmpstr);
+ name = kallsyms_lookup(address, size, offset, modname,
+ tmpstr, 0);
sync();
/* wait a little while to see if we get a machine check */
__delay(200);
@@ -2515,7 +2514,7 @@ static void xmon_print_symbol(unsigned l
if (name) {
printf(%s%s+%#lx/%#lx, mid, name, offset, size);
- if (modname)
+ if (modname[0
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Ingo Molnar wrote:
* Paulo Marques [EMAIL PROTECTED] wrote:
looking at the problem from another angle: wouldnt this be something
that would benefit from freeze_processes()/unfreeze_processes(), and
hence no locking would be required?
I also considered this, but it seemed a little too blunt to stop
everything (including completely unrelated processes and kernel
threads) just to remove a module.
'just to remove a module' is very, very rare, on the timescale of most
kernel ops. Almost no distro does it. Furthermore, because we want to do
CPU-hotplug that way, we really want to make
freeze_processes()/unfreeze_processes() 'instantaneous' to the human -
and it is that already. (if it isnt in some case we can make it so)
Ok. I started to look at this approach and realized that module.c
already does this:
static int __unlink_module(void *_mod)
{
struct module *mod = _mod;
list_del(mod-list);
return 0;
}
/* Free a module, remove from lists, etc (must hold module mutex). */
static void free_module(struct module *mod)
{
/* Delete from various lists */
stop_machine_run(__unlink_module, mod, NR_CPUS);
However stop_machine_run doesn't seem like the right thing to do,
because users of the modules list don't seem to do anything to prevent
preemption. Am I missing something?
Does freeze_processes() / unfreeze_processes() solve this by only
freezing processes that have voluntarily scheduled (opposed to just
being preempted)?
--
Paulo Marques - www.grupopie.com
The Computer made me do it.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Andrew Morton wrote:
On Fri, 16 Mar 2007 17:16:39 + Paulo Marques [EMAIL PROTECTED] wrote:
Does freeze_processes() / unfreeze_processes() solve this by only
freezing processes that have voluntarily scheduled (opposed to just
being preempted)?
It goes much much further than that. Those processes need to actually
perform an explicit call to try_to_freeze().
Ok, I've just done a few tests with the attached patch. It basically
creates a freeze_machine_run function that is equivalent in interface to
stop_machine_run, but uses freeze_processes / thaw_processes to stop the
machine.
This is more of a proof of concept than an actual patch. At the very
least freeze_machine_run should be moved to kernel/power/process.c and
declared at include/linux/freezer.h so that it could be treated as a
more general purpose function and not something that is module specific.
Anyway, I then tested it by running a modprobe/rmmod loop while running
a cat /proc/kallsyms loop.
On the first run I forgot to remove the mutex_lock(module_mutex) from
the /proc/kallsyms read path and the freezer was unable to freeze the
cat process that was waiting for the same mutex that the freezer
process was holding :P
After removing the module_mutex locking from module_get_kallsym
everything was going fine (at least I got no oopses) until I got this:
kernel: Stopping user space processes timed out after 20 seconds (1
tasks refusing to freeze):
kernel: kbluetoothd
kernel: Restarting tasks ... 4 Strange, kseriod not stopped
kernel: Strange, pdflush not stopped
kernel: Strange, pdflush not stopped
kernel: Strange, kswapd0 not stopped
kernel: Strange, cifsoplockd not stopped
kernel: Strange, cifsdnotifyd not stopped
kernel: Strange, jfsIO not stopped
kernel: Strange, jfsCommit not stopped
kernel: Strange, jfsCommit not stopped
kernel: Strange, jfsSync not stopped
kernel: Strange, xfslogd/0 not stopped
kernel: Strange, xfslogd/1 not stopped
kernel: Strange, xfsdatad/0 not stopped
kernel: Strange, xfsdatad/1 not stopped
kernel: Strange, kjournald not stopped
kernel: Strange, khubd not stopped
kernel: Strange, khelper not stopped
kernel: Strange, kbluetoothd not stopped
kernel: done.
I repeated the test and did a Alt+SysRq+T to try to find out what
kbluetoothd was doing and got this:
kernel: kbluetoothd D 79A11860 0 19156 1 19142
(NOTLB)
kernel: 9a269e4c 0082 0001 79a11860 79a09860 c7018030
0003
kernel: 9a269e71 78475100 c7ebe000 c6730e40 0001 0001
0001
kernel: 9a2d7570 79a11860 c7018140 1832 42430d03
00ab
kernel: Call Trace:
kernel: [7845dba3] wait_for_completion+0x7d/0xb7
kernel: [781190ba] default_wake_function+0x0/0xc
kernel: [781190ba] default_wake_function+0x0/0xc
kernel: [7812c759] call_usermodehelper_keys+0xd1/0xf1
kernel: [7812c41e] request_module+0x96/0xd9
kernel: [783e30fe] sock_alloc_inode+0x20/0x4e
kernel: [78172559] alloc_inode+0x15/0x115
kernel: [78172d87] new_inode+0x24/0x81
kernel: [783e4003] __sock_create+0x111/0x199
kernel: [783e40a3] sock_create+0x18/0x1d
kernel: [783e40e1] sys_socket+0x1c/0x43
kernel: [783e51da] sys_socketcall+0x247/0x24c
kernel: [78121b2d] sys_gettimeofday+0x2c/0x65
kernel: [78103f10] sysenter_past_esp+0x5d/0x81
And this was as far as I got...
This actually seems like a better approach than to hold module_mutex
everywhere to account for an operation that should be rare (module
loading/unloading). If something like this goes in, there are probably a
few more places inside module.c where we can drop the locking completely.
However, it still has a few gotchas. Apart from the problem above (which
may still be me doing something wrong) it makes module loading /
unloading depend on CONFIG_PM which is somewhat unexpected for the user.
Would it make sense to separate the process freezing / thawing API from
actual power management and create a new config option (CONFIG_FREEZER?)
that was automatically selected by the systems that used it (CONFIG_PM,
CONFIG_MODULES, etc.)? or is that overkill?
--
Paulo Marques - www.grupopie.com
Nostalgia isn't what it used to be.
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -35,7 +35,7 @@
#include linux/vermagic.h
#include linux/notifier.h
#include linux/sched.h
-#include linux/stop_machine.h
+#include linux/freezer.h
#include linux/device.h
#include linux/string.h
#include linux/mutex.h
@@ -618,13 +618,23 @@ static int __try_stop_module(void *_sref
return 0;
}
+static int freeze_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
+{
+ int ret;
+ freeze_processes();
+ ret = fn(data);
+ thaw_processes();
+ return ret;
+}
+
static int try_stop_module(struct module *mod, int flags, int *forced)
{
struct stopref sref = { mod, flags, forced };
- return stop_machine_run(__try_stop_module, sref, NR_CPUS);
+ return freeze_machine_run
Re: [PATCH] Fix some kallsyms_lookup() vs rmmod races
Alexey Dobriyan wrote: [cc'ing folks whose proc files are affected] kallsyms_lookup() can call module_address_lookup() which iterates over modules list without module_mutex taken. Comment at the top of module_address_lookup() says it's for oops resolution so races are irrelevant, but in some cases it's reachable from regular code: So maybe we should just add a new parameter to "kallsyms_lookup" to inform it if it is safe to take a mutex or not. Spreading module_mutex everywhere doesn't seem like the right interface for several reasons: - new users of "kallsyms_lookup" might not be aware that they should take module_mutex if it is safe - many times we will be taking module_mutex even when we are fetching a kernel symbol that shouldn't require the mutex at all - it just creates new dependencies (hint: this patch shouldn't even compile with current git since module_mutex is not declared in module.h, not to mention compile when CONFIG_MODULES not set) IMHO we should not expose module_mutex outside of module.c. That is just wrong from an encapsulation point of view. -- Paulo Marques - www.grupopie.com "667: The neighbor of the beast." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix some kallsyms_lookup() vs rmmod races
Alexey Dobriyan wrote: [cc'ing folks whose proc files are affected] kallsyms_lookup() can call module_address_lookup() which iterates over modules list without module_mutex taken. Comment at the top of module_address_lookup() says it's for oops resolution so races are irrelevant, but in some cases it's reachable from regular code: So maybe we should just add a new parameter to kallsyms_lookup to inform it if it is safe to take a mutex or not. Spreading module_mutex everywhere doesn't seem like the right interface for several reasons: - new users of kallsyms_lookup might not be aware that they should take module_mutex if it is safe - many times we will be taking module_mutex even when we are fetching a kernel symbol that shouldn't require the mutex at all - it just creates new dependencies (hint: this patch shouldn't even compile with current git since module_mutex is not declared in module.h, not to mention compile when CONFIG_MODULES not set) IMHO we should not expose module_mutex outside of module.c. That is just wrong from an encapsulation point of view. -- Paulo Marques - www.grupopie.com 667: The neighbor of the beast. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Race between cat /proc/kallsyms and rmmod
Alexey Dobriyan wrote: Iterating code of /proc/kallsyms calls module_get_kallsym() which grabs and drops module_mutex internally and returns "struct module *", module is removed, aforementioned "struct module *" is used in non-trivial way. So, grab module_mutex for entire operation like /proc/modules does. I would still prefer the other solution to avoid exposing "module_mutex" outside of module.c like this :( I'll try to send in a patch today for review. -- Paulo Marques - www.grupopie.com "As far as we know, our computer has never had an undetected error." Weisert - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: /proc/kallsyms race vs module unload
Alexey Dobriyan wrote: On Tue, Mar 13, 2007 at 06:49:50PM +, Paulo Marques wrote: Alexey Dobriyan wrote: [...] What happens is that module_get_kallsym() drops module_mutex, returns "struct module *", module unloaded, "struct module *" used. The only use for the "struct module *" is to display the name of the module. Ehh? This can be solved by adding a "char mod_name[MODULE_NAME_LEN];" field to "kallsym_iter" and copy the name of the module over, while still holding module_mutex. It would be slightly slower, but safer. iter->owner = module_get_kallsym(iter->pos - kallsyms_num_syms, >value, >type, iter->name, sizeof(iter->name)); if (iter->owner == NULL) return 0; /* Label it "global" if it is exported, "local" if not exported. */ iter->type = is_exported(iter->name, iter->owner) ^^^ Yes, there is this "is_exported" call, but his can be moved completely into "module_get_kallsym" and have the "type" returned be already upper / lower case. That, together with filling the module name "module_get_kallsym()" would make the returned "struct module *" unneeded. Since kallsyms is the only caller of that function, we can change its interface to not return a "struct module *" at all, and return just an integer that means "symbol found" or "no more symbols". I'm still volunteering to do that patch, but you seem more active than me at the moment... -- Paulo Marques Software Development Department - Grupo PIE, S.A. Phone: +351 252 290600, Fax: +351 252 290601 Web: www.grupopie.com - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: /proc/kallsyms race vs module unload
Alexey Dobriyan wrote: On Tue, Mar 13, 2007 at 06:49:50PM +, Paulo Marques wrote: Alexey Dobriyan wrote: [...] What happens is that module_get_kallsym() drops module_mutex, returns struct module *, module unloaded, struct module * used. The only use for the struct module * is to display the name of the module. Ehh? This can be solved by adding a char mod_name[MODULE_NAME_LEN]; field to kallsym_iter and copy the name of the module over, while still holding module_mutex. It would be slightly slower, but safer. iter-owner = module_get_kallsym(iter-pos - kallsyms_num_syms, iter-value, iter-type, iter-name, sizeof(iter-name)); if (iter-owner == NULL) return 0; /* Label it global if it is exported, local if not exported. */ iter-type = is_exported(iter-name, iter-owner) ^^^ Yes, there is this is_exported call, but his can be moved completely into module_get_kallsym and have the type returned be already upper / lower case. That, together with filling the module name module_get_kallsym() would make the returned struct module * unneeded. Since kallsyms is the only caller of that function, we can change its interface to not return a struct module * at all, and return just an integer that means symbol found or no more symbols. I'm still volunteering to do that patch, but you seem more active than me at the moment... -- Paulo Marques Software Development Department - Grupo PIE, S.A. Phone: +351 252 290600, Fax: +351 252 290601 Web: www.grupopie.com - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Race between cat /proc/kallsyms and rmmod
Alexey Dobriyan wrote: Iterating code of /proc/kallsyms calls module_get_kallsym() which grabs and drops module_mutex internally and returns struct module *, module is removed, aforementioned struct module * is used in non-trivial way. So, grab module_mutex for entire operation like /proc/modules does. I would still prefer the other solution to avoid exposing module_mutex outside of module.c like this :( I'll try to send in a patch today for review. -- Paulo Marques - www.grupopie.com As far as we know, our computer has never had an undetected error. Weisert - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: /proc/kallsyms race vs module unload
Alexey Dobriyan wrote: [...] What happens is that module_get_kallsym() drops module_mutex, returns "struct module *", module unloaded, "struct module *" used. The only use for the "struct module *" is to display the name of the module. This can be solved by adding a "char mod_name[MODULE_NAME_LEN];" field to "kallsym_iter" and copy the name of the module over, while still holding module_mutex. It would be slightly slower, but safer. We can even change the function's interface, so that it doesn't return a "struct module *" at all, since AFAICS kallsyms is the only user of that function. It will still produce strange artifacts, though. If the iterator is already past the removed module symbols, it will skip as many symbols as the module symbol count, failing to show some symbols from unrelated modules. It won't oops, though. I'll try to cook up a patch, if no one objects to this approach, -- Paulo Marques - www.grupopie.com "There cannot be a crisis today; my schedule is already full." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: /proc/kallsyms race vs module unload
Alexey Dobriyan wrote: [...] What happens is that module_get_kallsym() drops module_mutex, returns struct module *, module unloaded, struct module * used. The only use for the struct module * is to display the name of the module. This can be solved by adding a char mod_name[MODULE_NAME_LEN]; field to kallsym_iter and copy the name of the module over, while still holding module_mutex. It would be slightly slower, but safer. We can even change the function's interface, so that it doesn't return a struct module * at all, since AFAICS kallsyms is the only user of that function. It will still produce strange artifacts, though. If the iterator is already past the removed module symbols, it will skip as many symbols as the module symbol count, failing to show some symbols from unrelated modules. It won't oops, though. I'll try to cook up a patch, if no one objects to this approach, -- Paulo Marques - www.grupopie.com There cannot be a crisis today; my schedule is already full. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] Extend print_symbol capability TRY #3
Robert Peterson wrote: [...] It probably would be easy, but it's beyond the scope of this fix, and therefore probably best left to a separate patch. Feel free to send that out in another patch if you want. If you do, I'd be happy to test it, review it, and ACK it. Fair enough :) For what is worth, Acked-by: Paulo Marques <[EMAIL PROTECTED]> -- Paulo Marques - www.grupopie.com "Very funny Scotty. Now beam up my clothes." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] Extend print_symbol capability TRY #3
Robert Peterson wrote:
[...]
@@ -47,6 +52,11 @@ static inline const char *kallsyms_lookup(unsigned
long addr,
return NULL;
}
+static inline void sprint_symbol(char *buffer, unsigned long addr)
+{
+return;
+}
I'm really sorry for not replying sooner (I've been really busy), but
this function still doesn't seem right.
If someone does something like:
void my_function(unsigned long addr)
{
char buffer[KSYM_SYMBOL_LEN];
sprint_symbol(buffer, addr);
...
// use buffer to print somewhere
...
}
which seems like a perfectly natural thing to do, it might just oops the
kernel if CONFIG_KALLSYMS is not set, because the buffer will be left
uninitialized.
That is why I suggested to change it to something like "*buffer = '\0'"
instead.
The really nice solution IMHO, would be to remove the print_symbol and
sprint_symbol functions from the the "#ifdef CONFIG_KALLSYMS" and just
let them be available even in a not CONFIG_KALLSYMS kernel.
Since kallsyms_lookup is already #ifdef'ed to something sane,
sprint_symbol will just print out the symbol address in that case, but
it is better than not printing anything at all.
--
Paulo Marques - www.grupopie.com
"Very funny Scotty. Now beam up my clothes."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] Extend print_symbol capability TRY #3
Robert Peterson wrote:
[...]
@@ -47,6 +52,11 @@ static inline const char *kallsyms_lookup(unsigned
long addr,
return NULL;
}
+static inline void sprint_symbol(char *buffer, unsigned long addr)
+{
+return;
+}
I'm really sorry for not replying sooner (I've been really busy), but
this function still doesn't seem right.
If someone does something like:
void my_function(unsigned long addr)
{
char buffer[KSYM_SYMBOL_LEN];
sprint_symbol(buffer, addr);
...
// use buffer to print somewhere
...
}
which seems like a perfectly natural thing to do, it might just oops the
kernel if CONFIG_KALLSYMS is not set, because the buffer will be left
uninitialized.
That is why I suggested to change it to something like *buffer = '\0'
instead.
The really nice solution IMHO, would be to remove the print_symbol and
sprint_symbol functions from the the #ifdef CONFIG_KALLSYMS and just
let them be available even in a not CONFIG_KALLSYMS kernel.
Since kallsyms_lookup is already #ifdef'ed to something sane,
sprint_symbol will just print out the symbol address in that case, but
it is better than not printing anything at all.
--
Paulo Marques - www.grupopie.com
Very funny Scotty. Now beam up my clothes.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] Extend print_symbol capability TRY #3
Robert Peterson wrote: [...] It probably would be easy, but it's beyond the scope of this fix, and therefore probably best left to a separate patch. Feel free to send that out in another patch if you want. If you do, I'd be happy to test it, review it, and ACK it. Fair enough :) For what is worth, Acked-by: Paulo Marques [EMAIL PROTECTED] -- Paulo Marques - www.grupopie.com Very funny Scotty. Now beam up my clothes. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] Extend print_symbol capability
Robert Peterson wrote:
[...]
#define KSYM_NAME_LEN 127
+#define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s]") + KSYM_NAME_LEN + \
+2*(BITS_PER_LONG*3/10) + MODULE_NAME_LEN + 1)
#ifdef CONFIG_KALLSYMS
/* Lookup the address for a symbol. Returns 0 if not found. */
@@ -22,6 +24,9 @@ const char *kallsyms_lookup(unsigned long addr,
unsigned long *offset,
char **modname, char *namebuf);
+/* Look up a kernel symbol and return it in a text buffer. */
+extern void lookup_symbol(unsigned long addr, char *buffer);
I don't like this name much :(
We already have kallsyms_lookup and kallsyms_lookup_name. The name of
this function should imply that it will print the formatted result into
the buffer, not just lookup a symbol.
Maybe "__sprint_symbol", and change the interface to
"__sprint_symbol(char *buffer, unsigned long addr)"?
+
/* Replace "%s" in format with address, if found */
extern void __print_symbol(const char *fmt, unsigned long address);
@@ -47,6 +52,11 @@ static inline const char *kallsyms_lookup(unsigned
long addr,
return NULL;
}
+static inline void lookup_symbol(unsigned long addr, char *buffer)
+{
+ return NULL;
+}
Returning NULL in a function returning "void" doesn't seem right :P
Maybe it should be something like this instead:
{
*buffer = '\0';
}
[...]
Anyway, the change looks useful, so thanks for the patch :)
--
Paulo Marques - www.grupopie.com
"Very funny Scotty. Now beam up my clothes."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] Extend print_symbol capability
Robert Peterson wrote:
[...]
#define KSYM_NAME_LEN 127
+#define KSYM_SYMBOL_LEN (sizeof(%s+%#lx/%#lx [%s]) + KSYM_NAME_LEN + \
+2*(BITS_PER_LONG*3/10) + MODULE_NAME_LEN + 1)
#ifdef CONFIG_KALLSYMS
/* Lookup the address for a symbol. Returns 0 if not found. */
@@ -22,6 +24,9 @@ const char *kallsyms_lookup(unsigned long addr,
unsigned long *offset,
char **modname, char *namebuf);
+/* Look up a kernel symbol and return it in a text buffer. */
+extern void lookup_symbol(unsigned long addr, char *buffer);
I don't like this name much :(
We already have kallsyms_lookup and kallsyms_lookup_name. The name of
this function should imply that it will print the formatted result into
the buffer, not just lookup a symbol.
Maybe __sprint_symbol, and change the interface to
__sprint_symbol(char *buffer, unsigned long addr)?
+
/* Replace %s in format with address, if found */
extern void __print_symbol(const char *fmt, unsigned long address);
@@ -47,6 +52,11 @@ static inline const char *kallsyms_lookup(unsigned
long addr,
return NULL;
}
+static inline void lookup_symbol(unsigned long addr, char *buffer)
+{
+ return NULL;
+}
Returning NULL in a function returning void doesn't seem right :P
Maybe it should be something like this instead:
{
*buffer = '\0';
}
[...]
Anyway, the change looks useful, so thanks for the patch :)
--
Paulo Marques - www.grupopie.com
Very funny Scotty. Now beam up my clothes.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: SMP performance degradation with sysbench
Rik van Riel wrote:
J.A. Magallón wrote:
[...]
Its the same to answer 4+4 queries than 8 at half the speed, isn't it ?
That still doesn't fix the potential Linux problem that this
benchmark identified.
To clarify: I don't care as much about MySQL performance as
I care about identifying and fixing this potential bug in
Linux.
IIRC a long time ago there was a change in the scheduler to prevent a
low prio task running on a sibling of a hyperthreaded processor to slow
down a higher prio task on another sibling of the same processor.
Basically the scheduler would put the low prio task to sleep during an
adequate task slice to allow the other sibling to run at full speed for
a while.
I don't know the scheduler code well enough, but comments like this one
make me think that the change is still in place:
/*
* If an SMT sibling task has been put to sleep for priority
* reasons reschedule the idle task to see if it can now run.
*/
if (rq->nr_running) {
resched_task(rq->idle);
ret = 1;
}
If that is the case, turning off CONFIG_SCHED_SMT would solve the problem.
--
Paulo Marques - www.grupopie.com
"The face of a child can say it all, especially the
mouth part of the face."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: SMP performance degradation with sysbench
Rik van Riel wrote:
J.A. Magallón wrote:
[...]
Its the same to answer 4+4 queries than 8 at half the speed, isn't it ?
That still doesn't fix the potential Linux problem that this
benchmark identified.
To clarify: I don't care as much about MySQL performance as
I care about identifying and fixing this potential bug in
Linux.
IIRC a long time ago there was a change in the scheduler to prevent a
low prio task running on a sibling of a hyperthreaded processor to slow
down a higher prio task on another sibling of the same processor.
Basically the scheduler would put the low prio task to sleep during an
adequate task slice to allow the other sibling to run at full speed for
a while.
I don't know the scheduler code well enough, but comments like this one
make me think that the change is still in place:
/*
* If an SMT sibling task has been put to sleep for priority
* reasons reschedule the idle task to see if it can now run.
*/
if (rq-nr_running) {
resched_task(rq-idle);
ret = 1;
}
If that is the case, turning off CONFIG_SCHED_SMT would solve the problem.
--
Paulo Marques - www.grupopie.com
The face of a child can say it all, especially the
mouth part of the face.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: linux-2.6.19.3 build faild with "Inconsistent kallsyms data"
Toralf Förster wrote: Hello, the build with the attached .config failed, make ends with: ... scripts/kconfig/conf -s arch/i386/Kconfig CHK include/linux/version.h CHK include/linux/utsrelease.h CHK include/linux/compile.h AS .tmp_kallsyms2.o LD vmlinux SYSMAP System.map SYSMAP .tmp_System.map Inconsistent kallsyms data Try setting CONFIG_KALLSYMS_EXTRA_PASS make: *** [vmlinux] Error 1 Here's the config: Hummm, I just tried that .config with a vanilla 2.6.19.3 and it built just fine. So either you have some patches that make a difference, or it's a binutils version problem. Can you send the output of scripts/ver_linux to see what binutils version you are using? Also you can try a "make debug_kallsyms" build that creates a .tmp_map1 and a .tmp_map2 files that might help diagnose the problem. -- Paulo Marques - www.grupopie.com "The face of a child can say it all, especially the mouth part of the face." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: linux-2.6.19.3 build faild with Inconsistent kallsyms data
Toralf Förster wrote: Hello, the build with the attached .config failed, make ends with: ... scripts/kconfig/conf -s arch/i386/Kconfig CHK include/linux/version.h CHK include/linux/utsrelease.h CHK include/linux/compile.h AS .tmp_kallsyms2.o LD vmlinux SYSMAP System.map SYSMAP .tmp_System.map Inconsistent kallsyms data Try setting CONFIG_KALLSYMS_EXTRA_PASS make: *** [vmlinux] Error 1 Here's the config: Hummm, I just tried that .config with a vanilla 2.6.19.3 and it built just fine. So either you have some patches that make a difference, or it's a binutils version problem. Can you send the output of scripts/ver_linux to see what binutils version you are using? Also you can try a make debug_kallsyms build that creates a .tmp_map1 and a .tmp_map2 files that might help diagnose the problem. -- Paulo Marques - www.grupopie.com The face of a child can say it all, especially the mouth part of the face. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: PROBLEM: 2.6.13, Inconsistent kallsyms data
Jim McCloskey wrote:
When I try to compile 2.6.13, using a complete tarball from
kernel.org, the compilation fails with:
---
SYSMAP System.map
SYSMAP .tmp_System.map
Inconsistent kallsyms data
Try setting CONFIG_KALLSYMS_EXTRA_PASS
make: *** [vmlinux] Error 1
---
When CONFIG_KALLSYMS_EXTRA_PASS is set, the compilation completes
successfully.
Please try the attached patch too see if it fixes the problem for you.
This patch is already in -mm and scheduled to go in 2.6.14 (or at least
I hope it is ;)
--
Paulo Marques - www.grupopie.com
It is a mistake to think you can solve any major problems
just with potatoes.
Douglas Adams
--- ./scripts/kallsyms.c.orig 2005-06-23 19:20:20.0 +0100
+++ ./scripts/kallsyms.c2005-06-23 19:20:34.0 +0100
@@ -24,75 +24,37 @@
*
*/
+#define _GNU_SOURCE
+
#include
#include
#include
#include
-/* maximum token length used. It doesn't pay to increase it a lot, because
- * very long substrings probably don't repeat themselves too often. */
-#define MAX_TOK_SIZE 11
#define KSYM_NAME_LEN 127
-/* we use only a subset of the complete symbol table to gather the token count,
- * to speed up compression, at the expense of a little compression ratio */
-#define WORKING_SET1024
-
-/* first find the best token only on the list of tokens that would profit more
- * than GOOD_BAD_THRESHOLD. Only if this list is empty go to the "bad" list.
- * Increasing this value will put less tokens on the "good" list, so the search
- * is faster. However, if the good list runs out of tokens, we must painfully
- * search the bad list. */
-#define GOOD_BAD_THRESHOLD 10
-
-/* token hash parameters */
-#define HASH_BITS 18
-#define HASH_TABLE_SIZE(1 << HASH_BITS)
-#define HASH_MASK (HASH_TABLE_SIZE - 1)
-#define HASH_BASE_OFFSET 2166136261U
-#define HASH_FOLD(a) ((a)&(HASH_MASK))
-
-/* flags to mark symbols */
-#define SYM_FLAG_VALID 1
-#define SYM_FLAG_SAMPLED 2
struct sym_entry {
unsigned long long addr;
- char type;
- unsigned char flags;
- unsigned char len;
+ unsigned int len;
unsigned char *sym;
};
static struct sym_entry *table;
-static int size, cnt;
+static unsigned int table_size, table_cnt;
static unsigned long long _stext, _etext, _sinittext, _einittext, _sextratext,
_eextratext;
static int all_symbols = 0;
static char symbol_prefix_char = '\0';
-struct token {
- unsigned char data[MAX_TOK_SIZE];
- unsigned char len;
- /* profit: the number of bytes that could be saved by inserting this
-* token into the table */
- int profit;
- struct token *next; /* next token on the hash list */
- struct token *right;/* next token on the good/bad list */
- struct token *left;/* previous token on the good/bad list */
- struct token *smaller; /* token that is less one letter than this one */
- };
-
-struct token bad_head, good_head;
-struct token *hash_table[HASH_TABLE_SIZE];
+int token_profit[0x1];
/* the table that holds the result of the compression */
-unsigned char best_table[256][MAX_TOK_SIZE+1];
+unsigned char best_table[256][2];
unsigned char best_table_len[256];
-static void
-usage(void)
+static void usage(void)
{
fprintf(stderr, "Usage: kallsyms [--all-symbols]
[--symbol-prefix=] < in.map > out.S\n");
exit(1);
@@ -102,21 +64,19 @@ usage(void)
* This ignores the intensely annoying "mapping symbols" found
* in ARM ELF files: $a, $t and $d.
*/
-static inline int
-is_arm_mapping_symbol(const char *str)
+static inline int is_arm_mapping_symbol(const char *str)
{
return str[0] == '$' && strchr("atd", str[1])
&& (str[2] == '\0' || str[2] == '.');
}
-static int
-read_symbol(FILE *in, struct sym_entry *s)
+static int read_symbol(FILE *in, struct sym_entry *s)
{
char str[500];
- char *sym;
+ char *sym, stype;
int rc;
- rc = fscanf(in, "%llx %c %499s\n", >addr, >type, str);
+ rc = fscanf(in, "%llx %c %499s\n", >addr, , str);
if (rc != 3) {
if (rc != EOF) {
/* skip line */
@@ -143,7 +103,7 @@ read_symbol(FILE *in, struct sym_entry *
_sextratext = s->addr;
else if (strcmp(sym, "_eextratext") == 0)
_eextratext = s->addr;
- else if (toupper(s->type) == 'A')
+ else if (toupper(stype) == 'A')
{
/* Keep these useful absolute symbols */
if (strcmp(sym, "__kernel_syscall_via_break") &&
@@ -153,22 +113,21 @@ read_symbol(FILE *in,
Re: PROBLEM: 2.6.13, Inconsistent kallsyms data
Jim McCloskey wrote:
When I try to compile 2.6.13, using a complete tarball from
kernel.org, the compilation fails with:
---
SYSMAP System.map
SYSMAP .tmp_System.map
Inconsistent kallsyms data
Try setting CONFIG_KALLSYMS_EXTRA_PASS
make: *** [vmlinux] Error 1
---
When CONFIG_KALLSYMS_EXTRA_PASS is set, the compilation completes
successfully.
Please try the attached patch too see if it fixes the problem for you.
This patch is already in -mm and scheduled to go in 2.6.14 (or at least
I hope it is ;)
--
Paulo Marques - www.grupopie.com
It is a mistake to think you can solve any major problems
just with potatoes.
Douglas Adams
--- ./scripts/kallsyms.c.orig 2005-06-23 19:20:20.0 +0100
+++ ./scripts/kallsyms.c2005-06-23 19:20:34.0 +0100
@@ -24,75 +24,37 @@
*
*/
+#define _GNU_SOURCE
+
#include stdio.h
#include stdlib.h
#include string.h
#include ctype.h
-/* maximum token length used. It doesn't pay to increase it a lot, because
- * very long substrings probably don't repeat themselves too often. */
-#define MAX_TOK_SIZE 11
#define KSYM_NAME_LEN 127
-/* we use only a subset of the complete symbol table to gather the token count,
- * to speed up compression, at the expense of a little compression ratio */
-#define WORKING_SET1024
-
-/* first find the best token only on the list of tokens that would profit more
- * than GOOD_BAD_THRESHOLD. Only if this list is empty go to the bad list.
- * Increasing this value will put less tokens on the good list, so the search
- * is faster. However, if the good list runs out of tokens, we must painfully
- * search the bad list. */
-#define GOOD_BAD_THRESHOLD 10
-
-/* token hash parameters */
-#define HASH_BITS 18
-#define HASH_TABLE_SIZE(1 HASH_BITS)
-#define HASH_MASK (HASH_TABLE_SIZE - 1)
-#define HASH_BASE_OFFSET 2166136261U
-#define HASH_FOLD(a) ((a)(HASH_MASK))
-
-/* flags to mark symbols */
-#define SYM_FLAG_VALID 1
-#define SYM_FLAG_SAMPLED 2
struct sym_entry {
unsigned long long addr;
- char type;
- unsigned char flags;
- unsigned char len;
+ unsigned int len;
unsigned char *sym;
};
static struct sym_entry *table;
-static int size, cnt;
+static unsigned int table_size, table_cnt;
static unsigned long long _stext, _etext, _sinittext, _einittext, _sextratext,
_eextratext;
static int all_symbols = 0;
static char symbol_prefix_char = '\0';
-struct token {
- unsigned char data[MAX_TOK_SIZE];
- unsigned char len;
- /* profit: the number of bytes that could be saved by inserting this
-* token into the table */
- int profit;
- struct token *next; /* next token on the hash list */
- struct token *right;/* next token on the good/bad list */
- struct token *left;/* previous token on the good/bad list */
- struct token *smaller; /* token that is less one letter than this one */
- };
-
-struct token bad_head, good_head;
-struct token *hash_table[HASH_TABLE_SIZE];
+int token_profit[0x1];
/* the table that holds the result of the compression */
-unsigned char best_table[256][MAX_TOK_SIZE+1];
+unsigned char best_table[256][2];
unsigned char best_table_len[256];
-static void
-usage(void)
+static void usage(void)
{
fprintf(stderr, Usage: kallsyms [--all-symbols]
[--symbol-prefix=prefix char] in.map out.S\n);
exit(1);
@@ -102,21 +64,19 @@ usage(void)
* This ignores the intensely annoying mapping symbols found
* in ARM ELF files: $a, $t and $d.
*/
-static inline int
-is_arm_mapping_symbol(const char *str)
+static inline int is_arm_mapping_symbol(const char *str)
{
return str[0] == '$' strchr(atd, str[1])
(str[2] == '\0' || str[2] == '.');
}
-static int
-read_symbol(FILE *in, struct sym_entry *s)
+static int read_symbol(FILE *in, struct sym_entry *s)
{
char str[500];
- char *sym;
+ char *sym, stype;
int rc;
- rc = fscanf(in, %llx %c %499s\n, s-addr, s-type, str);
+ rc = fscanf(in, %llx %c %499s\n, s-addr, stype, str);
if (rc != 3) {
if (rc != EOF) {
/* skip line */
@@ -143,7 +103,7 @@ read_symbol(FILE *in, struct sym_entry *
_sextratext = s-addr;
else if (strcmp(sym, _eextratext) == 0)
_eextratext = s-addr;
- else if (toupper(s-type) == 'A')
+ else if (toupper(stype) == 'A')
{
/* Keep these useful absolute symbols */
if (strcmp(sym, __kernel_syscall_via_break)
@@ -153,22 +113,21 @@ read_symbol(FILE *in, struct sym_entry *
return -1;
}
- else if (toupper(s-type) == 'U' ||
+ else if (toupper(stype
