lkml.org 
[lkml]   [2015]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2015-07-21 10:41    [W:0.211 / U:1.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site