[dpdk-dev] [PATCH] atm: Remove machine definition

[dpdk-dev] [PATCH] atm: Remove machine definition

[Neil Horman]

With the new updated requirement for SSE4.2, dpdk no longer supports
building on atom machines, as they only support up to SSE3.  Remove the
machine definition.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Monjalon <thomas@monjalon.net>
---
 mk/machine/atm/rte.vars.mk | 58 ----------------------------------------------
 1 file changed, 58 deletions(-)
 delete mode 100644 mk/machine/atm/rte.vars.mk

diff --git a/mk/machine/atm/rte.vars.mk b/mk/machine/atm/rte.vars.mk
deleted file mode 100644
index cfed1108b..000000000
--- a/mk/machine/atm/rte.vars.mk
+++ /dev/null
@@ -1,58 +0,0 @@
-#   BSD LICENSE
-#
-#   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
-#   All rights reserved.
-#
-#   Redistribution and use in source and binary forms, with or without
-#   modification, are permitted provided that the following conditions
-#   are met:
-#
-#     * Redistributions of source code must retain the above copyright
-#       notice, this list of conditions and the following disclaimer.
-#     * Redistributions in binary form must reproduce the above copyright
-#       notice, this list of conditions and the following disclaimer in
-#       the documentation and/or other materials provided with the
-#       distribution.
-#     * Neither the name of Intel Corporation nor the names of its
-#       contributors may be used to endorse or promote products derived
-#       from this software without specific prior written permission.
-#
-#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
-#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
-#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
-#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
-#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
-#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
-#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
-#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
-#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
-#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-#
-# machine:
-#
-#   - can define ARCH variable (overridden by cmdline value)
-#   - can define CROSS variable (overridden by cmdline value)
-#   - define MACHINE_CFLAGS variable (overridden by cmdline value)
-#   - define MACHINE_LDFLAGS variable (overridden by cmdline value)
-#   - define MACHINE_ASFLAGS variable (overridden by cmdline value)
-#   - can define CPU_CFLAGS variable (overridden by cmdline value) that
-#     overrides the one defined in arch.
-#   - can define CPU_LDFLAGS variable (overridden by cmdline value) that
-#     overrides the one defined in arch.
-#   - can define CPU_ASFLAGS variable (overridden by cmdline value) that
-#     overrides the one defined in arch.
-#   - may override any previously defined variable
-#
-
-# ARCH =
-# CROSS =
-# MACHINE_CFLAGS =
-# MACHINE_LDFLAGS =
-# MACHINE_ASFLAGS =
-# CPU_CFLAGS =
-# CPU_LDFLAGS =
-# CPU_ASFLAGS =
-
-MACHINE_CFLAGS = -march=atom
-- 
2.13.3

Re: [dpdk-dev] [PATCH] atm: Remove machine definition

[Bruce Richardson]

On Wed, Aug 09, 2017 at 04:24:25PM -0400, Neil Horman wrote:
> With the new updated requirement for SSE4.2, dpdk no longer supports
> building on atom machines, as they only support up to SSE3.  Remove
> the machine definition.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com> CC: Thomas Monjalon
> <thomas@monjalon.net> --- mk/machine/atm/rte.vars.mk | 58
> ---------------------------------------------- 1 file changed, 58
> deletions(-) delete mode 100644 mk/machine/atm/rte.vars.mk
>
Yes, good catch, that should have been removed. However, I think the
commit log should be updated to mention that it no longer supports
"early" atom machines, or some similar phrase. Atom cores for the last
number of years do support SSE4, see:
https://en.wikipedia.org/wiki/Silvermont for example.

With that change,
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH] atm: Remove machine definition

[Neil Horman]

On Thu, Aug 10, 2017 at 09:34:18AM +0100, Bruce Richardson wrote:
> On Wed, Aug 09, 2017 at 04:24:25PM -0400, Neil Horman wrote:
> > With the new updated requirement for SSE4.2, dpdk no longer supports
> > building on atom machines, as they only support up to SSE3.  Remove
> > the machine definition.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> CC: Thomas Monjalon
> > <thomas@monjalon.net> --- mk/machine/atm/rte.vars.mk | 58
> > ---------------------------------------------- 1 file changed, 58
> > deletions(-) delete mode 100644 mk/machine/atm/rte.vars.mk
> >
> Yes, good catch, that should have been removed. However, I think the
> commit log should be updated to mention that it no longer supports
> "early" atom machines, or some similar phrase. Atom cores for the last
> number of years do support SSE4, see:
> https://en.wikipedia.org/wiki/Silvermont for example.
> 

