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 CAD1A8086 for ; Fri, 12 Dec 2014 14:07:31 +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 1XzPwN-0007d4-UB; Fri, 12 Dec 2014 08:07:30 -0500 Date: Fri, 12 Dec 2014 08:07:22 -0500 From: Neil Horman To: Tony Lu Message-ID: <20141212130722.GC14102@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> <20141211133919.GB28213@hmsreliant.think-freely.org> <003701d015e3$21f50690$65df13b0$@com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <003701d015e3$21f50690$65df13b0$@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: Fri, 12 Dec 2014 13:07:32 -0000 On Fri, Dec 12, 2014 at 04:10:21PM +0800, Tony Lu wrote: > >-----Original Message----- > >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman > >Sent: Thursday, December 11, 2014 9:39 PM > >To: Tony Lu > >Cc: dev@dpdk.org > >Subject: Re: [dpdk-dev] [PATCH 14/15] app/test: turn off cpu flag checks > for tile > >architecture > > > >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. > > Agree, Make sense. > > >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. > > Yes, in order not to break ACL or other libs/apps, we need to make the flags > of all > supported arches accessible. But I don't feel as strongly to create a > [arch][flag] array, > since checking if the specified flag is supported is at runtime, so we can > not assign it in > a predefine array according to its arch. For example, some old X86 processor > does not > support SSE3. > > Instead I prefer a one dimensional arch-specific [flag] array which contains > all the flags > of all supported arches, and we mark the flags that do not belong to the > current arch > as "not available". > > To implement this, we need to move the enum rte_cpu_flag_t from > arch-specific > rte_cpuflags.c to the generic one, and combine them as one enumeration. > > ACL rte_acl_init() itself has a bug that it should check the return value of > rte_cpu_get_flag_enabled() if it is "1", but not "!0", as it may return > "-EFAULT". > Thats all fine with me. We can debate the relative merits of implementation when its available. Its getting the interface right that I think is currently the priority. Neil > Thanks > -Zhigang > >