We want to move dump_stack related functions out of printk C
code and consolidate them in lib/dump_stack file. The reason why
dump_stack_print_info()/etc ended up in printk.c was a "generic"
(dummy) lib dump_stack() function, which archs can override.
For example, blackfin and nds32, define their own EXPORT_SYMBOL
dump_stack() functions.
In case of blackfin that arch-specific dump_stack() symbol invokes
a global dump_stack_print_info() function. So we can't easily move
dump_stack_print_info() to lib/dump_stack, because this way we will
link with lib/dump_stack.o file, which will bring in a generic
dump_stack() symbol with it, causing a multiple definitions error:
- one dump_stack() from arch/blackfin/dumpstack
- the other one from lib/dump_stack
Convert generic dump_stack() into a weak symbol. So we will be
able link with lib/dump_stack and at the same time archs will
be able to override dump_stack(). It also opens up a way for us
to move dump_stack_set_arch_desc(), dump_stack_print_info() and
show_regs_print_info() to lib/dump_stack.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
arch/blackfin/kernel/dumpstack.c | 1 -
arch/nds32/kernel/traps.c | 2 --
lib/dump_stack.c | 4 ++--
3 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/arch/blackfin/kernel/dumpstack.c b/arch/blackfin/kernel/dumpstack.c
index 3c992c1f8ef2..61af017130cd 100644
--- a/arch/blackfin/kernel/dumpstack.c
+++ b/arch/blackfin/kernel/dumpstack.c
@@ -174,4 +174,3 @@ void dump_stack(void)
show_stack(current, &stack);
trace_buffer_restore(tflags);
}
-EXPORT_SYMBOL(dump_stack);
diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
index 8828b4aeb72b..455bb0787367 100644
--- a/arch/nds32/kernel/traps.c
+++ b/arch/nds32/kernel/traps.c
@@ -166,8 +166,6 @@ void dump_stack(void)
__dump(NULL, base_reg);
}
-EXPORT_SYMBOL(dump_stack);
-
void show_stack(struct task_struct *tsk, unsigned long *sp)
{
unsigned long *base_reg;
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index c5edbedd364d..02a8ad163760 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -25,7 +25,7 @@ static void __dump_stack(void)
#ifdef CONFIG_SMP
static atomic_t dump_lock = ATOMIC_INIT(-1);
-asmlinkage __visible void dump_stack(void)
+asmlinkage __weak __visible void dump_stack(void)
{
unsigned long flags;
int was_locked;
@@ -58,7 +58,7 @@ asmlinkage __visible void dump_stack(void)
local_irq_restore(flags);
}
#else
-asmlinkage __visible void dump_stack(void)
+asmlinkage __weak __visible void dump_stack(void)
{
__dump_stack();
}
--
2.16.2
On Mon 2018-03-05 14:37:42, Sergey Senozhatsky wrote:
> We want to move dump_stack related functions out of printk C
> code and consolidate them in lib/dump_stack file. The reason why
> dump_stack_print_info()/etc ended up in printk.c was a "generic"
> (dummy) lib dump_stack() function, which archs can override.
> For example, blackfin and nds32, define their own EXPORT_SYMBOL
> dump_stack() functions.
>
> In case of blackfin that arch-specific dump_stack() symbol invokes
> a global dump_stack_print_info() function. So we can't easily move
> dump_stack_print_info() to lib/dump_stack, because this way we will
> link with lib/dump_stack.o file, which will bring in a generic
> dump_stack() symbol with it, causing a multiple definitions error:
> - one dump_stack() from arch/blackfin/dumpstack
> - the other one from lib/dump_stack
>
> Convert generic dump_stack() into a weak symbol. So we will be
> able link with lib/dump_stack and at the same time archs will
> be able to override dump_stack(). It also opens up a way for us
> to move dump_stack_set_arch_desc(), dump_stack_print_info() and
> show_regs_print_info() to lib/dump_stack.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> ---
> arch/blackfin/kernel/dumpstack.c | 1 -
> arch/nds32/kernel/traps.c | 2 --
> lib/dump_stack.c | 4 ++--
> 3 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/blackfin/kernel/dumpstack.c b/arch/blackfin/kernel/dumpstack.c
> index 3c992c1f8ef2..61af017130cd 100644
> --- a/arch/blackfin/kernel/dumpstack.c
> +++ b/arch/blackfin/kernel/dumpstack.c
> @@ -174,4 +174,3 @@ void dump_stack(void)
> show_stack(current, &stack);
> trace_buffer_restore(tflags);
> }
> -EXPORT_SYMBOL(dump_stack);
I was afraid that blackfin modules would not longer be able
to use arch-specific dump_stack symbol. But it seems that only
the symbol name is important. Alos the list of symbols look
promissing:
before:
$> objdump -x vmlinux | less | grep dump_stack
00248530 l O __ksymtab 00000008 ___ksymtab_dump_stack
002500d4 l O __ksymtab_strings 0000000c ___kstrtab_dump_stack
00272bb6 l O .bss 00000080 _dump_stack_arch_desc_str
000051a8 g F .text 00000042 _dump_stack
002ab05c g F .init.text 0000002a _dump_stack_set_arch_desc
0003051c g F .text 000000a4 _dump_stack_print_info
after:
$> objdump -x vmlinux.patched | less | grep dump_stack
00000000 l df *ABS* 00000000 lib/dump_stack.c
0027c3e8 l O .bss 00000080 _dump_stack_arch_desc_str
00248580 l O __ksymtab 00000008 ___ksymtab_dump_stack
002653d4 l O __ksymtab_strings 0000000c ___kstrtab_dump_stack
000051a8 g F .text 00000042 _dump_stack
002b69dc g F .init.text 0000002a _dump_stack_set_arch_desc
001c2a90 g F .text 000000a4 _dump_stack_print_info
I hope that I did not miss anything. I could not try this at
runtime. I could just cross-compile.
Anyway, from my side:
Reviewed-by: Petr Mladek <[email protected]>
I'll wait a bit and push it into printk.git for-4.17.
Greentime Hu, you tested this on nds32. Could I use your Tested-by,
please?
Best Regards,
Petr
2018-03-05 22:48 GMT+08:00 Petr Mladek <[email protected]>:
> On Mon 2018-03-05 14:37:42, Sergey Senozhatsky wrote:
>> We want to move dump_stack related functions out of printk C
>> code and consolidate them in lib/dump_stack file. The reason why
>> dump_stack_print_info()/etc ended up in printk.c was a "generic"
>> (dummy) lib dump_stack() function, which archs can override.
>> For example, blackfin and nds32, define their own EXPORT_SYMBOL
>> dump_stack() functions.
>>
>> In case of blackfin that arch-specific dump_stack() symbol invokes
>> a global dump_stack_print_info() function. So we can't easily move
>> dump_stack_print_info() to lib/dump_stack, because this way we will
>> link with lib/dump_stack.o file, which will bring in a generic
>> dump_stack() symbol with it, causing a multiple definitions error:
>> - one dump_stack() from arch/blackfin/dumpstack
>> - the other one from lib/dump_stack
>>
>> Convert generic dump_stack() into a weak symbol. So we will be
>> able link with lib/dump_stack and at the same time archs will
>> be able to override dump_stack(). It also opens up a way for us
>> to move dump_stack_set_arch_desc(), dump_stack_print_info() and
>> show_regs_print_info() to lib/dump_stack.
>>
>> Signed-off-by: Sergey Senozhatsky <[email protected]>
>> ---
>> arch/blackfin/kernel/dumpstack.c | 1 -
>> arch/nds32/kernel/traps.c | 2 --
>> lib/dump_stack.c | 4 ++--
>> 3 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/blackfin/kernel/dumpstack.c b/arch/blackfin/kernel/dumpstack.c
>> index 3c992c1f8ef2..61af017130cd 100644
>> --- a/arch/blackfin/kernel/dumpstack.c
>> +++ b/arch/blackfin/kernel/dumpstack.c
>> @@ -174,4 +174,3 @@ void dump_stack(void)
>> show_stack(current, &stack);
>> trace_buffer_restore(tflags);
>> }
>> -EXPORT_SYMBOL(dump_stack);
>
> I was afraid that blackfin modules would not longer be able
> to use arch-specific dump_stack symbol. But it seems that only
> the symbol name is important. Alos the list of symbols look
> promissing:
>
> before:
>
> $> objdump -x vmlinux | less | grep dump_stack
> 00248530 l O __ksymtab 00000008 ___ksymtab_dump_stack
> 002500d4 l O __ksymtab_strings 0000000c ___kstrtab_dump_stack
> 00272bb6 l O .bss 00000080 _dump_stack_arch_desc_str
> 000051a8 g F .text 00000042 _dump_stack
> 002ab05c g F .init.text 0000002a _dump_stack_set_arch_desc
> 0003051c g F .text 000000a4 _dump_stack_print_info
>
>
> after:
>
> $> objdump -x vmlinux.patched | less | grep dump_stack
> 00000000 l df *ABS* 00000000 lib/dump_stack.c
> 0027c3e8 l O .bss 00000080 _dump_stack_arch_desc_str
> 00248580 l O __ksymtab 00000008 ___ksymtab_dump_stack
> 002653d4 l O __ksymtab_strings 0000000c ___kstrtab_dump_stack
> 000051a8 g F .text 00000042 _dump_stack
> 002b69dc g F .init.text 0000002a _dump_stack_set_arch_desc
> 001c2a90 g F .text 000000a4 _dump_stack_print_info
>
> I hope that I did not miss anything. I could not try this at
> runtime. I could just cross-compile.
>
> Anyway, from my side:
>
> Reviewed-by: Petr Mladek <[email protected]>
>
> I'll wait a bit and push it into printk.git for-4.17.
>
>
> Greentime Hu, you tested this on nds32. Could I use your Tested-by,
> please?
>
Yes, please use it. :)
greentime@atcsqa02:/NOBACKUP/sqa2/greentime/contrib/src_pkg/linux-next
<next-20180302> $ nds32le-elf-objdump -x vmlinux | less | grep
dump_stack
00000000 l df *ABS* 00000000 dump_stack.c
b04f7910 l O .bss 00000080 dump_stack_arch_desc_str
b04995e8 l O __ksymtab 00000008 __ksymtab_dump_stack
b04b9086 l O __ksymtab_strings 0000000b __kstrtab_dump_stack
b002a464 g F .text 00000022 dump_stack
b03a568c g F .text 000000c2 dump_stack_print_info
b0024cf4 g F .init.text 00000038 dump_stack_set_arch_desc
On (03/05/18 15:48), Petr Mladek wrote:
[..]
>
> I hope that I did not miss anything. I could not try this at
> runtime.
I think you can. The rules are universal, you can do on x86
something like this
---
arch/x86/kernel/dumpstack.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index a2d8a3908670..5d45f406717e 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -375,3 +375,16 @@ static int __init code_bytes_setup(char *s)
return 1;
}
__setup("code_bytes=", code_bytes_setup);
+
+void dump_stack(void)
+{
+ dump_stack_print_info(KERN_DEFAULT);
+
+ pr_crit("\t\tLinux\n\n");
+
+ pr_crit("An error has occurred. To continue:\n"
+ "Press Enter to return to Linux, or\n"
+ "Press CTRL+ALT+DEL to restart your computer.\n");
+
+ pr_crit("\n\n\tPress any key to continue _");
+}
---
Should be enough for testing.
> Anyway, from my side:
>
> Reviewed-by: Petr Mladek <[email protected]>
Thanks.
-ss
On (03/06/18 10:50), Greentime Hu wrote:
[..]
> > Greentime Hu, you tested this on nds32. Could I use your Tested-by,
> > please?
> >
>
> Yes, please use it. :)
Thanks.
To be sure, is this
Tested-by: Greentime Hu <[email protected]> # nds32
or
Acked-by: Greentime Hu <[email protected]> # nds32
?
-ss
2018-03-06 12:31 GMT+08:00 Sergey Senozhatsky
<[email protected]>:
> On (03/06/18 10:50), Greentime Hu wrote:
> [..]
>> > Greentime Hu, you tested this on nds32. Could I use your Tested-by,
>> > please?
>> >
>>
>> Yes, please use it. :)
>
> Thanks.
>
> To be sure, is this
>
> Tested-by: Greentime Hu <[email protected]> # nds32
> or
> Acked-by: Greentime Hu <[email protected]> # nds32
>
Acked-by is prefered.
Thanks.
On Tue 2018-03-06 13:29:57, Sergey Senozhatsky wrote:
> On (03/05/18 15:48), Petr Mladek wrote:
> [..]
> >
> > I hope that I did not miss anything. I could not try this at
> > runtime.
>
> I think you can. The rules are universal, you can do on x86
> something like this
>
> ---
>
> arch/x86/kernel/dumpstack.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index a2d8a3908670..5d45f406717e 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -375,3 +375,16 @@ static int __init code_bytes_setup(char *s)
> return 1;
> }
> __setup("code_bytes=", code_bytes_setup);
> +
> +void dump_stack(void)
> +{
> + dump_stack_print_info(KERN_DEFAULT);
> +
> + pr_crit("\t\tLinux\n\n");
> +
> + pr_crit("An error has occurred. To continue:\n"
> + "Press Enter to return to Linux, or\n"
> + "Press CTRL+ALT+DEL to restart your computer.\n");
> +
> + pr_crit("\n\n\tPress any key to continue _");
> +}
>
> ---
>
> Should be enough for testing.
Yup, this worked.
I have pushed the patch into printk.git for-4.17 branch,
see https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/commit/?h=for-4.17&id=33251b634b4aa1317e2f911e3b723179949a0605
Greentime Hu,
the for-4.17 branch in printk.git is based on Linus' tree. Therefore
I had to remove the hunk against arch/nds32/kernel/traps.c because
this file is only in linux-next.
I think that it might be easier if you remove the EXPORT_SYMBOL()
in your branch. Is it OK for you, please?
Best Regards,
Petr
On Mon, Mar 5, 2018 at 6:37 AM, Sergey Senozhatsky
<[email protected]> wrote:
> We want to move dump_stack related functions out of printk C
> code and consolidate them in lib/dump_stack file. The reason why
> dump_stack_print_info()/etc ended up in printk.c was a "generic"
> (dummy) lib dump_stack() function, which archs can override.
> For example, blackfin and nds32, define their own EXPORT_SYMBOL
> dump_stack() functions.
>
> In case of blackfin that arch-specific dump_stack() symbol invokes
> a global dump_stack_print_info() function. So we can't easily move
> dump_stack_print_info() to lib/dump_stack, because this way we will
> link with lib/dump_stack.o file, which will bring in a generic
> dump_stack() symbol with it, causing a multiple definitions error:
> - one dump_stack() from arch/blackfin/dumpstack
> - the other one from lib/dump_stack
>
> Convert generic dump_stack() into a weak symbol. So we will be
> able link with lib/dump_stack and at the same time archs will
> be able to override dump_stack(). It also opens up a way for us
> to move dump_stack_set_arch_desc(), dump_stack_print_info() and
> show_regs_print_info() to lib/dump_stack.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> ---
> arch/blackfin/kernel/dumpstack.c | 1 -
> arch/nds32/kernel/traps.c | 2 --
> lib/dump_stack.c | 4 ++--
> 3 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/blackfin/kernel/dumpstack.c b/arch/blackfin/kernel/dumpstack.c
> index 3c992c1f8ef2..61af017130cd 100644
> --- a/arch/blackfin/kernel/dumpstack.c
> +++ b/arch/blackfin/kernel/dumpstack.c
> @@ -174,4 +174,3 @@ void dump_stack(void)
> show_stack(current, &stack);
> trace_buffer_restore(tflags);
> }
> -EXPORT_SYMBOL(dump_stack);
As we are now removing blackfin, based on the latest discussion, this
part should no longer be necessary.
> diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
> index 8828b4aeb72b..455bb0787367 100644
> --- a/arch/nds32/kernel/traps.c
> +++ b/arch/nds32/kernel/traps.c
> @@ -166,8 +166,6 @@ void dump_stack(void)
> __dump(NULL, base_reg);
> }
>
> -EXPORT_SYMBOL(dump_stack);
> -
> void show_stack(struct task_struct *tsk, unsigned long *sp)
> {
> unsigned long *base_reg;
nds32 currently only exists in linux-next, not in the mainline kernel.
If it's the only architecture that does something different from everyone
else, I think we should change nds32.
I looked at the nds32 show_stack() implementation now and it
seems to me that it is completely unnecessary, as the implementation
from lib/dump_stack.c does basically the same thing (by calling
show_stack(NULL, NULL)).
> diff --git a/lib/dump_stack.c b/lib/dump_stack.c
> index c5edbedd364d..02a8ad163760 100644
> --- a/lib/dump_stack.c
> +++ b/lib/dump_stack.c
> @@ -25,7 +25,7 @@ static void __dump_stack(void)
> #ifdef CONFIG_SMP
> static atomic_t dump_lock = ATOMIC_INIT(-1);
>
> -asmlinkage __visible void dump_stack(void)
> +asmlinkage __weak __visible void dump_stack(void)
> {
> unsigned long flags;
> int was_locked;
> @@ -58,7 +58,7 @@ asmlinkage __visible void dump_stack(void)
> local_irq_restore(flags);
> }
> #else
> -asmlinkage __visible void dump_stack(void)
> +asmlinkage __weak __visible void dump_stack(void)
> {
> __dump_stack();
> }
Weak symbols are generally discouraged in the kernel. We have
them in a couple of places, but I find them rather confusing as they
make it harder to figure out what is actually going on.
Arnd
Hi all,
On Tue, 6 Mar 2018 13:43:26 +0100 Petr Mladek <[email protected]> wrote:
>
> Greentime Hu,
>
> the for-4.17 branch in printk.git is based on Linus' tree. Therefore
> I had to remove the hunk against arch/nds32/kernel/traps.c because
> this file is only in linux-next.
>
> I think that it might be easier if you remove the EXPORT_SYMBOL()
> in your branch. Is it OK for you, please?
I have added the following as a merge resolution to the linux-next
merge of the printk tree from today (someone will need to mention this
to Linus when the printk and nds32 trees meet in Linus' tree):
From: Stephen Rothwell <[email protected]>
Date: Wed, 7 Mar 2018 09:47:26 +1100
Subject: [PATCH] nds32: merge fix for dump_stack update
Signed-off-by: Stephen Rothwell <[email protected]>
---
arch/nds32/kernel/traps.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
index 8828b4aeb72b..455bb0787367 100644
--- a/arch/nds32/kernel/traps.c
+++ b/arch/nds32/kernel/traps.c
@@ -166,8 +166,6 @@ void dump_stack(void)
__dump(NULL, base_reg);
}
-EXPORT_SYMBOL(dump_stack);
-
void show_stack(struct task_struct *tsk, unsigned long *sp)
{
unsigned long *base_reg;
--
2.16.1
--
Cheers,
Stephen Rothwell
Hello Arnd,
On (03/06/18 14:27), Arnd Bergmann wrote:
[..]
> As we are now removing blackfin, based on the latest discussion, this
> part should no longer be necessary.
When is this going to happen? 4.17?
[..]
> nds32 currently only exists in linux-next, not in the mainline kernel.
> If it's the only architecture that does something different from everyone
> else, I think we should change nds32.
>
> I looked at the nds32 show_stack() implementation now and it
> seems to me that it is completely unnecessary, as the implementation
> from lib/dump_stack.c does basically the same thing (by calling
> show_stack(NULL, NULL)).
Interesting point. I'll leave it to nds32 maintainers.
OTOH blackfin is still in linux-next, so I assume we need
that __weak dump_stack() for the time being.
[..]
> > +asmlinkage __weak __visible void dump_stack(void)
> > {
> > __dump_stack();
> > }
>
> Weak symbols are generally discouraged in the kernel. We have
> them in a couple of places, but I find them rather confusing as they
> make it harder to figure out what is actually going on.
Honestly, I kind of find __weak less confusing than EXPORT_SYMBOL(dump_stack)
in 3 different places. __weak hints that the symbol likely will be overridden
somewhere, while EXPORT_SYMBOL() does not (at least not to me). Dunno.
-ss
On Wed, Mar 7, 2018 at 3:21 AM, Sergey Senozhatsky
<[email protected]> wrote:
> Hello Arnd,
>
> On (03/06/18 14:27), Arnd Bergmann wrote:
> [..]
>> As we are now removing blackfin, based on the latest discussion, this
>> part should no longer be necessary.
>
> When is this going to happen? 4.17?
Originally I planned to wait a few more releases, but the last maintainer
has commented that he will now send a patch for immediate removal,
so 4.17 is almost certain at this point.
> [..]
>> nds32 currently only exists in linux-next, not in the mainline kernel.
>> If it's the only architecture that does something different from everyone
>> else, I think we should change nds32.
>>
>> I looked at the nds32 show_stack() implementation now and it
>> seems to me that it is completely unnecessary, as the implementation
>> from lib/dump_stack.c does basically the same thing (by calling
>> show_stack(NULL, NULL)).
>
> Interesting point. I'll leave it to nds32 maintainers.
> OTOH blackfin is still in linux-next, so I assume we need
> that __weak dump_stack() for the time being.
I did the review of all the nds32 patches, and would have commented
on this if I had noticed it earlier. I see no reason not to change it,
and would suggest that you continue under the assumption that
nds32 is going to be fixed, leaving it up to Greentime to add a fix
to his tree before he sends the pull request.
>> > +asmlinkage __weak __visible void dump_stack(void)
>> > {
>> > __dump_stack();
>> > }
>>
>> Weak symbols are generally discouraged in the kernel. We have
>> them in a couple of places, but I find them rather confusing as they
>> make it harder to figure out what is actually going on.
>
> Honestly, I kind of find __weak less confusing than EXPORT_SYMBOL(dump_stack)
> in 3 different places. __weak hints that the symbol likely will be overridden
> somewhere, while EXPORT_SYMBOL() does not (at least not to me). Dunno.
It's not either/or, they are both wrong ;-)
The EXPORT_SYMBOL() is not the thing that makes it work. The duplicate
declaration today only works because lib/dump_stack.o is listed as lib-y
in the Makefile instead of obj-y. On blackfin and nds32, this causes the entire
file to just not be included in the final vmlinux, because there are no
references to it.
With your patch, I would actually expect the lib/dump_stack.o file
to still not be picked up, so now you have a missing EXPORT_SYMBOL()
on the two unusual architectures until the point when you add another
(referenced) symbol to it.
Arnd
On Wed 2018-03-07 11:21:27, Sergey Senozhatsky wrote:
> Hello Arnd,
>
> On (03/06/18 14:27), Arnd Bergmann wrote:
> [..]
> > As we are now removing blackfin, based on the latest discussion, this
> > part should no longer be necessary.
>
> When is this going to happen? 4.17?
>
> [..]
> > nds32 currently only exists in linux-next, not in the mainline kernel.
> > If it's the only architecture that does something different from everyone
> > else, I think we should change nds32.
> >
> > I looked at the nds32 show_stack() implementation now and it
> > seems to me that it is completely unnecessary, as the implementation
> > from lib/dump_stack.c does basically the same thing (by calling
> > show_stack(NULL, NULL)).
>
> Interesting point. I'll leave it to nds32 maintainers.
> OTOH blackfin is still in linux-next, so I assume we need
> that __weak dump_stack() for the time being.
My understanding is that blacfin will not be removed in the first
wave. Therefore we would need to do something about it for 4.17.
Or is there any new info, Arnd?
> [..]
> > > +asmlinkage __weak __visible void dump_stack(void)
> > > {
> > > __dump_stack();
> > > }
> >
> > Weak symbols are generally discouraged in the kernel. We have
> > them in a couple of places, but I find them rather confusing as they
> > make it harder to figure out what is actually going on.
I agree that using weak symbols might be confusing. But I wonder what
is the preferred alternative when only few architectures do something
slightly different.
> Honestly, I kind of find __weak less confusing than EXPORT_SYMBOL(dump_stack)
> in 3 different places. __weak hints that the symbol likely will be overridden
> somewhere, while EXPORT_SYMBOL() does not (at least not to me). Dunno.
The trick used for dump_stack() is really confusing. I mean the
linking of lib/dump_stack.o only when there is no arch-specific
variant.
Best Regards,
Petr
Ah, there was a mid-air collision. Arnd already answered most of my
questions and even more.
On Wed 2018-03-07 09:46:27, Arnd Bergmann wrote:
> On Wed, Mar 7, 2018 at 3:21 AM, Sergey Senozhatsky
> <[email protected]> wrote:
> > On (03/06/18 14:27), Arnd Bergmann wrote:
> >> Weak symbols are generally discouraged in the kernel. We have
> >> them in a couple of places, but I find them rather confusing as they
> >> make it harder to figure out what is actually going on.
> >
> > Honestly, I kind of find __weak less confusing than EXPORT_SYMBOL(dump_stack)
> > in 3 different places. __weak hints that the symbol likely will be overridden
> > somewhere, while EXPORT_SYMBOL() does not (at least not to me). Dunno.
>
> It's not either/or, they are both wrong ;-)
>
> The EXPORT_SYMBOL() is not the thing that makes it work. The duplicate
> declaration today only works because lib/dump_stack.o is listed as lib-y
> in the Makefile instead of obj-y. On blackfin and nds32, this causes the entire
> file to just not be included in the final vmlinux, because there are no
> references to it.
>
> With your patch, I would actually expect the lib/dump_stack.o file
> to still not be picked up, so now you have a missing EXPORT_SYMBOL()
> on the two unusual architectures until the point when you add another
> (referenced) symbol to it.
Great catch! We should change it from lib-y to obj-y. Of course,
the best solution would be if nds32 uses the generic implementation
and we could avoid adding __weak.
Best Regards,
Petr
2018-03-07 17:09 GMT+08:00 Petr Mladek <[email protected]>:
> Ah, there was a mid-air collision. Arnd already answered most of my
> questions and even more.
>
> On Wed 2018-03-07 09:46:27, Arnd Bergmann wrote:
>> On Wed, Mar 7, 2018 at 3:21 AM, Sergey Senozhatsky
>> <[email protected]> wrote:
>> > On (03/06/18 14:27), Arnd Bergmann wrote:
>> >> Weak symbols are generally discouraged in the kernel. We have
>> >> them in a couple of places, but I find them rather confusing as they
>> >> make it harder to figure out what is actually going on.
>> >
>> > Honestly, I kind of find __weak less confusing than EXPORT_SYMBOL(dump_stack)
>> > in 3 different places. __weak hints that the symbol likely will be overridden
>> > somewhere, while EXPORT_SYMBOL() does not (at least not to me). Dunno.
>>
>> It's not either/or, they are both wrong ;-)
>>
>> The EXPORT_SYMBOL() is not the thing that makes it work. The duplicate
>> declaration today only works because lib/dump_stack.o is listed as lib-y
>> in the Makefile instead of obj-y. On blackfin and nds32, this causes the entire
>> file to just not be included in the final vmlinux, because there are no
>> references to it.
>>
>> With your patch, I would actually expect the lib/dump_stack.o file
>> to still not be picked up, so now you have a missing EXPORT_SYMBOL()
>> on the two unusual architectures until the point when you add another
>> (referenced) symbol to it.
>
> Great catch! We should change it from lib-y to obj-y. Of course,
> the best solution would be if nds32 uses the generic implementation
> and we could avoid adding __weak.
>
I agree too.
I think I can add a patch to remove the dump_stack() implementation in
nds32 and use the generic one.
diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
index 8828b4aeb72b..6e34eb9824a4 100644
--- a/arch/nds32/kernel/traps.c
+++ b/arch/nds32/kernel/traps.c
@@ -156,18 +156,6 @@ static void __dump(struct task_struct *tsk,
unsigned long *base_reg)
pr_emerg("\n");
}
-void dump_stack(void)
-{
- unsigned long *base_reg;
- if (!IS_ENABLED(CONFIG_FRAME_POINTER))
- __asm__ __volatile__("\tori\t%0, $sp, #0\n":"=r"(base_reg));
- else
- __asm__ __volatile__("\tori\t%0, $fp, #0\n":"=r"(base_reg));
- __dump(NULL, base_reg);
-}
-
-EXPORT_SYMBOL(dump_stack);
-
On Wed, Mar 7, 2018 at 10:44 AM, Greentime Hu <[email protected]> wrote:
> 2018-03-07 17:09 GMT+08:00 Petr Mladek <[email protected]>:
>> Ah, there was a mid-air collision. Arnd already answered most of my
>> questions and even more.
>>
>> On Wed 2018-03-07 09:46:27, Arnd Bergmann wrote:
>>> On Wed, Mar 7, 2018 at 3:21 AM, Sergey Senozhatsky
>>> <[email protected]> wrote:
>>> > On (03/06/18 14:27), Arnd Bergmann wrote:
>>> >> Weak symbols are generally discouraged in the kernel. We have
>>> >> them in a couple of places, but I find them rather confusing as they
>>> >> make it harder to figure out what is actually going on.
>>> >
>>> > Honestly, I kind of find __weak less confusing than EXPORT_SYMBOL(dump_stack)
>>> > in 3 different places. __weak hints that the symbol likely will be overridden
>>> > somewhere, while EXPORT_SYMBOL() does not (at least not to me). Dunno.
>>>
>>> It's not either/or, they are both wrong ;-)
>>>
>>> The EXPORT_SYMBOL() is not the thing that makes it work. The duplicate
>>> declaration today only works because lib/dump_stack.o is listed as lib-y
>>> in the Makefile instead of obj-y. On blackfin and nds32, this causes the entire
>>> file to just not be included in the final vmlinux, because there are no
>>> references to it.
>>>
>>> With your patch, I would actually expect the lib/dump_stack.o file
>>> to still not be picked up, so now you have a missing EXPORT_SYMBOL()
>>> on the two unusual architectures until the point when you add another
>>> (referenced) symbol to it.
>>
>> Great catch! We should change it from lib-y to obj-y. Of course,
>> the best solution would be if nds32 uses the generic implementation
>> and we could avoid adding __weak.
>>
>
> I agree too.
> I think I can add a patch to remove the dump_stack() implementation in
> nds32 and use the generic one.
Looks good,
Acked-by: Arnd Bergmann <[email protected]>
> diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
> index 8828b4aeb72b..6e34eb9824a4 100644
> --- a/arch/nds32/kernel/traps.c
> +++ b/arch/nds32/kernel/traps.c
> @@ -156,18 +156,6 @@ static void __dump(struct task_struct *tsk,
> unsigned long *base_reg)
> pr_emerg("\n");
> }
>
> -void dump_stack(void)
> -{
> - unsigned long *base_reg;
> - if (!IS_ENABLED(CONFIG_FRAME_POINTER))
> - __asm__ __volatile__("\tori\t%0, $sp, #0\n":"=r"(base_reg));
> - else
> - __asm__ __volatile__("\tori\t%0, $fp, #0\n":"=r"(base_reg));
> - __dump(NULL, base_reg);
> -}
> -
> -EXPORT_SYMBOL(dump_stack);
> -
On (03/07/18 09:46), Arnd Bergmann wrote:
> >
> > When is this going to happen? 4.17?
>
> Originally I planned to wait a few more releases, but the last maintainer
> has commented that he will now send a patch for immediate removal,
> so 4.17 is almost certain at this point.
Would be great to get it removed as soon as possible then. Otherwise we
will get broken blackfin build errors from Stephen (or would need to hold
off Dave's patch).
> > [..]
> I did the review of all the nds32 patches, and would have commented
> on this if I had noticed it earlier. I see no reason not to change it,
> and would suggest that you continue under the assumption that
> nds32 is going to be fixed, leaving it up to Greentime to add a fix
> to his tree before he sends the pull request.
Good.
> > Honestly, I kind of find __weak less confusing than EXPORT_SYMBOL(dump_stack)
> > in 3 different places. __weak hints that the symbol likely will be overridden
> > somewhere, while EXPORT_SYMBOL() does not (at least not to me). Dunno.
>
> It's not either/or, they are both wrong ;-)
>
> The EXPORT_SYMBOL() is not the thing that makes it work. The duplicate
> declaration today only works because lib/dump_stack.o is listed as lib-y
> in the Makefile instead of obj-y. On blackfin and nds32, this causes the entire
> file to just not be included in the final vmlinux, because there are no
> references to it.
Yeah.
But, admittedly, I learned that "obj-y vs lib-y" stuff quite recently.
> With your patch, I would actually expect the lib/dump_stack.o file
> to still not be picked up, so now you have a missing EXPORT_SYMBOL()
> on the two unusual architectures until the point when you add another
> (referenced) symbol to it.
Interesting point. Didn't check it. But I checked that we have at least
one reference to lib/dump_stack from every arch so __weak could work its
magic. The function is show_regs_print_info(). AFAICT, every arch calls
it (we have it in lib/dump_stack now, so we will link with lib/dump_stack).
Anyway, I'll be happy to drop my patch. Thanks for taking a look.
-ss
On Wed, Mar 7, 2018 at 11:40 AM, Sergey Senozhatsky
<[email protected]> wrote:
> On (03/07/18 09:46), Arnd Bergmann wrote:
>> >
>> > When is this going to happen? 4.17?
>>
>> Originally I planned to wait a few more releases, but the last maintainer
>> has commented that he will now send a patch for immediate removal,
>> so 4.17 is almost certain at this point.
>
> Would be great to get it removed as soon as possible then. Otherwise we
> will get broken blackfin build errors from Stephen (or would need to hold
> off Dave's patch).
You could also add a patch to your tree that removes the blackfin
dump_stack() function, or we could ask Stephen and the other
people operating build bots to stop building blackfin right now
(they will have to do that anyway once the arch gets removed).
>> With your patch, I would actually expect the lib/dump_stack.o file
>> to still not be picked up, so now you have a missing EXPORT_SYMBOL()
>> on the two unusual architectures until the point when you add another
>> (referenced) symbol to it.
>
> Interesting point. Didn't check it. But I checked that we have at least
> one reference to lib/dump_stack from every arch so __weak could work its
> magic. The function is show_regs_print_info(). AFAICT, every arch calls
> it (we have it in lib/dump_stack now, so we will link with lib/dump_stack).
> Anyway, I'll be happy to drop my patch. Thanks for taking a look.
Ah, right, that is after your second patch. So after the first one, it might
be broken, but the follow-up patch fixes it.
Since lib/dump_stack.c is mandatory then, I would suggest making it
obj-y and moving it out of lib/ into kernel/printk/.
Arnd
On (03/07/18 11:57), Arnd Bergmann wrote:
[..]
> >> Originally I planned to wait a few more releases, but the last maintainer
> >> has commented that he will now send a patch for immediate removal,
> >> so 4.17 is almost certain at this point.
> >
> > Would be great to get it removed as soon as possible then. Otherwise we
> > will get broken blackfin build errors from Stephen (or would need to hold
> > off Dave's patch).
>
> You could also add a patch to your tree that removes the blackfin
> dump_stack() function, or we could ask Stephen and the other
> people operating build bots to stop building blackfin right now
> (they will have to do that anyway once the arch gets removed).
OK. Petr, Stephen, what would be your preference?
> Since lib/dump_stack.c is mandatory then, I would suggest making it
> obj-y and moving it out of lib/ into kernel/printk/.
Totally agree on obj-y. And tend to agree on moving lib/dump_stack to
kernel/printk. lib/dump_stack depends on CONFIG_PRINTK and is partially
in printk.h. So it _mostly_ seems like the right place after all.
-ss
Hi Sergey,
On Wed, 7 Mar 2018 20:48:56 +0900 Sergey Senozhatsky <[email protected]> wrote:
>
> On (03/07/18 11:57), Arnd Bergmann wrote:
> [..]
> > >> Originally I planned to wait a few more releases, but the last maintainer
> > >> has commented that he will now send a patch for immediate removal,
> > >> so 4.17 is almost certain at this point.
> > >
> > > Would be great to get it removed as soon as possible then. Otherwise we
> > > will get broken blackfin build errors from Stephen (or would need to hold
> > > off Dave's patch).
> >
> > You could also add a patch to your tree that removes the blackfin
> > dump_stack() function, or we could ask Stephen and the other
> > people operating build bots to stop building blackfin right now
> > (they will have to do that anyway once the arch gets removed).
>
> OK. Petr, Stephen, what would be your preference?
I can easily stop building blackfin - and, if the intention is to
remove it, there is not much point in wasting resources building it any
anyway.
--
Cheers,
Stephen Rothwell
On Wed, Mar 7, 2018 at 1:41 PM, Stephen Rothwell <[email protected]> wrote:
> Hi Sergey,
>
> On Wed, 7 Mar 2018 20:48:56 +0900 Sergey Senozhatsky <[email protected]> wrote:
>>
>> On (03/07/18 11:57), Arnd Bergmann wrote:
>> [..]
>> > >> Originally I planned to wait a few more releases, but the last maintainer
>> > >> has commented that he will now send a patch for immediate removal,
>> > >> so 4.17 is almost certain at this point.
>> > >
>> > > Would be great to get it removed as soon as possible then. Otherwise we
>> > > will get broken blackfin build errors from Stephen (or would need to hold
>> > > off Dave's patch).
>> >
>> > You could also add a patch to your tree that removes the blackfin
>> > dump_stack() function, or we could ask Stephen and the other
>> > people operating build bots to stop building blackfin right now
>> > (they will have to do that anyway once the arch gets removed).
>>
>> OK. Petr, Stephen, what would be your preference?
>
> I can easily stop building blackfin - and, if the intention is to
> remove it, there is not much point in wasting resources building it any
> anyway.
Right. At the moment, the plan is to remove metag, score, unicore32,
m32r, frv and blackfin. If you are building any more of those, you can
stop that as well.
The fate of tile and mn10300 is still open, but I suspect they won't
last long either.
Arnd
Hi Arnd,
On Wed, 7 Mar 2018 13:48:09 +0100 Arnd Bergmann <[email protected]> wrote:
>
> Right. At the moment, the plan is to remove metag, score, unicore32,
> m32r, frv and blackfin. If you are building any more of those, you can
> stop that as well.
OK, thanks. Will I get a tree with those removal commits in linux-next
(sometime soon)? (I already have the metag removal via the metag tree.)
> The fate of tile and mn10300 is still open, but I suspect they won't
> last long either.
Noted.
--
Cheers,
Stephen Rothwell
On Wed, Mar 7, 2018 at 1:53 PM, Stephen Rothwell <[email protected]> wrote:
> Hi Arnd,
>
> On Wed, 7 Mar 2018 13:48:09 +0100 Arnd Bergmann <[email protected]> wrote:
>>
>> Right. At the moment, the plan is to remove metag, score, unicore32,
>> m32r, frv and blackfin. If you are building any more of those, you can
>> stop that as well.
>
> OK, thanks. Will I get a tree with those removal commits in linux-next
> (sometime soon)? (I already have the metag removal via the metag tree.)
Yes, I should do that any day now, but first have a stack of arm-soc pull
requests to take care of.
I was planning to add these to my asm-generic tree, which is already
in linux-next, and related (enough).
Arnd
Hi Arnd,
On Wed, 7 Mar 2018 13:58:13 +0100 Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Mar 7, 2018 at 1:53 PM, Stephen Rothwell <[email protected]> wrote:
> > Hi Arnd,
> >
> > On Wed, 7 Mar 2018 13:48:09 +0100 Arnd Bergmann <[email protected]> wrote:
> >>
> >> Right. At the moment, the plan is to remove metag, score, unicore32,
> >> m32r, frv and blackfin. If you are building any more of those, you can
> >> stop that as well.
> >
> > OK, thanks. Will I get a tree with those removal commits in linux-next
> > (sometime soon)? (I already have the metag removal via the metag tree.)
>
> Yes, I should do that any day now, but first have a stack of arm-soc pull
> requests to take care of.
>
> I was planning to add these to my asm-generic tree, which is already
> in linux-next, and related (enough).
Excellent, thanks.
--
Cheers,
Stephen Rothwell
On (03/07/18 13:48), Arnd Bergmann wrote:
[..]
> >
> > I can easily stop building blackfin - and, if the intention is to
> > remove it, there is not much point in wasting resources building it any
> > anyway.
>
> Right. At the moment, the plan is to remove metag, score, unicore32,
> m32r, frv and blackfin. If you are building any more of those, you can
> stop that as well.
>
> The fate of tile and mn10300 is still open, but I suspect they won't
> last long either.
That's a huge list.
I heard that CRIS is having some problems as well:
https://lkml.org/lkml/2018/1/11/403
-ss
Hi,
On (03/07/18 23:41), Stephen Rothwell wrote:
[..]
> > OK. Petr, Stephen, what would be your preference?
>
> I can easily stop building blackfin - and, if the intention is to
> remove it, there is not much point in wasting resources building it any
> anyway.
OK, sounds good to me.
Then we can drop my dump_stack patch from printk.git and keep
Dave's patch.
Thanks guys.
-ss
On Wed 2018-03-07 20:48:56, Sergey Senozhatsky wrote:
> On (03/07/18 11:57), Arnd Bergmann wrote:
> > Since lib/dump_stack.c is mandatory then, I would suggest making it
> > obj-y and moving it out of lib/ into kernel/printk/.
>
> Totally agree on obj-y. And tend to agree on moving lib/dump_stack to
> kernel/printk. lib/dump_stack depends on CONFIG_PRINTK and is partially
> in printk.h. So it _mostly_ seems like the right place after all.
I agree that we should define dump_stack.o as obj-y. Also it makes
sense to move it to kernel/printk/. As Sergey said: it depends on
CONFIG_PRINTK and is in printk.h.
Steven, do you agree or would you suggest another location?
It was you who suggested to move the several functions from printk.c
to dump_stack.c. IMHO, the motivation was to fix responsibilities
(maintainer-ship) of this stuff. This will not really happen if
we move the file to printk directory.
Best Regards,
Petr
On Fri, 9 Mar 2018 10:50:55 +0100
Petr Mladek <[email protected]> wrote:
> On Wed 2018-03-07 20:48:56, Sergey Senozhatsky wrote:
> > On (03/07/18 11:57), Arnd Bergmann wrote:
> > > Since lib/dump_stack.c is mandatory then, I would suggest making it
> > > obj-y and moving it out of lib/ into kernel/printk/.
> >
> > Totally agree on obj-y. And tend to agree on moving lib/dump_stack to
> > kernel/printk. lib/dump_stack depends on CONFIG_PRINTK and is partially
> > in printk.h. So it _mostly_ seems like the right place after all.
>
> I agree that we should define dump_stack.o as obj-y. Also it makes
> sense to move it to kernel/printk/. As Sergey said: it depends on
> CONFIG_PRINTK and is in printk.h.
>
> Steven, do you agree or would you suggest another location?
> It was you who suggested to move the several functions from printk.c
> to dump_stack.c. IMHO, the motivation was to fix responsibilities
> (maintainer-ship) of this stuff. This will not really happen if
> we move the file to printk directory.
>
I still would like to keep it in lib. If blackfin is going to be nuked
in 4.17, and nds32 is the only one that is causing a issue. I would
highly recommend fixing nds32 and have Stephen stop building blackfin.
-- Steve
Hi Steve,
On Wed, 14 Mar 2018 19:42:37 -0400 Steven Rostedt <[email protected]> wrote:
>
> I still would like to keep it in lib. If blackfin is going to be nuked
> in 4.17, and nds32 is the only one that is causing a issue. I would
> highly recommend fixing nds32 and have Stephen stop building blackfin.
nds32 was "fixed" a few days ago and I stopped building blackfin a while
ago as well.
--
Cheers,
Stephen Rothwell