From: Neil Horman <nhorman@tuxdriver.com>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH] atm: Remove machine definition
Date: Fri, 11 Aug 2017 07:04:46 -0400 [thread overview]
Message-ID: <20170811110446.GA29585@hmswarspite.think-freely.org> (raw)
In-Reply-To: <20170810154036.GA64220@bricha3-MOBL3.ger.corp.intel.com>
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
>
prev parent reply other threads:[~2017-08-11 11:05 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-09 20:24 Neil Horman
2017-08-10 8:34 ` Bruce Richardson
2017-08-10 11:33 ` Neil Horman
2017-08-10 14:04 ` Richardson, Bruce
2017-08-10 15:15 ` Neil Horman
2017-08-10 15:40 ` Bruce Richardson
2017-08-11 11:04 ` Neil Horman [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170811110446.GA29585@hmswarspite.think-freely.org \
--to=nhorman@tuxdriver.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).