I don't really agree with that. If you want to make the claim that only early
atom machines aren't supported, the implication then is that later atom machines
are, and we should keep the machine type, and fix it to build for those targeted
machines (as it stands currently, the compiler errors out on building atom
because it only emits SSE3 instructions and can't inline an SSE4 builtin). I'd
be totally fine with fixing the atom build (which I imageine amounts to passing
-machine=atom -msse4 or something simmilar to the build options.  Once we do
that however, we likely need a runtime check for sse4 support as well, so we
don't just get random SIGILL errors at run time.  

Neil

> With that change,
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> 

Re: [dpdk-dev] [PATCH] atm: Remove machine definition

[Richardson, Bruce]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Thursday, August 10, 2017 12:34 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH] atm: Remove machine definition
> 
> On Thu, Aug 10, 2017 at 09:34:18AM +0100, Bruce Richardson wrote:
> > On Wed, Aug 09, 2017 at 04:24:25PM -0400, Neil Horman wrote:
> > > With the new updated requirement for SSE4.2, dpdk no longer supports
> > > building on atom machines, as they only support up to SSE3.  Remove
> > > the machine definition.
> > >
> > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> CC: Thomas
> > > Monjalon <thomas@monjalon.net> --- mk/machine/atm/rte.vars.mk | 58
> > > ---------------------------------------------- 1 file changed, 58
> > > deletions(-) delete mode 100644 mk/machine/atm/rte.vars.mk
> > >
> > Yes, good catch, that should have been removed. However, I think the
> > commit log should be updated to mention that it no longer supports
> > "early" atom machines, or some similar phrase. Atom cores for the last
> > number of years do support SSE4, see:
> > https://en.wikipedia.org/wiki/Silvermont for example.
> >
> 
> I don't really agree with that. If you want to make the claim that only
> early atom machines aren't supported, the implication then is that later
> atom machines are, and we should keep the machine type, and fix it to
> build for those targeted machines (as it stands currently, the compiler
> errors out on building atom because it only emits SSE3 instructions and
> can't inline an SSE4 builtin). I'd be totally fine with fixing the atom
> build (which I imageine amounts to passing -machine=atom -msse4 or
> something simmilar to the build options.  Once we do that however, we
> likely need a runtime check for sse4 support as well, so we don't just get
> random SIGILL errors at run time.
> 
> Neil

For the runtime checks, "rte_eal_init()" already calls "rte_cpu_is_supported()" checks to ensure that all instruction sets that are built in are supported by the runtime platform.

For fixing the atom build, I actually thought we were moving away from having special confirm files for the rte_machine type, and just passing the RTE_MACHINE build-time value directly through to the compiler as -march - especially since gcc and clang have started using the microarchitecture names directly in the march flag rather than the older names like core-i7-avx. In fact, "atom" is no longer listed in the manpage for gcc on my Fedora 26 system, only "bonnell", "silvermont" are present as atom architectures.

If you see a need for fixing the atm build file, I have no objections, but I think removing it is an acceptable solution as anyone who needs it can build for atom by manually editing the RTE_MACHINE type to "silvermont". [Though, I do think the comments in config/common_base could do with being updated, since it's not required any more to have a build directory for each machine type.]

Regards,
/Bruce

Re: [dpdk-dev] [PATCH] atm: Remove machine definition

[Neil Horman]

On Thu, Aug 10, 2017 at 02:04:00PM +0000, Richardson, Bruce wrote:
> 
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Thursday, August 10, 2017 12:34 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>
> > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>
> > Subject: Re: [dpdk-dev] [PATCH] atm: Remove machine definition
> > 
> > On Thu, Aug 10, 2017 at 09:34:18AM +0100, Bruce Richardson wrote:
> > > On Wed, Aug 09, 2017 at 04:24:25PM -0400, Neil Horman wrote:
> > > > With the new updated requirement for SSE4.2, dpdk no longer supports
> > > > building on atom machines, as they only support up to SSE3.  Remove
> > > > the machine definition.
> > > >
> > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> CC: Thomas
> > > > Monjalon <thomas@monjalon.net> --- mk/machine/atm/rte.vars.mk | 58
> > > > ---------------------------------------------- 1 file changed, 58
> > > > deletions(-) delete mode 100644 mk/machine/atm/rte.vars.mk
> > > >
> > > Yes, good catch, that should have been removed. However, I think the
> > > commit log should be updated to mention that it no longer supports
> > > "early" atom machines, or some similar phrase. Atom cores for the last
> > > number of years do support SSE4, see:
> > > https://en.wikipedia.org/wiki/Silvermont for example.
> > >
> > 
> > I don't really agree with that. If you want to make the claim that only
> > early atom machines aren't supported, the implication then is that later
> > atom machines are, and we should keep the machine type, and fix it to
> > build for those targeted machines (as it stands currently, the compiler
> > errors out on building atom because it only emits SSE3 instructions and
> > can't inline an SSE4 builtin). I'd be totally fine with fixing the atom
> > build (which I imageine amounts to passing -machine=atom -msse4 or
> > something simmilar to the build options.  Once we do that however, we
> > likely need a runtime check for sse4 support as well, so we don't just get
> > random SIGILL errors at run time.
> > 
> > Neil
> 
> For the runtime checks, "rte_eal_init()" already calls "rte_cpu_is_supported()" checks to ensure that all instruction sets that are built in are supported by the runtime platform.
> 
Ah, yes, then we have the runtime check, and can just consider fixing the
machine build.

> For fixing the atom build, I actually thought we were moving away from having special confirm files for the rte_machine type, and just passing the RTE_MACHINE build-time value directly through to the compiler as -march - especially since gcc and clang have started using the microarchitecture names directly in the march flag rather than the older names like core-i7-avx. In fact, "atom" is no longer listed in the manpage for gcc on my Fedora 26 system, only "bonnell", "silvermont" are present as atom architectures.
> 
Well, we may be, though I don't see any discussion of that online (at least
nothing obvious).  Though it should be noted that atom is still a supported
machine type, its just not documented, and the result is a broken build, as
-march atom assumes no sse4 instructions are allowed.


> If you see a need for fixing the atm build file, I have no objections, but I think removing it is an acceptable solution as anyone who needs it can build for atom by manually editing the RTE_MACHINE type to "silvermont". [Though, I do think the comments in config/common_base could do with being updated, since it's not required any more to have a build directory for each machine type.]
> 
I don't really, I'm not in any way comitted to keeping atom support around. My
bigger concern, and I understand I didn't really make this clear above, and I
apologize for that, is that it doesn't make much sense to me to change the
commit log to say "we're only removing support for some older atom systems", if
the commit actually removes support for all of them (which is exactly what it
does).  If your intent is to still support the atom systems that do execute
sse4, then we should create machine definitions (using the existing way, or some
to be determined method) to explicitly support them in a separate commit.  I
wouldn't be opposed to doing it in this commit and adding your statement even,
though I don't think I have access to any atom systems that support sse4 for the
purposes of testing it out.

Neil

> Regards,
> /Bruce
> 
> 
> 

Re: [dpdk-dev] [PATCH] atm: Remove machine definition

[Bruce Richardson]

On Thu, Aug 10, 2017 at 11:15:27AM -0400, Neil Horman wrote:
> On Thu, Aug 10, 2017 at 02:04:00PM +0000, Richardson, Bruce wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > Sent: Thursday, August 10, 2017 12:34 PM
> > > To: Richardson, Bruce <bruce.richardson@intel.com>
> > > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>
> > > Subject: Re: [dpdk-dev] [PATCH] atm: Remove machine definition
> > > 
> > > On Thu, Aug 10, 2017 at 09:34:18AM +0100, Bruce Richardson wrote:
> > > > On Wed, Aug 09, 2017 at 04:24:25PM -0400, Neil Horman wrote:
> > > > > With the new updated requirement for SSE4.2, dpdk no longer supports
> > > > > building on atom machines, as they only support up to SSE3.  Remove
> > > > > the machine definition.
> > > > >
> > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> CC: Thomas
> > > > > Monjalon <thomas@monjalon.net> --- mk/machine/atm/rte.vars.mk | 58
> > > > > ---------------------------------------------- 1 file changed, 58
> > > > > deletions(-) delete mode 100644 mk/machine/atm/rte.vars.mk
> > > > >
> > > > Yes, good catch, that should have been removed. However, I think the
> > > > commit log should be updated to mention that it no longer supports
> > > > "early" atom machines, or some similar phrase. Atom cores for the last
> > > > number of years do support SSE4, see:
> > > > https://en.wikipedia.org/wiki/Silvermont for example.
> > > >
> > > 
> > > I don't really agree with that. If you want to make the claim that only
> > > early atom machines aren't supported, the implication then is that later
> > > atom machines are, and we should keep the machine type, and fix it to
> > > build for those targeted machines (as it stands currently, the compiler
> > > errors out on building atom because it only emits SSE3 instructions and
> > > can't inline an SSE4 builtin). I'd be totally fine with fixing the atom
> > > build (which I imageine amounts to passing -machine=atom -msse4 or
> > > something simmilar to the build options.  Once we do that however, we
> > > likely need a runtime check for sse4 support as well, so we don't just get
> > > random SIGILL errors at run time.
> > > 
> > > Neil
> > 
> > For the runtime checks, "rte_eal_init()" already calls "rte_cpu_is_supported()" checks to ensure that all instruction sets that are built in are supported by the runtime platform.
> > 
> Ah, yes, then we have the runtime check, and can just consider fixing the
> machine build.
> 
> > For fixing the atom build, I actually thought we were moving away from having special confirm files for the rte_machine type, and just passing the RTE_MACHINE build-time value directly through to the compiler as -march - especially since gcc and clang have started using the microarchitecture names directly in the march flag rather than the older names like core-i7-avx. In fact, "atom" is no longer listed in the manpage for gcc on my Fedora 26 system, only "bonnell", "silvermont" are present as atom architectures.
> > 
> Well, we may be, though I don't see any discussion of that online (at least
> nothing obvious).  Though it should be noted that atom is still a supported
> machine type, its just not documented, and the result is a broken build, as
> -march atom assumes no sse4 instructions are allowed.
> 
> 
> > If you see a need for fixing the atm build file, I have no objections, but I think removing it is an acceptable solution as anyone who needs it can build for atom by manually editing the RTE_MACHINE type to "silvermont". [Though, I do think the comments in config/common_base could do with being updated, since it's not required any more to have a build directory for each machine type.]
> > 
> I don't really, I'm not in any way comitted to keeping atom support around. My
> bigger concern, and I understand I didn't really make this clear above, and I
> apologize for that, is that it doesn't make much sense to me to change the
> commit log to say "we're only removing support for some older atom systems", if
> the commit actually removes support for all of them (which is exactly what it
> does).  If your intent is to still support the atom systems that do execute
> sse4, then we should create machine definitions (using the existing way, or some
> to be determined method) to explicitly support them in a separate commit.  I
> wouldn't be opposed to doing it in this commit and adding your statement even,
> though I don't think I have access to any atom systems that support sse4 for the
> purposes of testing it out.
> 
Thanks for clarifying.

My concern here is the confusion around the term "atom". As you say, from
a gcc perspective -march=atom implies the bonnell microarchitecture,
i.e. code that just has SSE3, rather than referring to all atom cores of
all generations, which is what you are thinking of. If we allow "atom"
as a machine type, while turning on SSE4, will that not cause confusion?
I would think it is better to drop "atom" as an allowed machine type and
instead have users specify "silvermont" (or any later architecture
that's added to gcc) directly as the machine type for an atom platform.

If most folks figure it's not going to be a problem, I'm ok with
updating the atm build file to have either -msse4 or -march=silvermont.

/Bruce

Re: [dpdk-dev] [PATCH] atm: Remove machine definition

[Neil Horman]

On Thu, Aug 10, 2017 at 04:40:37PM +0100, Bruce Richardson wrote:
> On Thu, Aug 10, 2017 at 11:15:27AM -0400, Neil Horman wrote:
> > On Thu, Aug 10, 2017 at 02:04:00PM +0000, Richardson, Bruce wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > > Sent: Thursday, August 10, 2017 12:34 PM
> > > > To: Richardson, Bruce <bruce.richardson@intel.com>
> > > > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>
> > > > Subject: Re: [dpdk-dev] [PATCH] atm: Remove machine definition
> > > > 
> > > > On Thu, Aug 10, 2017 at 09:34:18AM +0100, Bruce Richardson wrote:
> > > > > On Wed, Aug 09, 2017 at 04:24:25PM -0400, Neil Horman wrote:
> > > > > > With the new updated requirement for SSE4.2, dpdk no longer supports
> > > > > > building on atom machines, as they only support up to SSE3.  Remove
> > > > > > the machine definition.
> > > > > >
> > > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> CC: Thomas
> > > > > > Monjalon <thomas@monjalon.net> --- mk/machine/atm/rte.vars.mk | 58
> > > > > > ---------------------------------------------- 1 file changed, 58
> > > > > > deletions(-) delete mode 100644 mk/machine/atm/rte.vars.mk
> > > > > >
> > > > > Yes, good catch, that should have been removed. However, I think the
> > > > > commit log should be updated to mention that it no longer supports
> > > > > "early" atom machines, or some similar phrase. Atom cores for the last
> > > > > number of years do support SSE4, see:
> > > > > https://en.wikipedia.org/wiki/Silvermont for example.
> > > > >
> > > > 
> > > > I don't really agree with that. If you want to make the claim that only
> > > > early atom machines aren't supported, the implication then is that later
> > > > atom machines are, and we should keep the machine type, and fix it to
> > > > build for those targeted machines (as it stands currently, the compiler
> > > > errors out on building atom because it only emits SSE3 instructions and
> > > > can't inline an SSE4 builtin). I'd be totally fine with fixing the atom
> > > > build (which I imageine amounts to passing -machine=atom -msse4 or
> > > > something simmilar to the build options.  Once we do that however, we
> > > > likely need a runtime check for sse4 support as well, so we don't just get
> > > > random SIGILL errors at run time.
> > > > 
> > > > Neil
> > > 
> > > For the runtime checks, "rte_eal_init()" already calls "rte_cpu_is_supported()" checks to ensure that all instruction sets that are built in are supported by the runtime platform.
> > > 
> > Ah, yes, then we have the runtime check, and can just consider fixing the
> > machine build.
> > 
> > > For fixing the atom build, I actually thought we were moving away from having special confirm files for the rte_machine type, and just passing the RTE_MACHINE build-time value directly through to the compiler as -march - especially since gcc and clang have started using the microarchitecture names directly in the march flag rather than the older names like core-i7-avx. In fact, "atom" is no longer listed in the manpage for gcc on my Fedora 26 system, only "bonnell", "silvermont" are present as atom architectures.
> > > 
> > Well, we may be, though I don't see any discussion of that online (at least
> > nothing obvious).  Though it should be noted that atom is still a supported
> > machine type, its just not documented, and the result is a broken build, as
> > -march atom assumes no sse4 instructions are allowed.
> > 
> > 
> > > If you see a need for fixing the atm build file, I have no objections, but I think removing it is an acceptable solution as anyone who needs it can build for atom by manually editing the RTE_MACHINE type to "silvermont". [Though, I do think the comments in config/common_base could do with being updated, since it's not required any more to have a build directory for each machine type.]
> > > 
> > I don't really, I'm not in any way comitted to keeping atom support around. My
> > bigger concern, and I understand I didn't really make this clear above, and I
> > apologize for that, is that it doesn't make much sense to me to change the
> > commit log to say "we're only removing support for some older atom systems", if
> > the commit actually removes support for all of them (which is exactly what it
> > does).  If your intent is to still support the atom systems that do execute
> > sse4, then we should create machine definitions (using the existing way, or some
> > to be determined method) to explicitly support them in a separate commit.  I
> > wouldn't be opposed to doing it in this commit and adding your statement even,
> > though I don't think I have access to any atom systems that support sse4 for the
> > purposes of testing it out.
> > 
> Thanks for clarifying.
> 
> My concern here is the confusion around the term "atom". As you say, from
> a gcc perspective -march=atom implies the bonnell microarchitecture,
> i.e. code that just has SSE3, rather than referring to all atom cores of
> all generations, which is what you are thinking of. If we allow "atom"
> as a machine type, while turning on SSE4, will that not cause confusion?
> I would think it is better to drop "atom" as an allowed machine type and
> instead have users specify "silvermont" (or any later architecture
> that's added to gcc) directly as the machine type for an atom platform.
> 
I think we're saying the same thing - i.e. allowing a machine specification of
atm is the wrong thing to do here, because -matom in gcc nomenclature referrs to
architectures which cannot execute sse4 instructions, which is apparently
required by dpdk now (I have a separate thread on trying to figure out where and
how that was announced, but thats neither here nor there, and not relevant to
this discussion).

> If most folks figure it's not going to be a problem, I'm ok with
> updating the atm build file to have either -msse4 or -march=silvermont.
> 
No, I think the right thing to do here given your description, is to remove the
atm support as described, and follow it up with a subsequent patch to create a
new machine type, silvermont, which correctly addresses the atom subset that
dpdk is intended to support

Neil

> /Bruce
>