DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Richardson, Bruce" <bruce.richardson@intel.com>
To: Neil Horman <nhorman@tuxdriver.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH] atm: Remove machine definition
Date: Thu, 10 Aug 2017 14:04:00 +0000	[thread overview]
Message-ID: <59AF69C657FD0841A61C55336867B5B0721B6138@IRSMSX103.ger.corp.intel.com> (raw)
In-Reply-To: <20170810113356.GA14414@hmswarspite.think-freely.org>



> -----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

  reply	other threads:[~2017-08-10 14:09 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 [this message]
2017-08-10 15:15       ` Neil Horman
2017-08-10 15:40         ` Bruce Richardson
2017-08-11 11:04           ` Neil Horman

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=59AF69C657FD0841A61C55336867B5B0721B6138@IRSMSX103.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=nhorman@tuxdriver.com \
    --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).