[PATCH 0/4] amd64_edac: misc fixes

[PATCH 0/4] amd64_edac: misc fixes

[Borislav Petkov]

Hi,

here are some minor fixlets which came up lately. I'm sending them
separately as an offset from the rest of the patchset since nothing has
changed there. For details, see

git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git for-next

Thanks,
Boris.

[PATCH 1/4] x86: msr.h: fix build error

[Borislav Petkov]

From: Alexander Beregalov <a.beregalov@gmail.com>

Fix this build error:
.../asm/msr.h: In function 'rdmsr_on_cpus':
.../asm/msr.h:248: error: request for member 'l' in something not a structure or union
.../asm/msr.h:248: error: request for member 'h' in something not a structure or union
.../asm/msr.h:248: error: too few arguments to function 'rdmsr_on_cpu'
.../asm/msr.h: In function 'wrmsr_on_cpus':
.../asm/msr.h:253: error: request for member 'l' in something not a structure or union
.../asm/msr.h:253: error: request for member 'h' in something not a structure or union
.../asm/msr.h:253: error: too few arguments to function 'wrmsr_on_cpu'

This is !SMP code so `cpu` should be 0.

Signed-off-by: Alexander Beregalov <a.beregalov@gmail.com>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/include/asm/msr.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index e49c14e..fa082ba 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -243,14 +243,14 @@ static inline int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h)
 	return 0;
 }
 static inline int rdmsr_on_cpus(const cpumask_t *m, u32 msr_no,
-				struct msr **msrs)
+				struct msr *msrs)
 {
-	return rdmsr_on_cpu(msr_no, &(msrs[0].l), &(msrs[0].h));
+	return rdmsr_on_cpu(0, msr_no, &(msrs[0].l), &(msrs[0].h));
 }
 static inline int wrmsr_on_cpus(const cpumask_t *m, u32 msr_no,
-				struct msr **msrs)
+				struct msr *msrs)
 {
-	return wrmsr_on_cpu(msr_no, msrs[0].l, msrs[0].h);
+	return wrmsr_on_cpu(0, msr_no, msrs[0].l, msrs[0].h);
 }
 static inline int rdmsr_safe_on_cpu(unsigned int cpu, u32 msr_no,
 				    u32 *l, u32 *h)
-- 
1.6.2.4

[PATCH 2/4] amd64_edac: do not enable module by default

[Borislav Petkov]

While at it, fix a link failure when !K8_NB.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 drivers/edac/Kconfig |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 1c5a325..d294421 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -60,11 +60,10 @@ config EDAC_MM_EDAC
 
 config EDAC_AMD64
 	tristate "AMD64 (Opteron, Athlon64) K8, F10h, F11h"
-	depends on EDAC_MM_EDAC && X86 && PCI
-	default m
+	depends on EDAC_MM_EDAC && K8_NB && X86_64 && PCI
 	help
-	Support for error detection and correction on the AMD 64
-	Families of Memory Controllers (K8, F10h and F11h)
+	  Support for error detection and correction on the AMD 64
+	  Families of Memory Controllers (K8, F10h and F11h)
 
 config EDAC_AMD64_ERROR_INJECTION
 	bool "Sysfs Error Injection facilities"
-- 
1.6.2.4

[PATCH 3/4] EDAC: do not enable modules by default

[Borislav Petkov]

Prevent EDAC compilation units from being built by default and let the
user explicitly select the needed modules.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 drivers/edac/Kconfig |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index d294421..bb2cff8 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -49,7 +49,6 @@ config EDAC_DEBUG_VERBOSE
 
 config EDAC_MM_EDAC
 	tristate "Main Memory EDAC (Error Detection And Correction) reporting"
-	default y
 	help
 	  Some systems are able to detect and correct errors in main
 	  memory.  EDAC can report statistics on memory error
-- 
1.6.2.4

[PATCH 4/4] amd64_edac: add MAINTAINERS entry

[Borislav Petkov]

CC: Doug Thompson <norsk5@yahoo.com>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 MAINTAINERS |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2b349ba..1b93ae4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1961,6 +1961,16 @@ F:	Documentation/edac.txt
 F:	drivers/edac/edac_*
 F:	include/linux/edac.h
 
