| Message ID | 20241016123036.21366-9-qiuxu.zhuo@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/mce: Clean up some x86/mce code | expand |
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
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));
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(-)