Messages in this thread | | | Date | Tue, 21 Jul 2015 10:29:49 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 1/7] x86/mce: Provide a lockless memory pool to save error records |
| |
* Borislav Petkov <bp@suse.de> wrote:
> From: "Chen, Gong" <gong.chen@linux.intel.com>
> diff --git a/arch/x86/include/uapi/asm/mce.h b/arch/x86/include/uapi/asm/mce.h > index a0eab85ce7b8..76880ede9a35 100644 > --- a/arch/x86/include/uapi/asm/mce.h > +++ b/arch/x86/include/uapi/asm/mce.h > @@ -15,7 +15,8 @@ struct mce { > __u64 time; /* wall time_t when error was detected */ > __u8 cpuvendor; /* cpu vendor as encoded in system.h */ > __u8 inject_flags; /* software inject flags */ > - __u16 pad; > + __u8 severity; > + __u8 usable_addr; > __u32 cpuid; /* CPUID 1 EAX */ > __u8 cs; /* code segment */ > __u8 bank; /* machine check bank */
So this change appears to be completely unrelated to the stated purpose of this patch?
> +/* > + * printk() is not safe in MCE context. This is a lock-less memory allocator > + * 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
Missing comma.
> + * 2 pages to save MCE events for now (~80 MCE records at most). > + */ > +#define MCE_POOLSZ (2 * PAGE_SIZE)
> +bool mce_genpool_add(struct mce *mce) > +{ > + struct mce_evt_llist *node; > + > + if (!mce_evt_pool) > + return false; > + > + node = (void *)gen_pool_alloc(mce_evt_pool, sizeof(*node)); > + if (!node) { > + pr_warn_ratelimited("MCE records pool full!\n"); > + return false; > + } > + > + memcpy(&node->mce, mce, sizeof(*mce)); > + llist_add(&node->llnode, &mce_event_llist); > + > + return true; > +}
So I think the standard pattern for allocation failures with integer types is to return -ENOMEM, not bool. This really matters, because:
> + > +static int mce_genpool_create(void) > +{ > + struct gen_pool *tmpp; > + int ret = -ENOMEM; > + > + tmpp = gen_pool_create(ilog2(sizeof(struct mce_evt_llist)), -1); > + if (!tmpp) > + goto out; > + > + ret = gen_pool_add(tmpp, (unsigned long)genpool_buf, MCE_POOLSZ, -1); > + if (ret) { > + gen_pool_destroy(tmpp); > + goto out;
here gen_pool_add() has an inverted logic, and they looks confusing.
Furthermore, why do we spell it 'mce_genpool' if the generic facility is spelling it gen_pool?
Also, I'm questioning the whole premise of the patches:
> +/* > + * printk() is not safe in MCE context. This is a lock-less memory allocator > + * 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
So how are we going to report uncorrectable errors that forcibly crash/panic the system if we cannot use printk? How will the admin learn what was amiss?
Thanks,
Ingo
| |