+EDAC-AMD64
+P:	Doug Thompson
+M:	dougthompson@xmission.com
+P:	Borislav Petkov
+M:	borislav.petkov@amd.com
+L:	bluesmoke-devel@lists.sourceforge.net (moderated for non-subscribers)
+W:	bluesmoke.sourceforge.net
+S:	Supported
+F:	drivers/edac/amd64_edac*
+
 EDAC-E752X
 P:	Mark Gross
 P:	Doug Thompson
-- 
1.6.2.4

Re: [PATCH 0/4] amd64_edac: misc fixes

[Randy Dunlap]

Borislav Petkov wrote:
> Hi,
> 
> here are some minor fixlets which came up lately. I'm sending them
> separately as an offset from the rest of the patchset since nothing has
> changed there. For details, see
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git for-next

Acked-by: Randy Dunlap <randy.dunlap@oracle.com> # and Tested-by:


Thanks.
-- 
~Randy
LPC 2009, Sept. 23-25, Portland, Oregon
http://linuxplumbersconf.org/2009/

Re: [PATCH 0/4] amd64_edac: misc fixes

[Andrew Morton]

On Wed, 20 May 2009 20:43:53 +0200
Borislav Petkov <borislav.petkov@amd.com> wrote:

> here are some minor fixlets which came up lately. I'm sending them
> separately as an offset from the rest of the patchset since nothing has
> changed there. For details, see
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git for-next

My linux-next x86_64 allmodconfig fails to build drivers/edac/amd64_edac.o:

{standard input}: Assembler messages:
{standard input}:3026: Error: no such instruction: `popcnt %eax,%ecx'

Re: [PATCH 0/4] amd64_edac: misc fixes

[Borislav Petkov]

Hi,

On Thu, May 28, 2009 at 04:47:20PM -0700, Andrew Morton wrote:
> On Wed, 20 May 2009 20:43:53 +0200
> Borislav Petkov <borislav.petkov@amd.com> wrote:
> 
> > here are some minor fixlets which came up lately. I'm sending them
> > separately as an offset from the rest of the patchset since nothing has
> > changed there. For details, see
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git for-next
> 
> My linux-next x86_64 allmodconfig fails to build drivers/edac/amd64_edac.o:
> 
> {standard input}: Assembler messages:
> {standard input}:3026: Error: no such instruction: `popcnt %eax,%ecx'

I guess this is a rather old(er) toolchain you got there. What do

as --version
gcc --version

say?

-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

Re: [PATCH 0/4] amd64_edac: misc fixes

[Andrew Morton]

On Fri, 29 May 2009 12:33:29 +0200
Borislav Petkov <borislav.petkov@amd.com> wrote:

