linux-kernel - Re: [PATCH] x86/mce: Dynamically size space for machine check records

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 29 Feb 2024 10:49:18 -0500
From: Yazen Ghannam <yazen.ghannam@....com>
To: Sohil Mehta <sohil.mehta@...el.com>, Tony Luck <tony.luck@...el.com>,
 Borislav Petkov <bp@...en8.de>
Cc: yazen.ghannam@....com, "Naik, Avadhut" <avadnaik@....com>,
 "x86@...nel.org" <x86@...nel.org>,
 "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Avadhut Naik <avadhut.naik@....com>
Subject: Re: [PATCH] x86/mce: Dynamically size space for machine check records

On 2/28/2024 8:56 PM, Sohil Mehta wrote:
> A few other nits.
> 
> On 2/28/2024 3:14 PM, Tony Luck wrote:
>> diff --git a/arch/x86/kernel/cpu/mce/genpool.c b/arch/x86/kernel/cpu/mce/genpool.c
>> index fbe8b61c3413..a1f0a8f29cf5 100644
>> --- a/arch/x86/kernel/cpu/mce/genpool.c
>> +++ b/arch/x86/kernel/cpu/mce/genpool.c
>> @@ -16,14 +16,13 @@
>>    * used to save error information organized in a lock-less list.
>>    *
>>    * This memory pool is only to be used to save MCE records in MCE context.
>> - * MCE events are rare, so a fixed size memory pool should be enough. Use
>> - * 2 pages to save MCE events for now (~80 MCE records at most).
>> + * MCE events are rare, so a fixed size memory pool should be enough.
>> + * Allocate on a sliding scale based on number of CPUs.
>>    */
>> -#define MCE_POOLSZ	(2 * PAGE_SIZE)
>> +#define MCE_MIN_ENTRIES	80
>>   
>>   static struct gen_pool *mce_evt_pool;
>>   static LLIST_HEAD(mce_event_llist);
>> -static char gen_pool_buf[MCE_POOLSZ];
>>   
>>   /*
>>    * Compare the record "t" with each of the records on list "l" to see if
>> @@ -118,14 +117,25 @@ int mce_gen_pool_add(struct mce *mce)
>>   
>>   static int mce_gen_pool_create(void)
>>   {
>> +	int mce_numrecords, mce_poolsz;
> 
> Should order be also declared in this line? That way we can have all the
> uninitialized 'int's together.
> 
>>   	struct gen_pool *tmpp;
>>   	int ret = -ENOMEM;
>> +	void *mce_pool;
>> +	int order;
>>   
>> -	tmpp = gen_pool_create(ilog2(sizeof(struct mce_evt_llist)), -1);
>> +	order = ilog2(sizeof(struct mce_evt_llist)) + 1;
> 
> I didn't exactly understand why a +1 is needed here. Do you have a
> pointer to somewhere to help understand this?
> 
> Also, I think, a comment on top might be useful since this isn't obvious.
> 

Would order_base_2() work here? It automatically rounds up to the next power.

Thanks,
Yazen