Re: [dpdk-dev] [PATCH 14/15] app/test: turn off cpu flag checks for tile architecture

[Neil Horman <nhorman@tuxdriver.com>]

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 <zlu@ezchip.com>
> >> >> Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
> >> >> ---
> >> >>  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
> 
>