> Hi,
> 
> On Thu, May 28, 2009 at 04:47:20PM -0700, Andrew Morton wrote:
> > On Wed, 20 May 2009 20:43:53 +0200
> > Borislav Petkov <borislav.petkov@amd.com> wrote:
> > 
> > > here are some minor fixlets which came up lately. I'm sending them
> > > separately as an offset from the rest of the patchset since nothing has
> > > changed there. For details, see
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git for-next
> > 
> > My linux-next x86_64 allmodconfig fails to build drivers/edac/amd64_edac.o:
> > 
> > {standard input}: Assembler messages:
> > {standard input}:3026: Error: no such instruction: `popcnt %eax,%ecx'
> 
> I guess this is a rather old(er) toolchain you got there. What do
> 
> as --version
> gcc --version
> 
> say?

gcc-4.0.2, gas-2.16.1

Here's your sent-off-list patch (please don't do that):

diff -puN drivers/edac/amd64_edac.c~edac-build-fix drivers/edac/amd64_edac.c
--- a/drivers/edac/amd64_edac.c~edac-build-fix
+++ a/drivers/edac/amd64_edac.c
@@ -1419,7 +1419,7 @@ static u32 f10_determine_channel(struct 
 		if (dct_sel_interleave_addr(pvt) == 0)
 			cs = sys_addr >> 6 & 1;
 		else if ((dct_sel_interleave_addr(pvt) >> 1) & 1) {
-			temp = popcnt((u32) ((sys_addr >> 16) & 0x1F)) % 2;
+			temp = hweight_long((u32) ((sys_addr >> 16) & 0x1F)) % 2;
 
 			if (dct_sel_interleave_addr(pvt) & 1)
 				cs = (sys_addr >> 9 & 1) ^ temp;
diff -puN drivers/edac/amd64_edac.h~edac-build-fix drivers/edac/amd64_edac.h
--- a/drivers/edac/amd64_edac.h~edac-build-fix
+++ a/drivers/edac/amd64_edac.h
@@ -443,19 +443,6 @@ enum {
 #define K8_MSR_MC4STAT			0x0411
 #define K8_MSR_MC4ADDR			0x0412
 
-/*
- * popcnt - count the set bits in a bit vector.
- * @vec - bit vector
- *
- * This instruction is supported only on F10h and later CPUs.
- */
-#define popcnt(x)						\
-({								\
-	typeof(x) __ret;					\
-	__asm__("popcnt %1, %0" : "=r" (__ret) : "r" (x));	\
-	__ret;							\
-})
-
 /* AMD sets the first MC device at device ID 0x18. */
 static inline int get_mc_node_id_from_pdev(struct pci_dev *pdev)
 {
_

That'll work.

A suitable way to solve all this would be to arrange for x86's hweight()
to use the popcnt instruction, where that makes sense.

Re: [PATCH 0/4] amd64_edac: misc fixes

[Borislav Petkov]

On Fri, May 29, 2009 at 01:01:15PM -0700, Andrew Morton wrote:
> gcc-4.0.2, gas-2.16.1

yep, just as I thought. binutils got the Fam10h support around July 2006
and yours are from June 2005.

> Here's your sent-off-list patch (please don't do that):

Ups, sorry. This wasn't meant to be a patch but only to show the
functional change. I'll integrate it as such in the patchset and rebase
the whole thing next week.

> diff -puN drivers/edac/amd64_edac.c~edac-build-fix drivers/edac/amd64_edac.c
> --- a/drivers/edac/amd64_edac.c~edac-build-fix
> +++ a/drivers/edac/amd64_edac.c
> @@ -1419,7 +1419,7 @@ static u32 f10_determine_channel(struct 
>  		if (dct_sel_interleave_addr(pvt) == 0)
>  			cs = sys_addr >> 6 & 1;
>  		else if ((dct_sel_interleave_addr(pvt) >> 1) & 1) {
> -			temp = popcnt((u32) ((sys_addr >> 16) & 0x1F)) % 2;
> +			temp = hweight_long((u32) ((sys_addr >> 16) & 0x1F)) % 2;
>  
>  			if (dct_sel_interleave_addr(pvt) & 1)
>  				cs = (sys_addr >> 9 & 1) ^ temp;
> diff -puN drivers/edac/amd64_edac.h~edac-build-fix drivers/edac/amd64_edac.h
> --- a/drivers/edac/amd64_edac.h~edac-build-fix
> +++ a/drivers/edac/amd64_edac.h
> @@ -443,19 +443,6 @@ enum {
>  #define K8_MSR_MC4STAT			0x0411
>  #define K8_MSR_MC4ADDR			0x0412
>  
> -/*
> - * popcnt - count the set bits in a bit vector.
> - * @vec - bit vector
> - *
> - * This instruction is supported only on F10h and later CPUs.
> - */
> -#define popcnt(x)						\
> -({								\
> -	typeof(x) __ret;					\
> -	__asm__("popcnt %1, %0" : "=r" (__ret) : "r" (x));	\
> -	__ret;							\
> -})
> -
>  /* AMD sets the first MC device at device ID 0x18. */
>  static inline int get_mc_node_id_from_pdev(struct pci_dev *pdev)
>  {
> _
> 
> That'll work.
> 
> A suitable way to solve all this would be to arrange for x86's hweight()
> to use the popcnt instruction, where that makes sense.

That's funny, actually we _have_ _been_ experimenting here with
replacing hweight with popcnt and there's only a minor speedup. We
originally thought it might be a good idea to do the Hamming weight
calculation in hardware, especially when this is used in hot paths like
the scheduler but the bitmask counting is not being done enough times
to actually notice any significant improvement. Besides, the hweight_*
thingies are quite fast the way they are.

But sure, I could dig out my patchset and rediff it for review - it is
pretty small, actually. Also, I've been thinking about how the old(er)
toolchain problem can be addressed and one fairly doable thing would be
if I'd query the gas version in the kernel Makefile and define popcnt
dependent on it and for older assemblers simply slap in the opcode and
fixate the operands in an inline assembly so that it works.

Possible issues with that would be if someone uses a different toolchain
but do we support that at all? I'm guessing distro-patched bintuils
won't be a problem since even if they introduced popcnt support earlier,
the opcode would still be correct.

Opinions? Comments?

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/4] amd64_edac: misc fixes

[Andrew Morton]

On Sat, 30 May 2009 10:19:54 +0200 Borislav Petkov <petkovbb@googlemail.com> wrote:

> Also, I've been thinking about how the old(er)
> toolchain problem can be addressed and one fairly doable thing would be
> if I'd query the gas version in the kernel Makefile and define popcnt
> dependent on it and for older assemblers simply slap in the opcode and
> fixate the operands in an inline assembly so that it works.

We've done that before.  BUG() is one case (for other reasons), I think.

But if we have the code in there which uese the literal opcode, there's
no need to query gas or to add the conditional.

Is popcnt supported on all CPUs?

Re: [PATCH 0/4] amd64_edac: misc fixes

[Borislav Petkov]

On Sat, May 30, 2009 at 01:40:07AM -0700, Andrew Morton wrote:
> On Sat, 30 May 2009 10:19:54 +0200 Borislav Petkov <petkovbb@googlemail.com> wrote:
> 
> > Also, I've been thinking about how the old(er)
> > toolchain problem can be addressed and one fairly doable thing would be
> > if I'd query the gas version in the kernel Makefile and define popcnt
> > dependent on it and for older assemblers simply slap in the opcode and
> > fixate the operands in an inline assembly so that it works.
> 
> We've done that before.  BUG() is one case (for other reasons), I think.
> 
> But if we have the code in there which uese the literal opcode, there's
> no need to query gas or to add the conditional.

Right, we can do that too. Stay tuned.

> Is popcnt supported on all CPUs?

All >= Fam10h.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/4] amd64_edac: misc fixes

[H. Peter Anvin]

Andrew Morton wrote:
> On Sat, 30 May 2009 10:19:54 +0200 Borislav Petkov <petkovbb@googlemail.com> wrote:
> 
>> Also, I've been thinking about how the old(er)
>> toolchain problem can be addressed and one fairly doable thing would be
>> if I'd query the gas version in the kernel Makefile and define popcnt
>> dependent on it and for older assemblers simply slap in the opcode and
>> fixate the operands in an inline assembly so that it works.
> 
> We've done that before.  BUG() is one case (for other reasons), I think.
> 
> But if we have the code in there which uese the literal opcode, there's
> no need to query gas or to add the conditional.
> 
> Is popcnt supported on all CPUs?

Obviously not, since it's a relatively new opcode.  However, it is
supported by both Intel and AMD with the opcode F3 0F B8 /r.

The "/r" is the real problem ... it means one can't just mimic it with
hard-coding .byte directives without fixing the arguments (which means a
performance hit.)  Furthermore, the 0F B8 opcode is JMPE, which doesn't
take the same arguments either.

We have these kinds of toolchain issues regularly.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 0/4] amd64_edac: misc fixes

[Borislav Petkov]

> Obviously not, since it's a relatively new opcode.  However, it is
> supported by both Intel and AMD with the opcode F3 0F B8 /r.
> 
> The "/r" is the real problem ... it means one can't just mimic it with
> hard-coding .byte directives without fixing the arguments (which means a
> performance hit.)  Furthermore, the 0F B8 opcode is JMPE, which doesn't
> take the same arguments either.

How about we pin the src/dst into a register:

#define popcnt_spelled(x)                                       \
({                                                              \
        typeof(x) __ret;                                        \
        __asm__(".byte 0xf3\n\t.byte 0x48\n\t.byte 0x0f\n\t"    \
                ".byte 0xb8\n\t.byte 0xc0\n\t"                  \
                : "=a" (__ret)                                  \
                : "0" (x));                                     \
        __ret;                                                  \
})

which generates

  40055e:       48 8b 45 e8             mov    -0x18(%rbp),%rax
  400562:       f3 48 0f b8 c0          popcnt %rax,%rax
  400567:       48 89 45 f8             mov    %rax,-0x8(%rbp)

here.

For < 64bit operand sizes, the operands get zero-extended so that
garbage in the high 32/48 bits of %rax doesn't corrupt the result.
We might even want to do the movzwq explicitly so that some compiler
doesn't decide to take the version with the "0f b6" opcode which
zero-extends only the 16-/32-bit register. This way, you can popcnt even
single bytes although the popcnt implementation doesn't allow single
byte operands.

  400572:       0f b7 45 f2             movzwl -0xe(%rbp),%eax
  400579:       f3 48 0f b8 c0          popcnt %rax,%rax
  40057e:       66 89 45 f6             mov    %ax,-0xa(%rbp)


So, in addition to popcnt itself, we have two movs added. This is still
less than the 30+ ops (+ function call overhead) that hweight* get
translated into. I'll redo my kernel build benchmarks tomorrow to get
some more recent numbers on the performance gain.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/4] amd64_edac: misc fixes

[H. Peter Anvin]

Borislav Petkov wrote:
> 
> How about we pin the src/dst into a register:
> 
> #define popcnt_spelled(x)                                       \
> ({                                                              \
>         typeof(x) __ret;                                        \
>         __asm__(".byte 0xf3\n\t.byte 0x48\n\t.byte 0x0f\n\t"    \
>                 ".byte 0xb8\n\t.byte 0xc0\n\t"                  \
>                 : "=a" (__ret)                                  \
>                 : "0" (x));                                     \
>         __ret;                                                  \
> })
> 
> which generates
> 
>   40055e:       48 8b 45 e8             mov    -0x18(%rbp),%rax
>   400562:       f3 48 0f b8 c0          popcnt %rax,%rax
>   400567:       48 89 45 f8             mov    %rax,-0x8(%rbp)
> 
> here.
> 

Yes, we would have to do something like that.

However, if you're doing that you shouldn't use typeof() there...
instead this should be turned into an inline function with explicit
64-bit types.

It would be good if we could get Kbuild to export some kind of macro
that we can use to test binutils version, so we can do something like:

#if BINUTILS_VERSION >= KERNEL_VERSION(2,18,50)
/* Do the right thing */
#else
/* Do the wrong thing */
#endif

> For < 64bit operand sizes, the operands get zero-extended so that
> garbage in the high 32/48 bits of %rax doesn't corrupt the result.
> We might even want to do the movzwq explicitly so that some compiler
> doesn't decide to take the version with the "0f b6" opcode which
> zero-extends only the 16-/32-bit register. This way, you can popcnt even
> single bytes although the popcnt implementation doesn't allow single
> byte operands.
> 
>   400572:       0f b7 45 f2             movzwl -0xe(%rbp),%eax
>   400579:       f3 48 0f b8 c0          popcnt %rax,%rax
>   40057e:       66 89 45 f6             mov    %ax,-0xa(%rbp)
> 
> 
> So, in addition to popcnt itself, we have two movs added. This is still
> less than the 30+ ops (+ function call overhead) that hweight* get
> translated into. I'll redo my kernel build benchmarks tomorrow to get
> some more recent numbers on the performance gain.

With explicit types, the compiler should do the right thing.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 0/4] amd64_edac: misc fixes

[H. Peter Anvin]

H. Peter Anvin wrote:
> 
> Yes, we would have to do something like that.
> 
> However, if you're doing that you shouldn't use typeof() there...
> instead this should be turned into an inline function with explicit
> 64-bit types.
> 
> It would be good if we could get Kbuild to export some kind of macro
> that we can use to test binutils version, so we can do something like:
> 
> #if BINUTILS_VERSION >= KERNEL_VERSION(2,18,50)
> /* Do the right thing */
> #else
> /* Do the wrong thing */
> #endif
> 

The other option, and perhaps a better option, is to key it on the
version of gcc; then we can use the gcc intrinsics __builtin_popcount(),
__builtin_popcountl() and __builtin_popcountll(), which should produce
better code since gcc can schedule them appropriately.

Probably also means passing -msse4.2 to gcc while hoping that that
doesn't enable any #TS-generating instructions (SSE 4.2 is mostly a
collection of integer instructions).

(H.J., any comments?)

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 0/4] amd64_edac: misc fixes

[H.J. Lu]

On Mon, Jun 1, 2009 at 10:02 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> H. Peter Anvin wrote:
>>
>> Yes, we would have to do something like that.
>>
>> However, if you're doing that you shouldn't use typeof() there...
>> instead this should be turned into an inline function with explicit
>> 64-bit types.
>>
>> It would be good if we could get Kbuild to export some kind of macro
>> that we can use to test binutils version, so we can do something like:
>>
>> #if BINUTILS_VERSION >= KERNEL_VERSION(2,18,50)
>> /* Do the right thing */
>> #else
>> /* Do the wrong thing */
>> #endif
>>
>
> The other option, and perhaps a better option, is to key it on the
> version of gcc; then we can use the gcc intrinsics __builtin_popcount(),
> __builtin_popcountl() and __builtin_popcountll(), which should produce
> better code since gcc can schedule them appropriately.
>
> Probably also means passing -msse4.2 to gcc while hoping that that
> doesn't enable any #TS-generating instructions (SSE 4.2 is mostly a
> collection of integer instructions).
>
> (H.J., any comments?)
>

I don't think -msse4.2 will generate any SSE instructions unless you
turn on the vectorizer.


-- 
H.J.

Re: [PATCH 0/4] amd64_edac: misc fixes

[Borislav Petkov]

On Mon, Jun 01, 2009 at 09:54:41AM -0700, H. Peter Anvin wrote:
> Borislav Petkov wrote:
> > 
> > How about we pin the src/dst into a register:
> > 
> > #define popcnt_spelled(x)                                       \
> > ({                                                              \
> >         typeof(x) __ret;                                        \
> >         __asm__(".byte 0xf3\n\t.byte 0x48\n\t.byte 0x0f\n\t"    \
> >                 ".byte 0xb8\n\t.byte 0xc0\n\t"                  \
> >                 : "=a" (__ret)                                  \
> >                 : "0" (x));                                     \
> >         __ret;                                                  \
> > })
> > 
> > which generates
> > 
> >   40055e:       48 8b 45 e8             mov    -0x18(%rbp),%rax
> >   400562:       f3 48 0f b8 c0          popcnt %rax,%rax
> >   400567:       48 89 45 f8             mov    %rax,-0x8(%rbp)
> > 
> > here.
> > 
> 
> Yes, we would have to do something like that.
> 
> However, if you're doing that you shouldn't use typeof() there...
> instead this should be turned into an inline function with explicit
> 64-bit types.

right.

> It would be good if we could get Kbuild to export some kind of macro
> that we can use to test binutils version, so we can do something like:
> 
> #if BINUTILS_VERSION >= KERNEL_VERSION(2,18,50)
> /* Do the right thing */
> #else
> /* Do the wrong thing */
> #endif

Actually, popcnt got added to gas in July 2006 so checking the gas
version should suffice, IMHO.

Anyway, I proposed something similar before but Andrew suggested that we
should simply slap in the opcode so we don't need the Kbuild changes.
The advantage of the approach is that it works unconditionally on all
toolchains and introduces less code changes. Hmm...

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/4] amd64_edac: misc fixes

[H. Peter Anvin]

Borislav Petkov wrote:
> Actually, popcnt got added to gas in July 2006 so checking the gas
> version should suffice, IMHO.

gas is part of binutils.

> Anyway, I proposed something similar before but Andrew suggested that we
> should simply slap in the opcode so we don't need the Kbuild changes.
> The advantage of the approach is that it works unconditionally on all
> toolchains and introduces less code changes. Hmm...

That really sucks, though, in the long run.  I personally prefer to have
the "right thing" -- which in this case is probably gcc intrinsics --
and then a fallback that will gradually fall out of use.

	-hpa

Re: [PATCH 0/4] amd64_edac: misc fixes

[Borislav Petkov]

On Mon, Jun 01, 2009 at 11:57:18AM -0700, H. Peter Anvin wrote:
> Borislav Petkov wrote:
> > Actually, popcnt got added to gas in July 2006 so checking the gas
> > version should suffice, IMHO.
> 
> gas is part of binutils.
> 
> > Anyway, I proposed something similar before but Andrew suggested that we
> > should simply slap in the opcode so we don't need the Kbuild changes.
> > The advantage of the approach is that it works unconditionally on all
> > toolchains and introduces less code changes. Hmm...
> 
> That really sucks, though, in the long run.  I personally prefer to have
> the "right thing" -- which in this case is probably gcc intrinsics --
> and then a fallback that will gradually fall out of use.

Ok, here's a simple performance data measurement exercise:

I went and rerouted all the cpumask_weight calls in sched.c through a
noinline local definition:

static noinline unsigned int my_weight(const struct cpumask *mask)
{
       return cpumask_weight(mask);
}

so that I could be able to dynamically ftrace the invocations. Compiling
a kernel (make -j8) on a quad core Fam10h gave the following trace
(excerpt):

          <idle>-0     [000]   313.120141: my_weight <-scheduler_tick
          <idle>-0     [000]   313.120145: my_weight <-select_nohz_load_balancer
          <idle>-0     [000]   313.124133: my_weight <-scheduler_tick
          <idle>-0     [000]   313.124138: my_weight <-select_nohz_load_balancer
          <idle>-0     [000]   313.128124: my_weight <-scheduler_tick
          <idle>-0     [000]   313.128127: my_weight <-select_nohz_load_balancer
          <idle>-0     [000]   313.132116: my_weight <-scheduler_tick
          <idle>-0     [000]   313.132120: my_weight <-select_nohz_load_balancer
          <idle>-0     [000]   313.136109: my_weight <-scheduler_tick
          <idle>-0     [000]   313.136114: my_weight <-select_nohz_load_balancer
           <...>-3986  [002]   313.138868: my_weight <-sched_balance_self
           <...>-3986  [002]   313.138870: my_weight <-sched_balance_self
           <...>-4064  [003]   313.138942: my_weight <-sched_balance_self
           <...>-4064  [003]   313.138945: my_weight <-sched_balance_self
           <...>-4064  [000]   313.142034: my_weight <-sched_balance_self
           <...>-4064  [000]   313.142037: my_weight <-sched_balance_self
           <...>-4065  [001]   313.143509: my_weight <-sched_balance_self
           <...>-4065  [001]   313.143511: my_weight <-sched_balance_self
            make-3777  [000]   313.146553: my_weight <-sched_balance_self
            make-3777  [000]   313.146554: my_weight <-sched_balance_self
           <...>-4066  [001]   313.146614: my_weight <-sched_balance_self
           <...>-4066  [001]   313.146614: my_weight <-sched_balance_self
           <...>-4066  [003]   313.149516: my_weight <-sched_balance_self


and the following stats:

compile time: ~309.373623 secs
my_weight calls on _all_ cores: 54005
	(cpu0: 14262, cpu1: 14417, cpu2: 11654, cpu3: 13672)

leading to approx. 174.56 calls per second on _ALL_ cores combined. If,
hypothetically speaking, this is a representative workload and we forget
the ftrace overhead, it looks like there's no need to switch to the
hardware version of hweight since this'll bring a bunch of code changes
which simply wouldn't justify themselves wrt to performance improvement.
It is just not worth the effort.

Of course, I'm open for suggestions wrt to a better workload but from
looking at the code, the most frequent hweight call site seems to be
scheduler_tick which happens with HZ frequency and even this is by
several magnitudes not enough for a measurable performance improvement.

Hmm..?

-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632