| Message ID | 20171211125025.2270-1-sergey.senozhatsky@gmail.com |
|---|---|
| Headers | show |
| Series | replace print_symbol() with printk()-s | expand |
On Mon, 2017-12-11 at 21:50 +0900, Sergey Senozhatsky wrote:
> print_symbol
Yay.
Just about exactly 5 years earlier...
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137121.html
On (12/11/17 08:26), Joe Perches wrote: > On Mon, 2017-12-11 at 21:50 +0900, Sergey Senozhatsky wrote: > > print_symbol > > Yay. > > Just about exactly 5 years earlier... > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137121.html indeed :) hopefully it won't take us another 5 years to finally --- include/linux/kallsyms.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h index 7288f9c395b6..794fd35cad4b 100644 --- a/include/linux/kallsyms.h +++ b/include/linux/kallsyms.h @@ -176,7 +176,8 @@ void __check_printsym_format(const char *fmt, ...) { } -static inline void print_symbol(const char *fmt, unsigned long addr) +static inline void __deprecated print_symbol(const char *fmt, + unsigned long addr) { __check_printsym_format(fmt, ""); __print_symbol(fmt, (unsigned long) --- -ss
On Tue, 2017-12-12 at 11:47 +0900, Sergey Senozhatsky wrote: > On (12/11/17 08:26), Joe Perches wrote: > > On Mon, 2017-12-11 at 21:50 +0900, Sergey Senozhatsky wrote: > > > print_symbol > > Yay. > > Just about exactly 5 years earlier... > > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137121.html > > indeed :) > > hopefully it won't take us another 5 years to finally [] > diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h [] > -static inline void print_symbol(const char *fmt, unsigned long addr) > +static inline void __deprecated print_symbol(const char *fmt, > + unsigned long addr) > { > __check_printsym_format(fmt, ""); > __print_symbol(fmt, (unsigned long) As far as I'm concerned, as soon as there is no longer a single user in the kernel tree, better to delete it instead.
On (12/11/17 21:50), Sergey Senozhatsky wrote: > > A rather automatic replacement of print_symbol() > with direct printk() calls. print_symbol() uses extra stack > buffer (KSYM_SYMBOL_LEN 128 bytes) and, basically, should > be identical to printk(%pS). > > I can't test all of the patches, because I don't > own any of those exotic arch-s. Sorry for the inconvenience. > > Sergey Senozhatsky (13): > arm: do not use print_symbol() > arm64: do not use print_symbol() > c6x: do not use print_symbol() > ia64: do not use print_symbol() > mn10300: do not use print_symbol() > sh: do not use print_symbol() > unicore32: do not use print_symbol() > x86: do not use print_symbol() > drivers: do not use print_symbol() > sysfs: do not use print_symbol() > irq debug: do not use print_symbol() > lib: do not use print_symbol() > arc: do not use __print_symbol() Hello, can we please have more reviews/acks/etc? -ss
On (12/11/17 19:10), Joe Perches wrote: [..] > As far as I'm concerned, as soon as there is > no longer a single user in the kernel tree, > better to delete it instead. sounds good to me. can drop it, once the series upstreamed. 8< --- From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Subject: [PATCH] kallsyms: remove print_symbol() function No more print_symbol()/__print_symbol() users left, remove these symbols. Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Suggested-by: Joe Perches <joe@perches.com> --- Documentation/filesystems/sysfs.txt | 4 ++-- Documentation/translations/zh_CN/filesystems/sysfs.txt | 4 ++-- include/linux/kallsyms.h | 18 ------------------ kernel/kallsyms.c | 11 ----------- 4 files changed, 4 insertions(+), 33 deletions(-) diff --git a/Documentation/filesystems/sysfs.txt b/Documentation/filesystems/sysfs.txt index 9a3658cc399e..a1426cabcef1 100644 --- a/Documentation/filesystems/sysfs.txt +++ b/Documentation/filesystems/sysfs.txt @@ -154,8 +154,8 @@ static ssize_t dev_attr_show(struct kobject *kobj, struct attribute *attr, if (dev_attr->show) ret = dev_attr->show(dev, dev_attr, buf); if (ret >= (ssize_t)PAGE_SIZE) { - print_symbol("dev_attr_show: %s returned bad count\n", - (unsigned long)dev_attr->show); + printk("dev_attr_show: %pS returned bad count\n", + dev_attr->show); } return ret; } diff --git a/Documentation/translations/zh_CN/filesystems/sysfs.txt b/Documentation/translations/zh_CN/filesystems/sysfs.txt index 7d3b05edb8ce..452271dda141 100644 --- a/Documentation/translations/zh_CN/filesystems/sysfs.txt +++ b/Documentation/translations/zh_CN/filesystems/sysfs.txt @@ -167,8 +167,8 @@ static ssize_t dev_attr_show(struct kobject *kobj, struct attribute *attr, if (dev_attr->show) ret = dev_attr->show(dev, dev_attr, buf); if (ret >= (ssize_t)PAGE_SIZE) { - print_symbol("dev_attr_show: %s returned bad count\n", - (unsigned long)dev_attr->show); + printk("dev_attr_show: %pS returned bad count\n", + dev_attr->show); } return ret; } diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h index 7288f9c395b6..d79d1e7486bd 100644 --- a/include/linux/kallsyms.h +++ b/include/linux/kallsyms.h @@ -94,9 +94,6 @@ extern int sprint_symbol(char *buffer, unsigned long address); extern int sprint_symbol_no_offset(char *buffer, unsigned long address); extern int sprint_backtrace(char *buffer, unsigned long address); -/* Look up a kernel symbol and print it to the kernel messages. */ -extern void __print_symbol(const char *fmt, unsigned long address); - int lookup_symbol_name(unsigned long addr, char *symname); int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); @@ -166,23 +163,8 @@ static inline int kallsyms_show_value(void) return false; } -/* Stupid that this does nothing, but I didn't create this mess. */ -#define __print_symbol(fmt, addr) #endif /*CONFIG_KALLSYMS*/ -/* This macro allows us to keep printk typechecking */ -static __printf(1, 2) -void __check_printsym_format(const char *fmt, ...) -{ -} - -static inline void print_symbol(const char *fmt, unsigned long addr) -{ - __check_printsym_format(fmt, ""); - __print_symbol(fmt, (unsigned long) - __builtin_extract_return_addr((void *)addr)); -} - static inline void print_ip_sym(unsigned long ip) { printk("[<%p>] %pS\n", (void *) ip, (void *) ip); diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 24f456689f9c..a23e21ada81b 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -429,17 +429,6 @@ int sprint_backtrace(char *buffer, unsigned long address) return __sprint_symbol(buffer, address, -1, 1); } -/* Look up a kernel symbol and print it to the kernel messages. */ -void __print_symbol(const char *fmt, unsigned long address) -{ - char buffer[KSYM_SYMBOL_LEN]; - - sprint_symbol(buffer, address); - - printk(fmt, buffer); -} -EXPORT_SYMBOL(__print_symbol); - /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */ struct kallsym_iter { loff_t pos;
On Mon 2017-12-11 21:50:12, Sergey Senozhatsky wrote: > Hello, > > A rather automatic replacement of print_symbol() > with direct printk() calls. print_symbol() uses extra stack > buffer (KSYM_SYMBOL_LEN 128 bytes) and, basically, should > be identical to printk(%pS). To make it clear, both print_symbol() and printk(%pS) print the adress using sprint_symbol(). And even printk(%pS) uses the buffer on the stack, see: char *symbol_string(char *buf, char *end, void *ptr, struct printf_spec spec, const char *fmt) { [...] char sym[KSYM_SYMBOL_LEN]; [...] sprint_symbol(sym, value); } Anyway, print_symbol() is an old weird API and it would be nice to eventually get rid of it. I could take this patches into printk.git. Would you mind if I change the commit messages to something like?: print_symbol() is an old weird API. It has been obsoleted by printk() and %pS format specifier. Best Regards, Petr
On (01/05/18 11:03), Petr Mladek wrote: [..] > Anyway, print_symbol() is an old weird API and it would be nice > to eventually get rid of it. I could take this patches into > printk.git. no objections from my side if the patch set will go through the printk tree. shall we wait for ACKs or can we move on? do you plan to land it in 4.16? > Would you mind if I change the commit messages to something like?: > > print_symbol() is an old weird API. It has been > obsoleted by printk() and %pS format specifier. I wouldn't. let's drop the "weird" part. other than that looks good to me. -ss
On (01/05/18 19:21), Sergey Senozhatsky wrote: > On (01/05/18 11:03), Petr Mladek wrote: > [..] > > Anyway, print_symbol() is an old weird API and it would be nice > > to eventually get rid of it. I could take this patches into > > printk.git. > > no objections from my side if the patch set will go through the printk tree. > shall we wait for ACKs or can we move on? do you plan to land it in 4.16? > > > Would you mind if I change the commit messages to something like?: > > > > print_symbol() is an old weird API. It has been > > obsoleted by printk() and %pS format specifier. > > I wouldn't. let's drop the "weird" part. other than that looks > good to me. oh, one more thing. one extra patch, which gets rid of print_symbol()/__print_symbol(). 8< === From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Subject: [PATCH] kallsyms: remove print_symbol() function No more print_symbol()/__print_symbol() users left, remove these symbols. Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Suggested-by: Joe Perches <joe@perches.com> --- Documentation/filesystems/sysfs.txt | 4 ++-- Documentation/translations/zh_CN/filesystems/sysfs.txt | 4 ++-- include/linux/kallsyms.h | 18 ------------------ kernel/kallsyms.c | 11 ----------- 4 files changed, 4 insertions(+), 33 deletions(-) diff --git a/Documentation/filesystems/sysfs.txt b/Documentation/filesystems/sysfs.txt index 9a3658cc399e..a1426cabcef1 100644 --- a/Documentation/filesystems/sysfs.txt +++ b/Documentation/filesystems/sysfs.txt @@ -154,8 +154,8 @@ static ssize_t dev_attr_show(struct kobject *kobj, struct attribute *attr, if (dev_attr->show) ret = dev_attr->show(dev, dev_attr, buf); if (ret >= (ssize_t)PAGE_SIZE) { - print_symbol("dev_attr_show: %s returned bad count\n", - (unsigned long)dev_attr->show); + printk("dev_attr_show: %pS returned bad count\n", + dev_attr->show); } return ret; } diff --git a/Documentation/translations/zh_CN/filesystems/sysfs.txt b/Documentation/translations/zh_CN/filesystems/sysfs.txt index 7d3b05edb8ce..452271dda141 100644 --- a/Documentation/translations/zh_CN/filesystems/sysfs.txt +++ b/Documentation/translations/zh_CN/filesystems/sysfs.txt @@ -167,8 +167,8 @@ static ssize_t dev_attr_show(struct kobject *kobj, struct attribute *attr, if (dev_attr->show) ret = dev_attr->show(dev, dev_attr, buf); if (ret >= (ssize_t)PAGE_SIZE) { - print_symbol("dev_attr_show: %s returned bad count\n", - (unsigned long)dev_attr->show); + printk("dev_attr_show: %pS returned bad count\n", + dev_attr->show); } return ret; } diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h index 23190e5c940b..657a83b943f0 100644 --- a/include/linux/kallsyms.h +++ b/include/linux/kallsyms.h @@ -94,9 +94,6 @@ extern int sprint_symbol(char *buffer, unsigned long address); extern int sprint_symbol_no_offset(char *buffer, unsigned long address); extern int sprint_backtrace(char *buffer, unsigned long address); -/* Look up a kernel symbol and print it to the kernel messages. */ -extern void __print_symbol(const char *fmt, unsigned long address); - int lookup_symbol_name(unsigned long addr, char *symname); int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); @@ -166,23 +163,8 @@ static inline int kallsyms_show_value(void) return false; } -/* Stupid that this does nothing, but I didn't create this mess. */ -#define __print_symbol(fmt, addr) #endif /*CONFIG_KALLSYMS*/ -/* This macro allows us to keep printk typechecking */ -static __printf(1, 2) -void __check_printsym_format(const char *fmt, ...) -{ -} - -static inline void print_symbol(const char *fmt, unsigned long addr) -{ - __check_printsym_format(fmt, ""); - __print_symbol(fmt, (unsigned long) - __builtin_extract_return_addr((void *)addr)); -} - static inline void print_ip_sym(unsigned long ip) { printk("[<%px>] %pS\n", (void *) ip, (void *) ip); diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 24f456689f9c..a23e21ada81b 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -429,17 +429,6 @@ int sprint_backtrace(char *buffer, unsigned long address) return __sprint_symbol(buffer, address, -1, 1); } -/* Look up a kernel symbol and print it to the kernel messages. */ -void __print_symbol(const char *fmt, unsigned long address) -{ - char buffer[KSYM_SYMBOL_LEN]; - - sprint_symbol(buffer, address); - - printk(fmt, buffer); -} -EXPORT_SYMBOL(__print_symbol); - /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */ struct kallsym_iter { loff_t pos;
On Fri 2018-01-05 19:21:05, Sergey Senozhatsky wrote: > On (01/05/18 11:03), Petr Mladek wrote: > [..] > > Anyway, print_symbol() is an old weird API and it would be nice > > to eventually get rid of it. I could take this patches into > > printk.git. > > no objections from my side if the patch set will go through the printk tree. > shall we wait for ACKs or can we move on? do you plan to land it in 4.16? I am going to add this into for-4.16 branch. It is a rather cosmetic and trivial change. Therefore I do not think that we would need to wait for all the ACKs. Anyway, I am going to proactively check for potential conflicts with linux-next. > > Would you mind if I change the commit messages to something like?: > > > > print_symbol() is an old weird API. It has been > > obsoleted by printk() and %pS format specifier. > > I wouldn't. let's drop the "weird" part. other than that looks > good to me. OK. Best Regards, Petr
On (01/05/18 19:21), Sergey Senozhatsky wrote: > > print_symbol() is an old weird API. It has been > > obsoleted by printk() and %pS format specifier. > > I wouldn't. let's drop the "weird" part. hm... you are right, it is weird. and the weird part here is that print_symbol() is used for things like __show_regs() print_symbol("PC is at %s\n", instruction_pointer(regs)); print_symbol("LR is at %s\n", regs->ARM_lr); printk("pc : [<%08lx>] lr : [<%08lx>] psr: %08lx\n", regs->ARM_pc, regs->ARM_lr, regs->ARM_cpsr); or for EMERG error reporting pr_emerg("unexpected fault for address: 0x%08lx, last fault for address: 0x%08lx\n", addr, my_reason->addr); print_pte(addr); print_symbol(KERN_EMERG "faulting IP is at %s\n", regs->ip); print_symbol(KERN_EMERG "last faulting IP was at %s\n", my_reason->ip); #ifdef __i386__ pr_emerg("eax: %08lx ebx: %08lx ecx: %08lx edx: %08lx\n", regs->ax, regs->bx, regs->cx, regs->dx); or for error reporting in sysfs print_symbol("fill_read_buffer: %s returned bad count\n", (unsigned long)ops->show); and so on. but, print_symbol() is compiled out on !CONFIG_KALLSYMS systems. so, basically, we compile out some of errors print outs; even more, on ia64 ia64_do_show_stack() does nothing when there is no CONFIG_KALLSYMS [all ia64 defconfigs have KALLSYMS_ALL enabled]. printk(%pS), unlike print_symbol(), is not compiled out and prints the function address when symbolic name is not available. but, at a glance, print_symbol() in most of the cases has printk(registers) next to it or before it, so it doesn't look like we are introducing a regression here by switching to printk(%pS). -ss
On (01/05/18 21:01), Sergey Senozhatsky wrote: [..] > but, print_symbol() is compiled out on !CONFIG_KALLSYMS systems. so, > basically, we compile out some of errors print outs; even more, on ia64 > ia64_do_show_stack() does nothing when there is no CONFIG_KALLSYMS [all > ia64 defconfigs have KALLSYMS_ALL enabled]. printk(%pS), unlike > print_symbol(), is not compiled out and prints the function address > when symbolic name is not available. but, at a glance, print_symbol() > in most of the cases has printk(registers) next to it or before it, so > it doesn't look like we are introducing a regression here by switching > to printk(%pS). well, if this is a problem, then we can have static inline void print_symbol(const char *fmt, unsigned long addr) { printk(fmt, addr); } for CONFIG_KALLSYMS builds, and an empty print_symbol() for !CONFIG_KALLSYMS builds. but we still have tons printk(%pS) in the kernel and even print_ip_sym() (which is not compiled out on !CONFIG_KALLSYMS). so it seems to me that we can drop print_symbol()/__print_symbol() and switch to printk(%pS) after all. -ss
On Fri 2018-01-05 21:23:34, Sergey Senozhatsky wrote: > On (01/05/18 21:01), Sergey Senozhatsky wrote: > [..] > > but, print_symbol() is compiled out on !CONFIG_KALLSYMS systems. so, > > basically, we compile out some of errors print outs; even more, on ia64 > > ia64_do_show_stack() does nothing when there is no CONFIG_KALLSYMS [all > > ia64 defconfigs have KALLSYMS_ALL enabled]. printk(%pS), unlike > > print_symbol(), is not compiled out and prints the function address > > when symbolic name is not available. but, at a glance, print_symbol() > > in most of the cases has printk(registers) next to it or before it, so > > it doesn't look like we are introducing a regression here by switching > > to printk(%pS). > > well, if this is a problem, then we can have I believe that this is not a problem. If it was, we would most likely need to solve it in the existing printk(%pS) callers. > but we still have tons printk(%pS) in the kernel and even print_ip_sym() > (which is not compiled out on !CONFIG_KALLSYMS). so it seems to me that > we can drop print_symbol()/__print_symbol() and switch to printk(%pS) > after all. Exactly. BTW: print_symbol() looks weird to me because: + looks like a normal printk() but + only one format specifier (%s) is replaced + %s is used to print an address/pointer IMHO, this is counter-intuitive and even error prone. Also it makes people using crazy hacks like the one fixed in 4th patch, see https://lkml.kernel.org/r/20171211125025.2270-5-sergey.senozhatsky@gmail.com Best Regards, Petr
On Fri 2018-01-05 19:25:38, Sergey Senozhatsky wrote: > On (01/05/18 19:21), Sergey Senozhatsky wrote: > > On (01/05/18 11:03), Petr Mladek wrote: > > [..] > > > Anyway, print_symbol() is an old weird API and it would be nice > > > to eventually get rid of it. I could take this patches into > > > printk.git. > > > > no objections from my side if the patch set will go through the printk tree. > > shall we wait for ACKs or can we move on? do you plan to land it in 4.16? > > > > > Would you mind if I change the commit messages to something like?: > > > > > > print_symbol() is an old weird API. It has been > > > obsoleted by printk() and %pS format specifier. > > > > I wouldn't. let's drop the "weird" part. other than that looks > > good to me. > > oh, one more thing. one extra patch, which gets rid of > print_symbol()/__print_symbol(). I am all for it. But I would postpone this removal to 4.17. The reason is rather ugly. 13th patch is already in arc tree. We would need to shuffle the patch or coordinate pull requests. I think that it is not worth it. There is no real hurry. I doubt that the would be any new user in the meantime. Best Regards, Petr PS: I have just pushed 12 patches into printk.git for-4.16 branch. I will merge this to linux-next branch on Monday. I will not be around the computer over the weekend...
On (01/05/18 15:42), Petr Mladek wrote: [..] > > oh, one more thing. one extra patch, which gets rid of > > print_symbol()/__print_symbol(). > > I am all for it. But I would postpone this removal to 4.17. > The reason is rather ugly. 13th patch is already in arc tree. > We would need to shuffle the patch or coordinate pull requests. > I think that it is not worth it. There is no real hurry. > I doubt that the would be any new user in the meantime. > > Best Regards, > Petr > > PS: I have just pushed 12 patches into printk.git for-4.16 branch. > I will merge this to linux-next branch on Monday. I will not > be around the computer over the weekend... OK. thanks! -ss
On (01/05/18 15:42), Petr Mladek wrote: > > I am all for it. But I would postpone this removal to 4.17. > The reason is rather ugly. 13th patch is already in arc tree. > We would need to shuffle the patch or coordinate pull requests. JFI, the patch is in Linus's tree as of now (d0729bc6bee797fb). -ss
On Mon 2018-01-08 11:09:42, Sergey Senozhatsky wrote: > On (01/05/18 15:42), Petr Mladek wrote: > > > > I am all for it. But I would postpone this removal to 4.17. > > The reason is rather ugly. 13th patch is already in arc tree. > > We would need to shuffle the patch or coordinate pull requests. > > JFI, the patch is in Linus's tree as of now (d0729bc6bee797fb). Great. I have pushed the patch that removes printk_symbol() into printk.git, branch for-4.16-print-symbol. Note that I have updated the commit message similar way like I did for the other commits. Especially I wanted to mention what it was obsoleted by. The message is: kallsyms: remove print_symbol() function No more print_symbol()/__print_symbol() users left, remove these symbols. It was a very old API that encouraged people use continuous lines. It had been obsoleted by %pS format specifier in a normal printk() call. See also https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/commit/?h=for-4.16-print-symbol&id=d2279c9d7f7db7f97567368bfc4539b3411adf8d Best Regards, Petr
On (01/16/18 17:33), Petr Mladek wrote: [..] > > JFI, the patch is in Linus's tree as of now (d0729bc6bee797fb). > > Great. I have pushed the patch that removes printk_symbol() > into printk.git, branch for-4.16-print-symbol. > > Note that I have updated the commit message similar way > like I did for the other commits. Especially I wanted > to mention what it was obsoleted by. The message is: > > kallsyms: remove print_symbol() function > > No more print_symbol()/__print_symbol() users left, remove these > symbols. > > It was a very old API that encouraged people use continuous lines. > It had been obsoleted by %pS format specifier in a normal printk() > call. > > > See also > https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/commit/?h=for-4.16-print-symbol&id=d2279c9d7f7db7f97567368bfc4539b3411adf8d thanks! -ss