From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756969AbcDGRDb (ORCPT ); Thu, 7 Apr 2016 13:03:31 -0400 Received: from mail.skyhub.de ([78.46.96.112]:47860 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751706AbcDGRDa (ORCPT ); Thu, 7 Apr 2016 13:03:30 -0400 Date: Thu, 7 Apr 2016 19:03:26 +0200 From: Borislav Petkov To: Tony Luck Cc: linux-kernel@vger.kernel.org, Ashok Raj Subject: Re: [PATCHv2] x86/mce: Look in genpool instead of mcelog.entry[] for pending error records Message-ID: <20160407170326.GK3735@pd.tnic> References: <20160331084620.GB25191@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 07, 2016 at 09:34:06AM -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 > --- > v2: Better names and code layout (Boris) > Revised commments on mce record comparisons (Ashok) > > arch/x86/kernel/cpu/mcheck/mce-genpool.c | 46 +++++++++++++++++++++++++++++++ > arch/x86/kernel/cpu/mcheck/mce-internal.h | 15 ++++++++++ > arch/x86/kernel/cpu/mcheck/mce.c | 21 ++++++-------- > 3 files changed, 70 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..c43050b91d6d 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce-genpool.c > +++ b/arch/x86/kernel/cpu/mcheck/mce-genpool.c > @@ -26,6 +26,52 @@ 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 > + * a functionally equivalent one is present in the list. functionally? > + */ > +static bool is_duplicate_mce_record(struct mce_evt_llist *t, struct mce_evt_llist *l) > +{ > + struct mce_evt_llist *node; > + struct mce *m1, *m2; > + > + m1 = &t->mce; > + > + llist_for_each_entry(node, &l->llnode, llnode) { > + m2 = &node->mce; > + > + if (mce_cmp(m1, m2)) Sorry for nitpicking but isn't it usually the case that a _cmp()-something function should return 0 when both things are equal? I.e., you have: if (!strcmp(s1, s2)) ... I think if we do it this way here too, it'll be very natural. mce_cmp() would then have to do: return !(m1->bank == m2->bank && m1->status == m2->status && m1->addr == m2->addr && m1->misc == m2->misc); simply. Hmmm? Rest looks ok. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply.