On Fri, Mar 25, 2016 at 11:35:17AM -0700, Tony Luck wrote:
> Couple of issues here:
> 1) MCE_LOG_LEN is only 32 - so we may have more pending records than will
> fit in the buffer on high core count cpus
> 2) During a panic we may have a lot of duplicate records because multiple
> logical cpus may have seen and logged the same error because some
> banks are shared.
>
> Switch to using the genpool to look for the pending records. Squeeze
> out duplicated records.
>
> Signed-off-by: Tony Luck <[email protected]>
> ---
>
> New solution for an old problem. See
>
> https://lkml.kernel.org/r/[email protected]
> for the previous take where Ashok said "bump MCE_LOG_LEN from 32 to 128" and
> Boris replied "switch to the gen_pool".
>
> I think this clears the way to pull all of the /dev/mcelog driver
> code out of mce.c and make it just another driver that registers
> a notifier on x86_mce_decoder_chain.
Good, finally!
Once we have all the code around RAS daemon and things, we could finally
mark it deprecated and make it a default off config option for distros
which still need mcelog.
> arch/x86/kernel/cpu/mcheck/mce-genpool.c | 42
> +++++++++++++++++++++++++++++++
> arch/x86/kernel/cpu/mcheck/mce-internal.h | 10 ++++++++
> arch/x86/kernel/cpu/mcheck/mce.c | 21 +++++++---------
> 3 files changed, 61 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce-genpool.c
> b/arch/x86/kernel/cpu/mcheck/mce-genpool.c
> index 0a850100c594..2dc276260fea 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce-genpool.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce-genpool.c
> @@ -26,6 +26,48 @@ static struct gen_pool *mce_evt_pool;
> static LLIST_HEAD(mce_event_llist);
> static char gen_pool_buf[MCE_POOLSZ];
>
> +static bool duplicate_mce_record(struct mce_evt_llist *t, struct
> mce_evt_llist *l)
Hmm, having the verb at the beginning of the name makes me think this
function duplicates MCE records :)
Maybe
is_mce_record_duplicate()
or
mce_record_seen()
...
Bah, all ideas I have right now are meh.
> +{
> + struct mce_evt_llist *node;
> + struct mce *m1, *m2;
> +
> + m1 = &t->mce;
> +
> + llist_for_each_entry(node, &l->llnode, llnode) {
> + m2 = &node->mce;
> +
> + if (dup_mce_record(m1, m2))
> + return true;
> + }
> + return false;
> +}
> +
> +/*
> + * The system has paniced - we'd like to peruse the list of mce records
panicked
> + * that have been queued, but not seen by anyone yet. The list is in
> + * reverse time order, so we need to reverse it. While doing that we can
> + * also drop duplicate records (these were logged because some banks are
> + * shared between cores or by all threads on a socket).
> + */
> +struct llist_node *mce_gen_pool_panic(void)
I'd call it mce_gen_pool_prepare_records() or so.
> +{
> + struct llist_node *head;
> + LLIST_HEAD(new_head);
> + struct mce_evt_llist *node, *t;
> +
> + head = llist_del_all(&mce_event_llist);
> + if (!head)
> + return NULL;
> +
> + /* squeeze out duplicates while reversing order */
> + llist_for_each_entry_safe(node, t, head, llnode) {
> + if (!duplicate_mce_record(node, t))
> + llist_add(&node->llnode, &new_head);
> + }
> +
> + return new_head.first;
> +}
> +
> void mce_gen_pool_process(void)
> {
> struct llist_node *head;
> diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h
> b/arch/x86/kernel/cpu/mcheck/mce-internal.h
> index 547720efd923..492e38e4d02e 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
> +++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
> @@ -35,6 +35,7 @@ void mce_gen_pool_process(void);
> bool mce_gen_pool_empty(void);
> int mce_gen_pool_add(struct mce *mce);
> int mce_gen_pool_init(void);
> +struct llist_node *mce_gen_pool_panic(void);
>
> extern int (*mce_severity)(struct mce *a, int tolerant, char **msg, bool
> is_excp);
> struct dentry *mce_get_debugfs_dir(void);
> @@ -81,3 +82,12 @@ static inline int apei_clear_mce(u64 record_id)
> #endif
>
> void mce_inject_log(struct mce *m);
> +
> +/*
> + * We consider records to be duplicate if bank+status+addr+misc all match
> + */
> +static inline bool dup_mce_record(struct mce *m1, struct mce *m2)
Since this is a "comparison" of sorts, I'd prefer something like:
mce_records_equal(m1, m2);
or even better:
mce_cmp(m1, m2);
:-)
> +{
> + return m1->bank == m2->bank && m1->status == m2->status &&
> + m1->addr == m2->addr && m1->misc == m2->misc;
Should be more readable like this:
return m1->bank == m2->bank &&
m1->status == m2->status &&
m1->addr == m2->addr &&
m1->misc == m2->misc;
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
