* [PATCH] x86/platform/uv: make const pointer dots a static const array
@ 2021-11-27 17:03 Colin Ian King
2021-11-30 19:34 ` Steve Wahl
0 siblings, 1 reply; 6+ messages in thread
From: Colin Ian King @ 2021-11-27 17:03 UTC (permalink / raw)
To: Steve Wahl, Mike Travis, Dimitri Sivanich, Russ Anderson,
Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H . Peter Anvin,
platform-driver-x86
Cc: kernel-janitors, linux-kernel
Don't populate the const array dots on the stack but make it static
const and make the pointer an array to remove a dereference. Shrinks
object code a few bytes too.
Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
---
arch/x86/platform/uv/uv_nmi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
index 1e9ff28bc2e0..2c69a0c30632 100644
--- a/arch/x86/platform/uv/uv_nmi.c
+++ b/arch/x86/platform/uv/uv_nmi.c
@@ -725,7 +725,7 @@ static void uv_nmi_dump_cpu_ip(int cpu, struct pt_regs *regs)
*/
static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
{
- const char *dots = " ................................. ";
+ static const char dots[] = " ................................. ";
if (cpu == 0)
uv_nmi_dump_cpu_ip_hdr();
--
2.33.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/platform/uv: make const pointer dots a static const array
2021-11-27 17:03 [PATCH] x86/platform/uv: make const pointer dots a static const array Colin Ian King
@ 2021-11-30 19:34 ` Steve Wahl
2021-12-01 0:26 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Steve Wahl @ 2021-11-30 19:34 UTC (permalink / raw)
To: Colin Ian King
Cc: Steve Wahl, Mike Travis, Dimitri Sivanich, Russ Anderson,
Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H . Peter Anvin,
platform-driver-x86, kernel-janitors, linux-kernel
On Sat, Nov 27, 2021 at 05:03:20PM +0000, Colin Ian King wrote:
> Don't populate the const array dots on the stack
That is a misunderstanding of what the original code does. The
original code has a constant char array (string constant) that is
placed in an initialized data section of memory, the address off which
would be assigned to the pointer "dots" on the stack -- to be clear,
stack contents would not be a full array, but a pointer to it. Then
that pointer would be passed to the pr_info function (which boils down
to a call to printk).
Examination of the disassembly shows that the compiler actually
eliminates the creation of the pointer "dots" on the stack and just
passes the address of the string constant to the printk function.
So this change should not have any actual effect (I don't know where
you got the "shrinks object code" from), and in my humble opinion
makes the code less clear.
As such, unless there's something here I don't understand, I vote to
reject this patch.
--> Steve Wahl <steve.wahl@hpe.com>
> but make it static
> const and make the pointer an array to remove a dereference. Shrinks
> object code a few bytes too.
>
> Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
> ---
> arch/x86/platform/uv/uv_nmi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
> index 1e9ff28bc2e0..2c69a0c30632 100644
> --- a/arch/x86/platform/uv/uv_nmi.c
> +++ b/arch/x86/platform/uv/uv_nmi.c
> @@ -725,7 +725,7 @@ static void uv_nmi_dump_cpu_ip(int cpu, struct pt_regs *regs)
> */
> static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
> {
> - const char *dots = " ................................. ";
> + static const char dots[] = " ................................. ";
>
> if (cpu == 0)
> uv_nmi_dump_cpu_ip_hdr();
> --
> 2.33.1
>
--
Steve Wahl, Hewlett Packard Enterprise
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/platform/uv: make const pointer dots a static const array
2021-11-30 19:34 ` Steve Wahl
@ 2021-12-01 0:26 ` Joe Perches
2021-12-01 21:39 ` Steve Wahl
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2021-12-01 0:26 UTC (permalink / raw)
To: Steve Wahl, Colin Ian King
Cc: Mike Travis, Dimitri Sivanich, Russ Anderson, Darren Hart,
Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H . Peter Anvin, platform-driver-x86,
kernel-janitors, linux-kernel
On Tue, 2021-11-30 at 13:34 -0600, Steve Wahl wrote:
> On Sat, Nov 27, 2021 at 05:03:20PM +0000, Colin Ian King wrote:
> > Don't populate the const array dots on the stack
[]
> Examination of the disassembly shows that the compiler actually
> eliminates the creation of the pointer "dots" on the stack and just
> passes the address of the string constant to the printk function.
>
> So this change should not have any actual effect (I don't know where
> you got the "shrinks object code" from), and in my humble opinion
> makes the code less clear.
Probably shrinks an allmodconfig where the symbols are referenced.
It probably doesn't do anything to a defconfig.
> As such, unless there's something here I don't understand, I vote to
> reject this patch.
[]
> > but make it static
> > const and make the pointer an array to remove a dereference. Shrinks
> > object code a few bytes too.
[]
> > diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
[]
> > @@ -725,7 +725,7 @@ static void uv_nmi_dump_cpu_ip(int cpu, struct pt_regs *regs)
> > */
> > static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
> > {
> > - const char *dots = " ................................. ";
> > + static const char dots[] = " ................................. ";
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/platform/uv: make const pointer dots a static const array
2021-12-01 0:26 ` Joe Perches
@ 2021-12-01 21:39 ` Steve Wahl
2021-12-02 9:10 ` Hans de Goede
0 siblings, 1 reply; 6+ messages in thread
From: Steve Wahl @ 2021-12-01 21:39 UTC (permalink / raw)
To: Joe Perches
Cc: Steve Wahl, Colin Ian King, Mike Travis, Dimitri Sivanich,
Russ Anderson, Darren Hart, Andy Shevchenko, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin,
platform-driver-x86, kernel-janitors, linux-kernel
On Tue, Nov 30, 2021 at 04:26:39PM -0800, Joe Perches wrote:
> On Tue, 2021-11-30 at 13:34 -0600, Steve Wahl wrote:
> > On Sat, Nov 27, 2021 at 05:03:20PM +0000, Colin Ian King wrote:
> > > Don't populate the const array dots on the stack
> []
> > Examination of the disassembly shows that the compiler actually
> > eliminates the creation of the pointer "dots" on the stack and just
> > passes the address of the string constant to the printk function.
> >
> > So this change should not have any actual effect (I don't know where
> > you got the "shrinks object code" from), and in my humble opinion
> > makes the code less clear.
>
> Probably shrinks an allmodconfig where the symbols are referenced.
> It probably doesn't do anything to a defconfig.
OK, I looked. Under allmodconfig, the new code is one byte smaller.
Defconfig doesn't include CONFIG_X86_UV and this file doesn't get
compiled.
Using defconfig plus CONFIG_X86_UV and prerequisites, the new code is
24 bytes larger, probably because of alignment added.
allmodconfig:
text data bss dec hex filename
30827 18358 1472 50657 c5e1 uv_nmi.o
30828 18358 1472 50658 c5e2 uv_nmi.orig.o
default config + CONFIG_X86_UV:
text data bss dec hex filename
9918 216 160 10294 2836 uv_nmi.o
9894 216 160 10270 281e uv_nmi.orig.o
So I still don't think this patch makes sense.
--> Steve Wahl
> > As such, unless there's something here I don't understand, I vote to
> > reject this patch.
> []
> > > but make it static
> > > const and make the pointer an array to remove a dereference. Shrinks
> > > object code a few bytes too.
> []
> > > diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
> []
> > > @@ -725,7 +725,7 @@ static void uv_nmi_dump_cpu_ip(int cpu, struct pt_regs *regs)
> > > */
> > > static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
> > > {
> > > - const char *dots = " ................................. ";
> > > + static const char dots[] = " ................................. ";
>
>
--
Steve Wahl, Hewlett Packard Enterprise
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/platform/uv: make const pointer dots a static const array
2021-12-01 21:39 ` Steve Wahl
@ 2021-12-02 9:10 ` Hans de Goede
2021-12-02 9:21 ` Colin King (gmail)
0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2021-12-02 9:10 UTC (permalink / raw)
To: Steve Wahl, Joe Perches
Cc: Colin Ian King, Mike Travis, Dimitri Sivanich, Russ Anderson,
Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H . Peter Anvin,
platform-driver-x86, kernel-janitors, linux-kernel
Hi,
On 12/1/21 22:39, Steve Wahl wrote:
> On Tue, Nov 30, 2021 at 04:26:39PM -0800, Joe Perches wrote:
>> On Tue, 2021-11-30 at 13:34 -0600, Steve Wahl wrote:
>>> On Sat, Nov 27, 2021 at 05:03:20PM +0000, Colin Ian King wrote:
>>>> Don't populate the const array dots on the stack
>> []
>>> Examination of the disassembly shows that the compiler actually
>>> eliminates the creation of the pointer "dots" on the stack and just
>>> passes the address of the string constant to the printk function.
>>>
>>> So this change should not have any actual effect (I don't know where
>>> you got the "shrinks object code" from), and in my humble opinion
>>> makes the code less clear.
>>
>> Probably shrinks an allmodconfig where the symbols are referenced.
>> It probably doesn't do anything to a defconfig.
>
> OK, I looked. Under allmodconfig, the new code is one byte smaller.
>
> Defconfig doesn't include CONFIG_X86_UV and this file doesn't get
> compiled.
>
> Using defconfig plus CONFIG_X86_UV and prerequisites, the new code is
> 24 bytes larger, probably because of alignment added.
>
> allmodconfig:
>
> text data bss dec hex filename
> 30827 18358 1472 50657 c5e1 uv_nmi.o
> 30828 18358 1472 50658 c5e2 uv_nmi.orig.o
>
> default config + CONFIG_X86_UV:
>
> text data bss dec hex filename
> 9918 216 160 10294 2836 uv_nmi.o
> 9894 216 160 10270 281e uv_nmi.orig.o
>
> So I still don't think this patch makes sense.
I agree, so I've dropped this patch from the queue.
Regards,
Hans
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/platform/uv: make const pointer dots a static const array
2021-12-02 9:10 ` Hans de Goede
@ 2021-12-02 9:21 ` Colin King (gmail)
0 siblings, 0 replies; 6+ messages in thread
From: Colin King (gmail) @ 2021-12-02 9:21 UTC (permalink / raw)
To: Hans de Goede, Steve Wahl, Joe Perches
Cc: Colin Ian King, Mike Travis, Dimitri Sivanich, Russ Anderson,
Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H . Peter Anvin,
platform-driver-x86, kernel-janitors, linux-kernel
On 02/12/2021 09:10, Hans de Goede wrote:
> Hi,
>
> On 12/1/21 22:39, Steve Wahl wrote:
>> On Tue, Nov 30, 2021 at 04:26:39PM -0800, Joe Perches wrote:
>>> On Tue, 2021-11-30 at 13:34 -0600, Steve Wahl wrote:
>>>> On Sat, Nov 27, 2021 at 05:03:20PM +0000, Colin Ian King wrote:
>>>>> Don't populate the const array dots on the stack
>>> []
>>>> Examination of the disassembly shows that the compiler actually
>>>> eliminates the creation of the pointer "dots" on the stack and just
>>>> passes the address of the string constant to the printk function.
>>>>
>>>> So this change should not have any actual effect (I don't know where
>>>> you got the "shrinks object code" from), and in my humble opinion
>>>> makes the code less clear.
>>>
>>> Probably shrinks an allmodconfig where the symbols are referenced.
>>> It probably doesn't do anything to a defconfig.
>>
>> OK, I looked. Under allmodconfig, the new code is one byte smaller.
>>
>> Defconfig doesn't include CONFIG_X86_UV and this file doesn't get
>> compiled.
>>
>> Using defconfig plus CONFIG_X86_UV and prerequisites, the new code is
>> 24 bytes larger, probably because of alignment added.
>>
>> allmodconfig:
>>
>> text data bss dec hex filename
>> 30827 18358 1472 50657 c5e1 uv_nmi.o
>> 30828 18358 1472 50658 c5e2 uv_nmi.orig.o
>>
>> default config + CONFIG_X86_UV:
>>
>> text data bss dec hex filename
>> 9918 216 160 10294 2836 uv_nmi.o
>> 9894 216 160 10270 281e uv_nmi.orig.o
>>
>> So I still don't think this patch makes sense.
>
> I agree, so I've dropped this patch from the queue.
>
> Regards,
>
> Hans
>
+1. Apologies for wasting your valuable time. I appreciate the
detailed review.
Colin
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-02 9:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-27 17:03 [PATCH] x86/platform/uv: make const pointer dots a static const array Colin Ian King
2021-11-30 19:34 ` Steve Wahl
2021-12-01 0:26 ` Joe Perches
2021-12-01 21:39 ` Steve Wahl
2021-12-02 9:10 ` Hans de Goede
2021-12-02 9:21 ` Colin King (gmail)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).