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

[Yazen Ghannam]

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