Re: [PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu
On Mon, Apr 08, 2019 at 10:48:34PM +, Ghannam, Yazen wrote: > Okay, so drop the export and leave the injector code as-is (it's > already doing a rdmsrl_on_cpu()). Yes of course. The number of MCA banks is none of modules' business and should not be exported at all. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
RE: [PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu
> -Original Message- > From: linux-edac-ow...@vger.kernel.org On > Behalf Of Luck, Tony > Sent: Monday, April 8, 2019 6:23 PM > To: Ghannam, Yazen > Cc: Borislav Petkov ; linux-e...@vger.kernel.org; > linux-kernel@vger.kernel.org; x...@kernel.org > Subject: Re: [PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu > > On Mon, Apr 08, 2019 at 10:48:34PM +, Ghannam, Yazen wrote: > > Okay, so drop the export and leave the injector code as-is (it's > > already doing a rdmsrl_on_cpu()). > > It's still a globally visible symbol (shared by core.c and amd.c). > So I think it needs a "mce_" prefix. > > While it doesn't collide now, there are a bunch of other > subsystems that have "banks" and a variable to count them. > > Look at output from "git grep -w num_banks". > Okay, I'll add the prefix. And thanks for the tip. I'll try to keep this in mind. Thanks, Yazen
Re: [PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu
On Mon, Apr 08, 2019 at 10:48:34PM +, Ghannam, Yazen wrote: > Okay, so drop the export and leave the injector code as-is (it's > already doing a rdmsrl_on_cpu()). It's still a globally visible symbol (shared by core.c and amd.c). So I think it needs a "mce_" prefix. While it doesn't collide now, there are a bunch of other subsystems that have "banks" and a variable to count them. Look at output from "git grep -w num_banks". -Tony
RE: [PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu
> -Original Message- > From: linux-edac-ow...@vger.kernel.org On > Behalf Of Borislav Petkov > Sent: Monday, April 8, 2019 3:50 PM > To: Luck, Tony > Cc: Ghannam, Yazen ; linux-e...@vger.kernel.org; > linux-kernel@vger.kernel.org; x...@kernel.org > Subject: Re: [PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu > > On Mon, Apr 08, 2019 at 08:42:36PM +, Luck, Tony wrote: > > > Actually, it should not be exported at all. A function returning the num > > > banks is better instead. > > > > Are all the places it is used in non-pre-emptible sections of code? Looping > > in the CMCI and #MC handlers should be fine. But do we need > > get_cpu()/put_cpu() > > in any places? > > That export is needed only in the mce injector. Actually, it would be > much cleaner if the injector would find out the count straight from the > MSR as it does now, but be changed to do rdmsr_on_cpu() now, since can > have different num_banks on a CPU. > Okay, so drop the export and leave the injector code as-is (it's already doing a rdmsrl_on_cpu()). Is that okay? Thanks, Yazen
Re: [PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu
On Mon, Apr 08, 2019 at 08:42:36PM +, Luck, Tony wrote: > > Actually, it should not be exported at all. A function returning the num > > banks is better instead. > > Are all the places it is used in non-pre-emptible sections of code? Looping > in the CMCI and #MC handlers should be fine. But do we need > get_cpu()/put_cpu() > in any places? That export is needed only in the mce injector. Actually, it would be much cleaner if the injector would find out the count straight from the MSR as it does now, but be changed to do rdmsr_on_cpu() now, since can have different num_banks on a CPU. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
RE: [PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu
> Actually, it should not be exported at all. A function returning the num > banks is better instead. Are all the places it is used in non-pre-emptible sections of code? Looping in the CMCI and #MC handlers should be fine. But do we need get_cpu()/put_cpu() in any places? -Tony
Re: [PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu
On Mon, Apr 08, 2019 at 01:26:59PM -0700, Luck, Tony wrote: > On Mon, Apr 08, 2019 at 02:12:17PM +, Ghannam, Yazen wrote: > > +DEFINE_PER_CPU_READ_MOSTLY(u8, num_banks); > > +EXPORT_PER_CPU_SYMBOL_GPL(num_banks); > > The name "num_banks" is a bit generic for an exported symbol. > I think it should have a "mce_" prefix. Actually, it should not be exported at all. A function returning the num banks is better instead. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu
On Mon, Apr 08, 2019 at 02:12:17PM +, Ghannam, Yazen wrote: > +DEFINE_PER_CPU_READ_MOSTLY(u8, num_banks); > +EXPORT_PER_CPU_SYMBOL_GPL(num_banks); The name "num_banks" is a bit generic for an exported symbol. I think it should have a "mce_" prefix. -Tony
[PATCH RESEND 4/5] x86/MCE: Make number of MCA banks per_cpu
From: Yazen Ghannam
The number of MCA banks is provided per logical CPU. Historically, this
number has been the same across all CPUs, but this is not an
architectural guarantee. Future AMD systems may have MCA bank counts
that vary between logical CPUs in a system.
This issue was partially addressed in
commit ("006c077041dc x86/mce: Handle varying MCA bank counts")
by allocating structures using the maximum number of MCA banks and by
saving the maximum MCA bank count in a system as the global count. This
means that some extra structures are allocated. Also, this means that
CPUs will spend more time in the #MC and other handlers checking extra
MCA banks.
Define the number of MCA banks as a per_cpu variable. Replace all uses
of mca_cfg.banks with this.
Also, use the per_cpu bank count when allocating per_cpu structures.
Print the number of banks per CPU as a debug message for those who may
be interested.
Signed-off-by: Yazen Ghannam
---
arch/x86/kernel/cpu/mce/amd.c | 16 +-
arch/x86/kernel/cpu/mce/core.c | 49 +-
arch/x86/kernel/cpu/mce/inject.c | 7 +
arch/x86/kernel/cpu/mce/internal.h | 2 +-
4 files changed, 38 insertions(+), 36 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index f0644b59848d..acce672efb45 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -493,7 +493,7 @@ static u32 get_block_address(u32 current_addr, u32 low, u32
high,
{
u32 addr = 0, offset = 0;
- if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
+ if ((bank >= per_cpu(num_banks, cpu)) || (block >= NR_BLOCKS))
return addr;
if (mce_flags.smca)
@@ -605,7 +605,7 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
disable_err_thresholding(c);
- for (bank = 0; bank < mca_cfg.banks; ++bank) {
+ for (bank = 0; bank < this_cpu_read(num_banks); ++bank) {
if (mce_flags.smca)
smca_configure(bank, cpu);
@@ -948,7 +948,7 @@ static void amd_deferred_error_interrupt(void)
{
unsigned int bank;
- for (bank = 0; bank < mca_cfg.banks; ++bank)
+ for (bank = 0; bank < this_cpu_read(num_banks); ++bank)
log_error_deferred(bank);
}
@@ -989,7 +989,7 @@ static void amd_threshold_interrupt(void)
struct threshold_block *first_block = NULL, *block = NULL, *tmp = NULL;
unsigned int bank, cpu = smp_processor_id();
- for (bank = 0; bank < mca_cfg.banks; ++bank) {
+ for (bank = 0; bank < this_cpu_read(num_banks); ++bank) {
if (!(per_cpu(bank_map, cpu) & (1 << bank)))
continue;
@@ -1176,7 +1176,7 @@ static int allocate_threshold_blocks(unsigned int cpu,
unsigned int bank,
u32 low, high;
int err;
- if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
+ if ((bank >= per_cpu(num_banks, cpu)) || (block >= NR_BLOCKS))
return 0;
if (rdmsr_safe_on_cpu(cpu, address, , ))
@@ -1410,7 +1410,7 @@ int mce_threshold_remove_device(unsigned int cpu)
{
unsigned int bank;
- for (bank = 0; bank < mca_cfg.banks; ++bank) {
+ for (bank = 0; bank < per_cpu(num_banks, cpu); ++bank) {
if (!(per_cpu(bank_map, cpu) & (1 << bank)))
continue;
threshold_remove_bank(cpu, bank);
@@ -1431,14 +1431,14 @@ int mce_threshold_create_device(unsigned int cpu)
if (bp)
return 0;
- bp = kcalloc(mca_cfg.banks, sizeof(struct threshold_bank *),
+ bp = kcalloc(per_cpu(num_banks, cpu), sizeof(struct threshold_bank *),
GFP_KERNEL);
if (!bp)
return -ENOMEM;
per_cpu(threshold_banks, cpu) = bp;
- for (bank = 0; bank < mca_cfg.banks; ++bank) {
+ for (bank = 0; bank < per_cpu(num_banks, cpu); ++bank) {
if (!(per_cpu(bank_map, cpu) & (1 << bank)))
continue;
err = threshold_create_bank(cpu, bank);
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 14583c5c6e12..4a066c1e8ab2 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -64,6 +64,9 @@ static DEFINE_MUTEX(mce_sysfs_mutex);
DEFINE_PER_CPU(unsigned, mce_exception_count);
+DEFINE_PER_CPU_READ_MOSTLY(u8, num_banks);
+EXPORT_PER_CPU_SYMBOL_GPL(num_banks);
+
struct mce_bank {
u64 ctl;/* subevents to enable */
boolinit; /* initialise bank? */
@@ -699,7 +702,7 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t
*b)
if (flags & MCP_TIMESTAMP)
m.tsc = rdtsc();
- for (i = 0; i < mca_cfg.banks; i++) {
+ for (i = 0; i < this_cpu_read(num_banks); i++) {
if (!this_cpu_read(mce_banks)[i].ctl || !test_bit(i, *b))
continue;
@@ -801,7 +804,7
