[v1] EDAC, pnd2: Make read-only const array intlv static

[PATCH][next] EDAC, pnd2: Make read-only const array intlv static

Posted by Colin Ian King 1 year, 4 months ago

Don't populate the const read-only array intlv on the stack at
run time, instead make it static.

Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
---
 drivers/edac/pnd2_edac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c
index f93f2f2b1cf2..af14c8a3279f 100644
--- a/drivers/edac/pnd2_edac.c
+++ b/drivers/edac/pnd2_edac.c
@@ -372,7 +372,7 @@ static int gen_asym_mask(struct b_cr_slice_channel_hash *p,
 			 struct b_cr_asym_mem_region1_mchbar *as1,
 			 struct b_cr_asym_2way_mem_region_mchbar *as2way)
 {
-	const int intlv[] = { 0x5, 0xA, 0x3, 0xC };
+	static const int intlv[] = { 0x5, 0xA, 0x3, 0xC };
 	int mask = 0;
 
 	if (as2way->asym_2way_interleave_enable)
@@ -489,7 +489,7 @@ static int dnv_get_registers(void)
  */
 static int get_registers(void)
 {
-	const int intlv[] = { 10, 11, 12, 12 };
+	static const int intlv[] = { 10, 11, 12, 12 };
 
 	if (RD_REG(&tolud, b_cr_tolud_pci) ||
 		RD_REG(&touud_lo, b_cr_touud_lo_pci) ||
-- 
2.39.2

RE: [PATCH][next] EDAC, pnd2: Make read-only const array intlv static

Posted by Zhuo, Qiuxu 1 year, 4 months ago

> From: Colin Ian King <colin.i.king@gmail.com>
> [...]
> Subject: [PATCH][next] EDAC, pnd2: Make read-only const array intlv static
> 
> Don't populate the const read-only array intlv on the stack at run time, instead
> make it static.
> 
> Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
> ---
>  drivers/edac/pnd2_edac.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c index
> f93f2f2b1cf2..af14c8a3279f 100644
> --- a/drivers/edac/pnd2_edac.c
> +++ b/drivers/edac/pnd2_edac.c
> @@ -372,7 +372,7 @@ static int gen_asym_mask(struct
> b_cr_slice_channel_hash *p,
>  			 struct b_cr_asym_mem_region1_mchbar *as1,
>  			 struct b_cr_asym_2way_mem_region_mchbar
> *as2way)  {
> -	const int intlv[] = { 0x5, 0xA, 0x3, 0xC };
> +	static const int intlv[] = { 0x5, 0xA, 0x3, 0xC };
>  	int mask = 0;
> 
>  	if (as2way->asym_2way_interleave_enable)
> @@ -489,7 +489,7 @@ static int dnv_get_registers(void)
>   */
>  static int get_registers(void)
>  {
> -	const int intlv[] = { 10, 11, 12, 12 };
> +	static const int intlv[] = { 10, 11, 12, 12 };
> 

I didn't see the why and the benefits of these changes.
Could you elaborate more?

Re: [PATCH][next] EDAC, pnd2: Make read-only const array intlv static

Posted by Colin King (gmail) 1 year, 4 months ago

On 19/09/2024 08:19, Zhuo, Qiuxu wrote:
>> From: Colin Ian King <colin.i.king@gmail.com>
>> [...]
>> Subject: [PATCH][next] EDAC, pnd2: Make read-only const array intlv static
>>
>> Don't populate the const read-only array intlv on the stack at run time, instead
>> make it static.
>>
>> Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
>> ---
>>   drivers/edac/pnd2_edac.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c index
>> f93f2f2b1cf2..af14c8a3279f 100644
>> --- a/drivers/edac/pnd2_edac.c
>> +++ b/drivers/edac/pnd2_edac.c
>> @@ -372,7 +372,7 @@ static int gen_asym_mask(struct
>> b_cr_slice_channel_hash *p,
>>   			 struct b_cr_asym_mem_region1_mchbar *as1,
>>   			 struct b_cr_asym_2way_mem_region_mchbar
>> *as2way)  {
>> -	const int intlv[] = { 0x5, 0xA, 0x3, 0xC };
>> +	static const int intlv[] = { 0x5, 0xA, 0x3, 0xC };
>>   	int mask = 0;
>>
>>   	if (as2way->asym_2way_interleave_enable)
>> @@ -489,7 +489,7 @@ static int dnv_get_registers(void)
>>    */
>>   static int get_registers(void)
>>   {
>> -	const int intlv[] = { 10, 11, 12, 12 };
>> +	static const int intlv[] = { 10, 11, 12, 12 };
>>
> 
> I didn't see the why and the benefits of these changes.
> Could you elaborate more?

The non-const construct will generate code to put the array on the stack 
and this occurs on each call, so there is a small amount of extra object 
code overhead to do this at run time. Making it static will put the data
into a data section so there is run-time penalty. So this change 
potentially shrinks the object code and run time overhead a very small 
amount.

Colin

RE: [PATCH][next] EDAC, pnd2: Make read-only const array intlv static

Posted by Zhuo, Qiuxu 1 year, 4 months ago

> From: Colin King (gmail) <colin.i.king@gmail.com>
> [...]

> >> -	const int intlv[] = { 10, 11, 12, 12 };
> >> +	static const int intlv[] = { 10, 11, 12, 12 };
> >>
> >
> > I didn't see the why and the benefits of these changes.
> > Could you elaborate more?
> 
> The non-const construct will generate code to put the array on the stack and

Typo? 
s/non-const/non-static/

> this occurs on each call, so there is a small amount of extra object code
> overhead to do this at run time. Making it static will put the data into a data
> section so there is run-time penalty. So this change potentially shrinks the
> object code and run time overhead a very small amount.
> 

Thanks! 

As these are not in hot paths, there should be no measurable performance difference. 

But, I do see that the text size is reduced by 12 bytes [1] after these changes.
Could you add a short description of this benefit in the commit message? 
Other than that,

    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

[1] $ size pnd2_edac.o.*

   text    data     bss     dec     hex filename
  15632     264    1384   17280    4380 pnd2_edac.o.new
  15644     264    1384   17292    438c pnd2_edac.o.old

- Qiuxu