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 EFC7E6A95 for ; Thu, 11 Dec 2014 14:39:59 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1Xz3y3-0004vV-0U; Thu, 11 Dec 2014 08:39:53 -0500 Date: Thu, 11 Dec 2014 08:39:19 -0500 From: Neil Horman To: Tony Lu Message-ID: <20141211133919.GB28213@hmsreliant.think-freely.org> References: <1418029178-25162-1-git-send-email-zlu@ezchip.com> <1418029178-25162-15-git-send-email-zlu@ezchip.com> <20141209150321.GC28871@hmsreliant.think-freely.org> <000901d014fd$106f85d0$314e9170$@com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <000901d014fd$106f85d0$314e9170$@com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 14/15] app/test: turn off cpu flag checks for tile architecture X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Dec 2014 13:40:00 -0000 On Thu, Dec 11, 2014 at 12:43:36PM +0800, Tony Lu wrote: > >-----Original Message----- > >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman > >Sent: Tuesday, December 09, 2014 11:03 PM > >To: Zhigang Lu > >Cc: dev@dpdk.org > >Subject: Re: [dpdk-dev] [PATCH 14/15] app/test: turn off cpu flag checks > for tile > >architecture > > > >On Mon, Dec 08, 2014 at 04:59:37PM +0800, Zhigang Lu wrote: > >> Tile processor doesn't have CPU flag hardware registers, so this patch > >> turns off cpu flag checks for tile. > >> > >> Signed-off-by: Zhigang Lu > >> Signed-off-by: Cyril Chemparathy > >> --- > >> app/test/test_cpuflags.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/app/test/test_cpuflags.c b/app/test/test_cpuflags.c index > >> 5aeba5d..da93af5 100644 > >> --- a/app/test/test_cpuflags.c > >> +++ b/app/test/test_cpuflags.c > >> @@ -113,7 +113,7 @@ test_cpuflags(void) > >> > >> printf("Check for ICACHE_SNOOP:\t\t"); > >> CHECK_FOR_FLAG(RTE_CPUFLAG_ICACHE_SNOOP); > >> -#else > >> +#elif !defined(RTE_ARCH_TILE) > >> printf("Check for SSE:\t\t"); > >> CHECK_FOR_FLAG(RTE_CPUFLAG_SSE); > >> > >Please stop this. It doesn't make sense for a library that supports > multiple > >arches, we need some way to generically test for flags that doesn't involve > >forcing applications to do ton's of ifdeffing. Perhaps > rte_cpu_get_flag_enabled > >needs to do a flag table lookup based on the detected arch at run time, and > >return the appropriate response. In the case of tile, it can just be an > empty > >table, so 0 is always returned. But making an application responsible for > doing > >arch checks is a guarantee to write non-portable applications > > > >Neil > > > > Thanks for taking a look at this. > This change just follows what PPC did in commit 9ae15538. The root cause is Yes, and I objected to it there as well: http://dpdk.org/ml/archives/dev/2014-November/008628.html To which the response was effectively "Sure, we'll do that later". You're effectively making the same argument. If no one ever steps up to change the interface when adding a new arch, it will never get done, and we'll have a fragmented cpuflag test mechanism that creates completely non-portable code accross arches. > that > the test_cpuflags.c explicitly tests X86-specific CPU flags, so we might > need to revise > this test case to make it architecture-independent. > Exactly what I said in my email to the powerpc people. If you're going to add a new arch, and a given interface doesn't support doing so, please try to re-design the interface to make it more friendly, otherwise we'll be left with unmaintainable code. Thinking about it, you probably don't even need to change the api call to do this. You just need to create a unified map for all flags of all supported arches, that is to say a two dimensional array with the indicies [arch][flag] where the stored value is the arch specific data to help determine if the feature is supported, or a universal "not supported" flag. Neil