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

Yazen Ghannam
2024-02-29T15:49:31Z

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