As the entire mce structure is initialized to zero using memset(0)
within mce_gather_info(), remove the redundant zeroing assignments to
mce->misc and mce->addr.

This results in a reduction of 64 bytes in the text size.

  $ size core.o.*
     text	   data	    bss	    dec	    hex	filename
    21348	   4181	   3776	  29305	   7279	core.o.old
    21284	   4181	   3776	  29241	   7239	core.o.new

Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
 arch/x86/kernel/cpu/mce/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index e718b9bbe8e5..844a6f8d6f39 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -706,8 +706,6 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		if (!mce_banks[i].ctl || !test_bit(i, *b))
 			continue;
 
-		m.misc = 0;
-		m.addr = 0;
 		m.bank = i;
 
 		barrier();
@@ -1284,8 +1282,6 @@ __mc_scan_banks(struct mce *m, struct pt_regs *regs, struct mce *final,
 		if (!mce_banks[i].ctl)
 			continue;
 
-		m->misc = 0;
-		m->addr = 0;
 		m->bank = i;
 
 		m->status = mce_rdmsrl(mca_msr_reg(i, MCA_STATUS));
-- 
2.17.1

On 10/16/2024 5:30 AM, Qiuxu Zhuo wrote:
> As the entire mce structure is initialized to zero using memset(0)
> within mce_gather_info(), remove the redundant zeroing assignments to
> mce->misc and mce->addr.
> 

...

> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index e718b9bbe8e5..844a6f8d6f39 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -706,8 +706,6 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
>  		if (!mce_banks[i].ctl || !test_bit(i, *b))
>  			continue;
>  
> -		m.misc = 0;
> -		m.addr = 0;
>  		m.bank = i;
>  

This makes sense since mce_gather_info() happens in the same function.

>  		barrier();
> @@ -1284,8 +1282,6 @@ __mc_scan_banks(struct mce *m, struct pt_regs *regs, struct mce *final,
>  		if (!mce_banks[i].ctl)
>  			continue;
>  
> -		m->misc = 0;
> -		m->addr = 0;
>  		m->bank = i;
>  

However, in this case, I am not fully convinced if the misc and addr
would already be 0 when we reach here.

There are potentially a lot of things that happen in do_machine_check()
between mce_gather_info() and __mc_scan_banks(). Especially,
mce_no_way_out() which could theoretically call mce_read_aux() in some
cases.

Maybe it doesn't matter, misc and addr would be overwritten anyway. But
I feel some more details in the commit message would be useful. It
doesn't seem as simple as the brief description makes it sound (at least
to me).


>  		m->status = mce_rdmsrl(mca_msr_reg(i, MCA_STATUS));

> From: Mehta, Sohil <sohil.mehta@intel.com>
> [...]
> > @@ -1284,8 +1282,6 @@ __mc_scan_banks(struct mce *m, struct pt_regs
> *regs, struct mce *final,
> >  		if (!mce_banks[i].ctl)
> >  			continue;
> >
> > -		m->misc = 0;
> > -		m->addr = 0;
> >  		m->bank = i;
> >
> 
> However, in this case, I am not fully convinced if the misc and addr would
> already be 0 when we reach here.
> 
> There are potentially a lot of things that happen in do_machine_check()
> between mce_gather_info() and __mc_scan_banks(). Especially,
> mce_no_way_out() which could theoretically call mce_read_aux() in some
> cases.
> 
> Maybe it doesn't matter, misc and addr would be overwritten anyway. But I
> feel some more details in the commit message would be useful. It doesn't
> seem as simple as the brief description makes it sound (at least to me).
> 

Your concern is reasonable. Thanks!

For both diffs, mce->misc and mce->addr can be guaranteed to be zeroed the first time
they reach here. However, I didn't notice that both diffs were in a for() loop where 
mce->misc and mce->addr could retain the old values assigned by mce_read_aux() in 
the previous iteration. So need to zero mce-misc and mce->addr in each iteration to 
ensure they don't contain stale values. 

 I'll drop this patch in the next version.

-Qiuxu

On October 19, 2024 12:37:05 AM PDT, "Zhuo, Qiuxu" <qiuxu.zhuo@intel.com> wrote:
>> From: Mehta, Sohil <sohil.mehta@intel.com>
>> [...]
>> > @@ -1284,8 +1282,6 @@ __mc_scan_banks(struct mce *m, struct pt_regs
>> *regs, struct mce *final,
>> >  		if (!mce_banks[i].ctl)
>> >  			continue;
>> >
>> > -		m->misc = 0;
>> > -		m->addr = 0;
>> >  		m->bank = i;
>> >
>> 
>> However, in this case, I am not fully convinced if the misc and addr would
>> already be 0 when we reach here.
>> 
>> There are potentially a lot of things that happen in do_machine_check()
>> between mce_gather_info() and __mc_scan_banks(). Especially,
>> mce_no_way_out() which could theoretically call mce_read_aux() in some
>> cases.
>> 
>> Maybe it doesn't matter, misc and addr would be overwritten anyway. But I
>> feel some more details in the commit message would be useful. It doesn't
>> seem as simple as the brief description makes it sound (at least to me).
>> 
>
>Your concern is reasonable. Thanks!
>
>For both diffs, mce->misc and mce->addr can be guaranteed to be zeroed the first time
>they reach here. However, I didn't notice that both diffs were in a for() loop where 
>mce->misc and mce->addr could retain the old values assigned by mce_read_aux() in 
>the previous iteration. So need to zero mce-misc and mce->addr in each iteration to 
>ensure they don't contain stale values. 
>
> I'll drop this patch in the next version.
>
>-Qiuxu
>

Keep in mind that usually the compiler will remove redundant assignments, and if they are too obscure for the compiler to discover, they are probably too subtle for programmers to not introduce bugs in the future ...

> From: H. Peter Anvin <hpa@zytor.com>
> [...]
> 
> Keep in mind that usually the compiler will remove redundant assignments,
> and if they are too obscure for the compiler to discover, they are probably too
> subtle for programmers to not introduce bugs in the future ...

Thanks, H.Peter.

This is a good tip to quickly check whether a cleanup of removing unnecessary
assignments changes the function. If there is no difference in the text before and
after the cleanup, then it's OK. Otherwise, the cleanup probably changes the 
function in an unintended way.

-Qiuxu

On October 19, 2024 1:30:04 AM PDT, "Zhuo, Qiuxu" <qiuxu.zhuo@intel.com> wrote:
>> From: H. Peter Anvin <hpa@zytor.com>
>> [...]
>> 
>> Keep in mind that usually the compiler will remove redundant assignments,
>> and if they are too obscure for the compiler to discover, they are probably too
>> subtle for programmers to not introduce bugs in the future ...
>
>Thanks, H.Peter.
>
>This is a good tip to quickly check whether a cleanup of removing unnecessary
>assignments changes the function. If there is no difference in the text before and
>after the cleanup, then it's OK. Otherwise, the cleanup probably changes the 
>function in an unintended way.
>
>-Qiuxu
>

Yes and no. Deleting things like redundant reinitialization should only be done if it makes the code clearer. You can think of the redundant statements as comments/asserts.

> From: H. Peter Anvin <hpa@zytor.com>
> [...]
> >This is a good tip to quickly check whether a cleanup of removing
> >unnecessary assignments changes the function. If there is no difference
> >in the text before and after the cleanup, then it's OK. Otherwise, the
> >cleanup probably changes the function in an unintended way.
> >
> >-Qiuxu
> >
> 
> Yes and no. Deleting things like redundant reinitialization should only be done
> if it makes the code clearer. You can think of the redundant statements as
> comments/asserts.

Thanks for the further clarification.

-Qiuxu