LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Don Zickus <dzickus@redhat.com>
To: Jiri Kosina <jkosina@suse.cz>
Cc: Borislav Petkov <bp@alien8.de>,
	linux-edac <linux-edac@vger.kernel.org>,
	Borislav Petkov <bp@suse.de>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Len Brown <lenb@kernel.org>, Tony Luck <tony.luck@intel.com>,
	Tomasz Nowicki <tomasz.nowicki@linaro.org>,
	"Chen, Gong" <gong.chen@linux.intel.com>,
	Wolfram Sang <wsa@the-dreams.de>, Lv Zheng <lv.zheng@intel.com>,
	Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>,
	linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
	Huang Ying <ying.huang@intel.com>
Subject: Re: [RFC PATCH 5/5] GHES: Make NMI handler have a single reader
Date: Tue, 28 Apr 2015 10:42:07 -0400	[thread overview]
Message-ID: <20150428144207.GB98296@redhat.com> (raw)
In-Reply-To: <20150428143009.GA98296@redhat.com>

On Tue, Apr 28, 2015 at 10:30:09AM -0400, Don Zickus wrote:
> On Wed, Apr 01, 2015 at 09:45:53AM +0200, Jiri Kosina wrote:
> > On Fri, 27 Mar 2015, Borislav Petkov wrote:
> > 
> > > From: Jiri Kosina <jkosina@suse.cz>
> > > 
> > > Since GHES sources are global, we theoretically need only a single CPU
> > > reading them per NMI instead of a thundering herd of CPUs waiting on a
> > > spinlock in NMI context for no reason at all.
> > 
> > I originally wasn't 100% sure whether GHES sources are global (i.e. if it 
> > really doesn't matter which CPU is reading the registers), but looking at 
> > the code more it actually seems that this is really the right thing to do.
> > 
> > Rationale: ghes_ioremap_pfn_nmi() always ioremaps() (exclusively) the page 
> > with the registers, performs apei_read() (which is ghes-source specific, 
> > but not CPU-specific) and unmaps the page again.
> > 
> > There is nothing that would make this CPU-specific. Adding Huang Ying (the 
> > original author of the code) to confirm this. Huang?
> 
> Hi,
> 
> I believe the answer to this question is no, they are not global but
> instead external.  All external NMIs are routed to one cpu, normally cpu0.
> This spinlock was made global to handle the 'someday' case of hotplugging
> the bsp cpu (cpu0).
> 
> The other external NMIs (IO_CHECK and SERR) suffer from the same spinlock
> problem.  I tried using an irq_workqueue to work around quirks there and
> PeterZ wasn't very fond of it (though he couldn't think of a better way to
> solve it).
> 
> This patch seems interesting but you might still run into the thundering
> herd problem with the global spinlock in
> arch/x86/kernel/nmi.c::default_do_nmi().  That functions grabs a global
> spinlock before processing the external NMI list (which GHES is a part of).

Grr, I mispoke.   I sent a patchset a year ago to split out internal and
external NMIs to simplify the problem.  So I wrote the above paragraph
thinking the GHES NMI handler was wrapped with the external NMI spinlock,
when in fact it isn't.  However, with perf running with lots of events, it
is possible to start 'swallowing' NMIs which requires passing through the
spinlock I just mentioned.  This might cause random delays in your
measurements and is still worth modifying.

Cheers,
Don

> 
> 
> So I am assuming this patch solves the 'thundering herd' problem by
> minimizing all the useless writes the spinlock would do for each cpu that
> noticed it had no work to do?
> 
> In that case, I am in favor of this solution and would like to apply a
> similar solution to arch/x86/kernel/nmi.c, to see if that helps there too.
> 
> Cheers,
> Don
> 
> 
> > 
> > > Do that.
> > 
> > I think this should indeed be pushed forward. It fixes horrible spinlock 
> > contention on systems which are under NMI storm (such as when perf is 
> > active) unrelated to GHES.
> > 
> > Thanks,
> > 
> > -- 
> > Jiri Kosina
> > SUSE Labs
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

  reply	other threads:[~2015-04-28 14:42 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-27  9:22 [RFC PATCH 0/5] GHES NMI handler cleanup Borislav Petkov
2015-03-27  9:22 ` [RFC PATCH 1/5] GHES: Carve out error queueing in a separate function Borislav Petkov
2015-03-27  9:22 ` [RFC PATCH 2/5] GHES: Carve out the panic functionality Borislav Petkov
2015-03-27  9:22 ` [RFC PATCH 3/5] GHES: Panic right after detection Borislav Petkov
2015-03-27  9:22 ` [RFC PATCH 4/5] GHES: Elliminate double-loop in the NMI handler Borislav Petkov
2015-03-27  9:22 ` [RFC PATCH 5/5] GHES: Make NMI handler have a single reader Borislav Petkov
2015-04-01  7:45   ` Jiri Kosina
2015-04-01 13:49     ` Borislav Petkov
2015-04-23  8:39       ` Jiri Kosina
2015-04-23  8:59         ` Borislav Petkov
2015-04-23 18:00           ` Luck, Tony
2015-04-27 20:23             ` Borislav Petkov
2015-04-28 14:30     ` Don Zickus
2015-04-28 14:42       ` Don Zickus [this message]
2015-04-28 14:55       ` Borislav Petkov
2015-04-28 15:35         ` Don Zickus
2015-04-28 16:22           ` Borislav Petkov
2015-04-28 18:44             ` Don Zickus
2015-05-04 15:40               ` Borislav Petkov
2015-04-27  3:16   ` Zheng, Lv
2015-04-27  8:46     ` Borislav Petkov
2015-04-28  0:44       ` Zheng, Lv
2015-04-28  2:24       ` Zheng, Lv
2015-04-28  7:38         ` Borislav Petkov
2015-04-28 13:38   ` Zheng, Lv
2015-04-28 13:59     ` Borislav Petkov
2015-04-29  0:24       ` Zheng, Lv
2015-04-29  0:49       ` Zheng, Lv
2015-04-29  8:13         ` Borislav Petkov
2015-04-30  8:05           ` Zheng, Lv
2015-04-30  8:48             ` Borislav Petkov
2015-05-02  0:34               ` Zheng, Lv

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150428144207.GB98296@redhat.com \
    --to=dzickus@redhat.com \
    --cc=bp@alien8.de \
    --cc=bp@suse.de \
    --cc=gong.chen@linux.intel.com \
    --cc=jkosina@suse.cz \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lv.zheng@intel.com \
    --cc=n-horiguchi@ah.jp.nec.com \
    --cc=rjw@rjwysocki.net \
    --cc=tomasz.nowicki@linaro.org \
    --cc=tony.luck@intel.com \
    --cc=wsa@the-dreams.de \
    --cc=ying.huang@intel.com \
    --subject='Re: [RFC PATCH 5/5] GHES: Make NMI handler have a single reader' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).