Re: Re: One question about ghes_notify_nmi

Re: Re: One question about ghes_notify_nmi

[yaoaili126]

From: Aili Yao <yaoaili@kingsoft.com>

Hi! Thanks for your reply.
Got you!
And for x86 platform, NMI is not only for hw errors, it does have some other
functions like watchdog, and maybe others i don't know.
when CPU is in heavy workload, the NMI watchdog will be triggered 
repeatily, and it will come to ghes_notify_nmi, the atomic raw_spin_lock
may lock the memory bus which may have little performance inpact to other
coresi i think.
I think you may modify it.

Thanks
Aili Yao

> -----Original Message-----
> From: James Morse [mailto:james.morse@arm.com]
> Sent: Friday, October 30, 2020 8:40 PM
> To: yaoaili126@163.com
> Cc: rjw@rjwysocki.net; lenb@kernel.org; tony.luck@intel.com; bp@alien8.de;
> linux-acpi@vger.kernel.org; YANGFENG1
> <YANGFENG1@kingsoft.com>; yaoaili<yaoaili@kingsoft.com>
> Subject: Re: One question about ghes_notify_nmi
> 
> Hello,
> 
> On 30/10/2020 02:41, yaoaili126@163.com wrote:
> > From: Aili Yao <yaoaili@kingsoft.com>
> > Sorry for my ignorance，When I look in to this code, I am totally condused.
> 
> No worries - this code is pretty confusing!
> 
> 
> > The Line 1136 has guarranted that Only one NMI will enter following
> > code I think, Is this right? if so, what is ghes_notify_lock_nmi going to
> pretect?
> 
> Looking at one of the others like ghes_notify_sea() might be simpler.
> The lock protects the fixmap slot, in case the notification occurs on multiple
> CPUs.
> 
> notify_nmi is weird as it has this atomic_add_unless() which seems to throw
> away some of the CPUs if they arrive ~together. It was added by commit
> 6fe9e7c26a971 ("GHES: Make NMI handler have a single reader"), which
> describes the motivation.
> 
> I'm not familiar with how x86 CPUs trigger NMI. From the commit message
> I've assumed this means there is some broadcast source of NMI, that is never
> firmware-first. I thought the trip via SMM for firmware-first did 'something'
> to hold the other CPUs so only one CPU takes the NMI - but I couldn't find it
> last time I went looking.
> 
> As I've no idea how this works, I decided not to change it. I added the
> spinlock so that the the fixmap slot provided to
> ghes_in_nmi_spool_from_list() is always protected by a spinlock, but the
> atomic means that for notify_nmi, the lock will never be contended.
> 
> 
> 
> Thanks,
> 
> James

Re: One question about ghes_notify_nmi

[James Morse]

Hello,

On 03/11/2020 07:31, yaoaili126@163.com wrote:
> From: Aili Yao <yaoaili@kingsoft.com>
> 
> Hi! Thanks for your reply.
> Got you!
> And for x86 platform, NMI is not only for hw errors, it does have some other
> functions like watchdog, and maybe others i don't know.
> when CPU is in heavy workload, the NMI watchdog will be triggered 
> repeatily, and it will come to ghes_notify_nmi, the atomic raw_spin_lock
> may lock the memory bus which may have little performance inpact to other
> coresi i think.
> I think you may modify it.

But the atomic property is needed. Without it, according to the commit that added that
code, CPUs serialise in the GHES code.


Thanks,

James