[PATCH] x86/raid6: correctly check for assembler capabilities

[PATCH] x86/raid6: correctly check for assembler capabilities

[Jan Beulich]

Just like for AVX2 (which simply needs an #if -> #ifdef conversion),
SSSE3 assembler support should be checked for before using it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Jim Kukunas <james.t.kukunas@linux.intel.com>
Cc: Neil Brown <neilb@suse.de>
---
 arch/x86/Makefile       |    1 +
 lib/raid6/algos.c       |    2 +-
 lib/raid6/recov_avx2.c  |    2 +-
 lib/raid6/recov_ssse3.c |    6 ++++++
 4 files changed, 9 insertions(+), 2 deletions(-)

--- 3.19-rc5/arch/x86/Makefile
+++ 3.19-rc5-raid6-SSSE3/arch/x86/Makefile
@@ -148,6 +148,7 @@ cfi-sections := $(call as-instr,.cfi_sec
 
 # does binutils support specific instructions?
 asinstr := $(call as-instr,fxsaveq (%rax),-DCONFIG_AS_FXSAVEQ=1)
+asinstr += $(call as-instr,pshufb %xmm0$(comma)%xmm0,-DCONFIG_AS_SSSE3=1)
 asinstr += $(call as-instr,crc32l %eax$(comma)%eax,-DCONFIG_AS_CRC32=1)
 avx_instr := $(call as-instr,vxorps %ymm0$(comma)%ymm1$(comma)%ymm2,-DCONFIG_AS_AVX=1)
 avx2_instr :=$(call as-instr,vpbroadcastb %xmm0$(comma)%ymm1,-DCONFIG_AS_AVX2=1)
--- 3.19-rc5/lib/raid6/algos.c
+++ 3.19-rc5-raid6-SSSE3/lib/raid6/algos.c
@@ -89,10 +89,10 @@ void (*raid6_datap_recov)(int, size_t, i
 EXPORT_SYMBOL_GPL(raid6_datap_recov);
 
 const struct raid6_recov_calls *const raid6_recov_algos[] = {
-#if (defined(__i386__) || defined(__x86_64__)) && !defined(__arch_um__)
 #ifdef CONFIG_AS_AVX2
 	&raid6_recov_avx2,
 #endif
+#ifdef CONFIG_AS_SSSE3
 	&raid6_recov_ssse3,
 #endif
 	&raid6_recov_intx1,
--- 3.19-rc5/lib/raid6/recov_avx2.c
+++ 3.19-rc5-raid6-SSSE3/lib/raid6/recov_avx2.c
@@ -8,7 +8,7 @@
  * of the License.
  */
 
-#if CONFIG_AS_AVX2
+#ifdef CONFIG_AS_AVX2
 
 #include <linux/raid/pq.h>
 #include "x86.h"
--- 3.19-rc5/lib/raid6/recov_ssse3.c
+++ 3.19-rc5-raid6-SSSE3/lib/raid6/recov_ssse3.c
@@ -7,6 +7,8 @@
  * of the License.
  */
 
+#ifdef CONFIG_AS_SSSE3
+
 #include <linux/raid/pq.h>
 #include "x86.h"
 
@@ -330,3 +332,7 @@ const struct raid6_recov_calls raid6_rec
 #endif
 	.priority = 1,
 };
+
+#else
+#warning "your version of binutils lacks SSSE3 support"
+#endif

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[Thomas Gleixner]

On Fri, 23 Jan 2015, Jan Beulich wrote:

> Just like for AVX2 (which simply needs an #if -> #ifdef conversion),
> SSSE3 assembler support should be checked for before using it.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Jim Kukunas <james.t.kukunas@linux.intel.com>
> Cc: Neil Brown <neilb@suse.de>
> ---
>  arch/x86/Makefile       |    1 +

For the x86 part:

Acked-by: Thomas Gleixner <tglx@linutronix.de>

This should go through the raid tree.

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

On Fri, 23 Jan 2015 17:16:59 +0100 (CET) Thomas Gleixner <tglx@linutronix.de>
wrote:

> On Fri, 23 Jan 2015, Jan Beulich wrote:
> 
> > Just like for AVX2 (which simply needs an #if -> #ifdef conversion),
> > SSSE3 assembler support should be checked for before using it.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > Cc: Jim Kukunas <james.t.kukunas@linux.intel.com>
> > Cc: Neil Brown <neilb@suse.de>
> > ---
> >  arch/x86/Makefile       |    1 +
> 
> For the x86 part:
> 
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> 
> This should go through the raid tree.

Thanks.  Applied to the raid tree.

NeilBrown


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[Paul Bolle]

On Fri, 2015-01-23 at 08:29 +0000, Jan Beulich wrote:
> Just like for AVX2 (which simply needs an #if -> #ifdef conversion),
> SSSE3 assembler support should be checked for before using it.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Jim Kukunas <james.t.kukunas@linux.intel.com>
> Cc: Neil Brown <neilb@suse.de>

This patch became commit be46ac86a81b ("x86/raid6: correctly check for
assembler capabilities") in today's linux-next (ie, next-20150203). I
noticed because a script I use to check linux-next spotted a potential
problem with it.

> ---
>  arch/x86/Makefile       |    1 +
>  lib/raid6/algos.c       |    2 +-
>  lib/raid6/recov_avx2.c  |    2 +-
>  lib/raid6/recov_ssse3.c |    6 ++++++
>  4 files changed, 9 insertions(+), 2 deletions(-)
> 
> --- 3.19-rc5/arch/x86/Makefile
> +++ 3.19-rc5-raid6-SSSE3/arch/x86/Makefile
> @@ -148,6 +148,7 @@ cfi-sections := $(call as-instr,.cfi_sec
>  
>  # does binutils support specific instructions?
>  asinstr := $(call as-instr,fxsaveq (%rax),-DCONFIG_AS_FXSAVEQ=1)
> +asinstr += $(call as-instr,pshufb %xmm0$(comma)%xmm0,-DCONFIG_AS_SSSE3=1)

This Makefile defines a preprocessor macro with a CONFIG_ prefix. Almost
all macros with that prefix are defined through the kconfig system. A
handful, like CONFIG_AS_SSSE3 and a few other macros defined in this
Makefile, are not. Apparently this is a pet peeve I share with few
people, but would any other prefix than CONFIG_ work for you too?

>  asinstr += $(call as-instr,crc32l %eax$(comma)%eax,-DCONFIG_AS_CRC32=1)
>  avx_instr := $(call as-instr,vxorps %ymm0$(comma)%ymm1$(comma)%ymm2,-DCONFIG_AS_AVX=1)
>  avx2_instr :=$(call as-instr,vpbroadcastb %xmm0$(comma)%ymm1,-DCONFIG_AS_AVX2=1)


Paul Bolle

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

On Tue, 03 Feb 2015 21:31:30 +0100 Paul Bolle <pebolle@tiscali.nl> wrote:

> On Fri, 2015-01-23 at 08:29 +0000, Jan Beulich wrote:
> > Just like for AVX2 (which simply needs an #if -> #ifdef conversion),
> > SSSE3 assembler support should be checked for before using it.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > Cc: Jim Kukunas <james.t.kukunas@linux.intel.com>
> > Cc: Neil Brown <neilb@suse.de>
> 
> This patch became commit be46ac86a81b ("x86/raid6: correctly check for
> assembler capabilities") in today's linux-next (ie, next-20150203). I
> noticed because a script I use to check linux-next spotted a potential
> problem with it.
> 
> > ---
> >  arch/x86/Makefile       |    1 +
> >  lib/raid6/algos.c       |    2 +-
> >  lib/raid6/recov_avx2.c  |    2 +-
> >  lib/raid6/recov_ssse3.c |    6 ++++++
> >  4 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > --- 3.19-rc5/arch/x86/Makefile
> > +++ 3.19-rc5-raid6-SSSE3/arch/x86/Makefile
> > @@ -148,6 +148,7 @@ cfi-sections := $(call as-instr,.cfi_sec
> >  
> >  # does binutils support specific instructions?
> >  asinstr := $(call as-instr,fxsaveq (%rax),-DCONFIG_AS_FXSAVEQ=1)
> > +asinstr += $(call as-instr,pshufb %xmm0$(comma)%xmm0,-DCONFIG_AS_SSSE3=1)
> 
> This Makefile defines a preprocessor macro with a CONFIG_ prefix. Almost
> all macros with that prefix are defined through the kconfig system. A
> handful, like CONFIG_AS_SSSE3 and a few other macros defined in this
> Makefile, are not. Apparently this is a pet peeve I share with few
> people, but would any other prefix than CONFIG_ work for you too?

Actually the prefix of this macro is "CONFIG_AS_", not "CONFIG_" :-)
CONFIG_AS_ is reserved for assembly magic, and is never used by the the
kconfig system.

(Well..... I might have made bits of that up, but "git grep 'config AS_'"
doesn't find anything).

NeilBrown


> 
> >  asinstr += $(call as-instr,crc32l %eax$(comma)%eax,-DCONFIG_AS_CRC32=1)
> >  avx_instr := $(call as-instr,vxorps %ymm0$(comma)%ymm1$(comma)%ymm2,-DCONFIG_AS_AVX=1)
> >  avx2_instr :=$(call as-instr,vpbroadcastb %xmm0$(comma)%ymm1,-DCONFIG_AS_AVX2=1)
> 
> 
> Paul Bolle


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[Paul Bolle]

On Wed, 2015-02-04 at 07:50 +1100, NeilBrown wrote:
> Actually the prefix of this macro is "CONFIG_AS_", not "CONFIG_" :-)
> CONFIG_AS_ is reserved for assembly magic, and is never used by the the
> kconfig system.
> 
> (Well..... I might have made bits of that up, but "git grep 'config AS_'"
> doesn't find anything).

That's correct, there are no Kconfig symbols starting with AS_. But
still, I would like to hear whether there's a reasonable chance I might
convince other people to adopt my peeve.

The thinking behind that peeve is, basically, that where people
encounter a CONFIG_* macro they should only have to check the .config
file to see how that macro was evaluated in the build that was used.

Thanks,


Paul Bolle

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

On Tue, 03 Feb 2015 22:03:35 +0100 Paul Bolle <pebolle@tiscali.nl> wrote:

> On Wed, 2015-02-04 at 07:50 +1100, NeilBrown wrote:
> > Actually the prefix of this macro is "CONFIG_AS_", not "CONFIG_" :-)
> > CONFIG_AS_ is reserved for assembly magic, and is never used by the the
> > kconfig system.
> > 
> > (Well..... I might have made bits of that up, but "git grep 'config AS_'"
> > doesn't find anything).
> 
> That's correct, there are no Kconfig symbols starting with AS_. But
> still, I would like to hear whether there's a reasonable chance I might
> convince other people to adopt my peeve.
> 
> The thinking behind that peeve is, basically, that where people
> encounter a CONFIG_* macro they should only have to check the .config
> file to see how that macro was evaluated in the build that was used.
> 

Personally, I don't care.

But I find that developers in general are more responsive to code than to
peeves.

So if you post a patch which makes the change that you want, then you are
more likely to get a useful response than if you just post a peeve.
It may not be the response you want of course....

NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[Valentin Rothberg]

On Tue, Feb 3, 2015 at 10:09 PM, NeilBrown <neilb@suse.de> wrote:
> On Tue, 03 Feb 2015 22:03:35 +0100 Paul Bolle <pebolle@tiscali.nl> wrote:
>
>> On Wed, 2015-02-04 at 07:50 +1100, NeilBrown wrote:
>> > Actually the prefix of this macro is "CONFIG_AS_", not "CONFIG_" :-)
>> > CONFIG_AS_ is reserved for assembly magic, and is never used by the the
>> > kconfig system.
>> >
>> > (Well..... I might have made bits of that up, but "git grep 'config AS_'"
>> > doesn't find anything).
>>
>> That's correct, there are no Kconfig symbols starting with AS_. But
>> still, I would like to hear whether there's a reasonable chance I might
>> convince other people to adopt my peeve.
>>
>> The thinking behind that peeve is, basically, that where people
>> encounter a CONFIG_* macro they should only have to check the .config
>> file to see how that macro was evaluated in the build that was used.
>>
>
> Personally, I don't care.

A problem with those identifiers is that the CONFIG_ prefix is
reserved for Kconfig features in Make and CPP syntax.  The _MODULE
suffix for CPP alone.  Sadly, this convention is only documented in
the Kconfig C code itself.  Nonetheless, such cases give hard times to
static analysis tools that then have to deal with such false
positives.

Kind regards,
 Valentin

> But I find that developers in general are more responsive to code than to
> peeves.
>
> So if you post a patch which makes the change that you want, then you are
> more likely to get a useful response than if you just post a peeve.
> It may not be the response you want of course....
>
> NeilBrown

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[Jan Beulich]

>>> On 03.02.15 at 22:03, <pebolle@tiscali.nl> wrote:
> On Wed, 2015-02-04 at 07:50 +1100, NeilBrown wrote:
>> Actually the prefix of this macro is "CONFIG_AS_", not "CONFIG_" :-)
>> CONFIG_AS_ is reserved for assembly magic, and is never used by the the
>> kconfig system.
>> 
>> (Well..... I might have made bits of that up, but "git grep 'config AS_'"
>> doesn't find anything).
> 
> That's correct, there are no Kconfig symbols starting with AS_. But
> still, I would like to hear whether there's a reasonable chance I might
> convince other people to adopt my peeve.
> 
> The thinking behind that peeve is, basically, that where people
> encounter a CONFIG_* macro they should only have to check the .config
> file to see how that macro was evaluated in the build that was used.

I too found it curious that CONFIG_* is being used here instead of
e.g. HAVE_*, but with the patch at hand I simply followed suit (as
that normally is the route involving less discussions in order to get
a necessary/desirable fix accepted). In the end I guess you'd need
to convince the x86 maintainers.

Jan

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[H. Peter Anvin]

On 02/03/2015 01:03 PM, Paul Bolle wrote:
> On Wed, 2015-02-04 at 07:50 +1100, NeilBrown wrote:
>> Actually the prefix of this macro is "CONFIG_AS_", not "CONFIG_" :-)
>> CONFIG_AS_ is reserved for assembly magic, and is never used by the the
>> kconfig system.
>>
>> (Well..... I might have made bits of that up, but "git grep 'config AS_'"
>> doesn't find anything).
> 
> That's correct, there are no Kconfig symbols starting with AS_. But
> still, I would like to hear whether there's a reasonable chance I might
> convince other people to adopt my peeve.
> 
> The thinking behind that peeve is, basically, that where people
> encounter a CONFIG_* macro they should only have to check the .config
> file to see how that macro was evaluated in the build that was used.
> 

There is a hope/intent that eventually the config system will be able to
incorporate toolchain dependencies for a bunch of reasons.

	-hpa

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[Paul Bolle]

On Tue, 2015-06-16 at 12:46 -0700, H. Peter Anvin wrote:
> There is a hope/intent that eventually the config system will be able to
> incorporate toolchain dependencies for a bunch of reasons.

This restarts a four months old thread with a one sentence remark. So
could you please elaborate, because now you've left me and, perhaps, the
other people reading this wondering what "toolchain dependencies"
actually means and what those "bunch of reasons" are.

Thanks,


Paul Bolle

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[H. Peter Anvin]

On 06/16/2015 12:56 PM, Paul Bolle wrote:
> On Tue, 2015-06-16 at 12:46 -0700, H. Peter Anvin wrote:
>> There is a hope/intent that eventually the config system will be able to
>> incorporate toolchain dependencies for a bunch of reasons.
> 
> This restarts a four months old thread with a one sentence remark. So
> could you please elaborate, because now you've left me and, perhaps, the
> other people reading this wondering what "toolchain dependencies"
> actually means and what those "bunch of reasons" are.
> 

Sorry, missed the date on this in my inbox for some reason.

So this related to the CONFIG_AS_ symbols for assembly.  We really would
like to do things like actually adding dependencies on assembler or
compiler support into Kconfig proper, rather than having two independent
mechanisms.  That way we could do, for example:

config RAID6_AVX2
	depends on X86 && AS_AVX2

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[Paul Bolle]

On Tue, 2015-06-16 at 13:05 -0700, H. Peter Anvin wrote:
> So this related to the CONFIG_AS_ symbols for assembly.  We really would
> like to do things like actually adding dependencies on assembler or
> compiler support into Kconfig proper, rather than having two independent
> mechanisms.  That way we could do, for example:
> 
> config RAID6_AVX2
> 	depends on X86 && AS_AVX2

I have no idea what AS_AVX2 means, sorry. So what would it mean if
someone has a .config with
    CONFIG_AS_AVX2=foo

Ie, what would happen if someone using a toolchain for which that would
not be possible tries to build a kernel with that symbol set? That
person's .config would end up containing
    # CONFIG_AS_AVX2 is not set

or similar, right? 


Paul Bolle

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[H. Peter Anvin]

On 06/16/2015 01:37 PM, Paul Bolle wrote:
> 
> I have no idea what AS_AVX2 means, sorry. So what would it mean if
> someone has a .config with
>     CONFIG_AS_AVX2=foo
> 
> Ie, what would happen if someone using a toolchain for which that would
> not be possible tries to build a kernel with that symbol set? That
> person's .config would end up containing
>     # CONFIG_AS_AVX2 is not set
> 
> or similar, right? 
> 

Yes, these would be generated, not user input.

	-hpa

Re: [PATCH] x86/raid6: correctly check for assembler capabilities

[Paul Bolle]

On Tue, 2015-06-16 at 13:42 -0700, H. Peter Anvin wrote:
> Yes, these would be generated, not user input.

I think someone (Jiri Kosina?) proposed shelling out to set Kconfig
values. I don't think any patches were ever submitted. But I guess
something along those lines would be needed to adopt the .config to the
toolchain at hand. As long as we don't do anything funny once we've
generated the .config file that might just work.

Yes, I know, the devil hides in the details.

Thanks,


Paul Bolle