From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id EE3A4137C for ; Fri, 11 Aug 2017 13:05:04 +0200 (CEST) Received: from [2606:a000:111b:4246:ce17:ba0d:7461:5cd1] (helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1dg7kF-0002ja-RX; Fri, 11 Aug 2017 07:05:01 -0400 Date: Fri, 11 Aug 2017 07:04:46 -0400 From: Neil Horman To: Bruce Richardson Cc: "dev@dpdk.org" , Thomas Monjalon Message-ID: <20170811110446.GA29585@hmswarspite.think-freely.org> References: <20170809202425.1356-1-nhorman@tuxdriver.com> <20170810083418.GA53940@bricha3-MOBL3.ger.corp.intel.com> <20170810113356.GA14414@hmswarspite.think-freely.org> <59AF69C657FD0841A61C55336867B5B0721B6138@IRSMSX103.ger.corp.intel.com> <20170810151527.GD14414@hmswarspite.think-freely.org> <20170810154036.GA64220@bricha3-MOBL3.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170810154036.GA64220@bricha3-MOBL3.ger.corp.intel.com> User-Agent: Mutt/1.8.3 (2017-05-23) X-Spam-Score: -2.9 (--) X-Spam-Status: No Subject: Re: [dpdk-dev] [PATCH] atm: Remove machine definition X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Aug 2017 11:05:05 -0000 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 > > > > Cc: dev@dpdk.org; Thomas Monjalon > > > > 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 CC: Thomas > > > > > > Monjalon --- 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 >