RE: [RFC PATCH 5/5] GHES: Make NMI handler have a single reader

["Zheng, Lv" <lv.zheng@intel.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1659 bytes --]

Hi,

> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, April 28, 2015 9:59 PM
> Subject: Re: [RFC PATCH 5/5] GHES: Make NMI handler have a single reader
> 
> On Tue, Apr 28, 2015 at 01:38:41PM +0000, Zheng, Lv wrote:
> > > -	raw_spin_lock(&ghes_nmi_lock);
> > > +	if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
> > > +		return ret;
> > > +
> >
> > if (atomic_cmpxchg(&ghes_in_nmi, 0, 1))
> > 	return ret;
> 
> Ok, now I understand what you mean.
> 
> We absolutely want to use atomic_add_unless() because we get to save us
> the expensive
> 
> 	LOCK; CMPXCHG
> 
> if the value was already 1. Which is exactly what this patch is trying
> to avoid - a thundering herd of cores CMPXCHGing a global variable.

IMO, on most architectures, the "cmp" part should work just like what you've done with "if".
And on some architectures, if the "xchg" doesn't happen, the "cmp" part even won't cause a pipe line hazard.

Thanks and best regards
-Lv


> 
> I.e.,
> 
> 	movl	ghes_in_nmi(%rip), %ecx	# MEM[(const int *)&ghes_in_nmi], c
> 	cmpl	$1, %ecx	#, c
> 	je	.L311	#,				<--- exit here if ghes_in_nmi == 1.
> 	leal	1(%rcx), %edx	#, D.37163
> 	movl	%ecx, %eax	# c, c
> #APP
> # 177 "./arch/x86/include/asm/atomic.h" 1
> 	.pushsection .smp_locks,"a"
> .balign 4
> .long 671f - .
> .popsection
> 671:
> 	lock; cmpxchgl %edx,ghes_in_nmi(%rip)	# D.37163, MEM[(volatile u32 *)&ghes_in_nmi]
> # 0 "" 2
> #NO_APP
> 
> --
> Regards/Gruss,
>     Boris.
> 
> ECO tip #101: Trim your mails when you reply.
> --
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