On Fri, Jun 27, 2025 at 02:14:40PM +0300, Nikolay Borisov wrote: > > > On 6/25/25 19:52, Nikolay Borisov wrote: > > > > > > On 6/24/25 17:16, Yazen Ghannam wrote: > > > The threshold_bank structure is a container for one or more > > > threshold_block structures. Currently, the container has a single > > > pointer to the 'first' threshold_block structure which then has a linked > > > list of the remaining threshold_block structures. > > > > > > This results in an extra level of indirection where the 'first' block is > > > checked before iterating over the remaining blocks. > > > > > > Remove the indirection by including the head of the block list in the > > > threshold_bank structure which already acts as a container for all the > > > bank's thresholding blocks. > > > > > > Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@xxxxxxxxx> > > > Tested-by: Tony Luck <tony.luck@xxxxxxxxx> > > > Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx> > > > Signed-off-by: Yazen Ghannam <yazen.ghannam@xxxxxxx> > > > --- > > > > > > Notes: > > > Link: > > > https://lore.kernel.org/r/20250415-wip-mca-updates- > > > v3-4-8ffd9eb4aa56@xxxxxxx > > > v3->v4: > > > * No change. > > > v2->v3: > > > * Added tags from Qiuxu and Tony. > > > v1->v2: > > > * New in v2. > > > > > > arch/x86/kernel/cpu/mce/amd.c | 43 +++++++++++ > > > +------------------------------- > > > 1 file changed, 12 insertions(+), 31 deletions(-) > > > > > > diff --git a/arch/x86/kernel/cpu/mce/amd.c > > > b/arch/x86/kernel/cpu/mce/ amd.c > > > index 0ffbee329a8c..5d351ec863cd 100644 > > > --- a/arch/x86/kernel/cpu/mce/amd.c > > > +++ b/arch/x86/kernel/cpu/mce/amd.c > > > @@ -241,7 +241,8 @@ struct threshold_block { > > > struct threshold_bank { > > > struct kobject *kobj; > > > - struct threshold_block *blocks; > > > + /* List of threshold blocks within this MCA bank. */ > > > + struct list_head miscj; > > > }; > > > static DEFINE_PER_CPU(struct threshold_bank **, threshold_banks); > > > @@ -900,9 +901,9 @@ static void log_and_reset_block(struct > > > threshold_block *block) > > > */ > > > static void amd_threshold_interrupt(void) > > > { > > > - struct threshold_block *first_block = NULL, *block = NULL, *tmp > > > = NULL; > > > - struct threshold_bank **bp = this_cpu_read(threshold_banks); > > > + struct threshold_bank **bp = this_cpu_read(threshold_banks), > > > *thr_bank; > > > unsigned int bank, cpu = smp_processor_id(); > > > + struct threshold_block *block, *tmp; > > > /* > > > * Validate that the threshold bank has been initialized > > > already. The > > > @@ -916,16 +917,11 @@ static void amd_threshold_interrupt(void) > > > if (!(per_cpu(bank_map, cpu) & BIT_ULL(bank))) > > > continue; > > > > <slight off topic> > > > > nit: I wonder if instead of using per_cpu and manual bit testing can't a > > direct > > call to x86_this_cpu_test_bit be a better solution. The assembly looks > > like: > > > > [OLD] > > > > xorl %r14d, %r14d # ivtmp.245 > > movq %rax, 8(%rsp) # cpu, %sfp > > # arch/x86/kernel/cpu/mce/amd.c:917: if (!(per_cpu(bank_map, cpu) > > & BIT_ULL(bank))) > > movq $bank_map, %rax #, __ptr > > movq %rax, (%rsp) # __ptr, %sfp > > .L236: > > movq 8(%rsp), %rax # %sfp, cpu > > # arch/x86/kernel/cpu/mce/amd.c:917: if (!(per_cpu(bank_map, cpu) > > & BIT_ULL(bank))) > > movq (%rsp), %rsi # %sfp, __ptr > > # arch/x86/kernel/cpu/mce/amd.c:917: if (!(per_cpu(bank_map, cpu) > > & BIT_ULL(bank))) > > movq __per_cpu_offset(,%rax,8), %rax # __per_cpu_offset[cpu_23], > > __per_cpu_offset[cpu_23] > > # arch/x86/kernel/cpu/mce/amd.c:917: if (!(per_cpu(bank_map, cpu) > > & BIT_ULL(bank))) > > movq (%rax,%rsi), %rax > > # arch/x86/kernel/cpu/mce/amd.c:917: if (!(per_cpu(bank_map, cpu) > > & BIT_ULL(bank))) > > btq %r14, %rax > > > > [NEW] > > > > xorl %r15d, %r15d # ivtmp.246 > > .L236: > > # 917 "arch/x86/kernel/cpu/mce/amd.c" 1 > > btl %r15d, %gs:bank_map(%rip) # ivtmp.246, *_9 > > > > > > That way you end up with a single btl (but I guess a version that uses > > btq should be added as well) > > inside the loop rather than a bunch of instructions moving data around > > for per_cpu. > > > > Alternatively, since this is running in interrupt context can't you use > > directly this_cpu_read(bank_map) and eliminate the smp_processor_id > > invocation? > > > Actually the total number of banks are at most 128 as per the layout of > MCG_CAP register, so using btl is fine. Also I'm not sure why the original > code uses BIT_ULL vs just BIT since we can't have a 64bit value. > > Hi Nikolay, MCG_CAP[Count] is an 8-bit field, so we can (potentially) have up to 255 MCA banks. "bank_map" is a bitmask, and current systems can have up to 64 MCA banks. That is why BIT_ULL is needed. Thanks, Yazen
