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

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

A alternative change to this test case is as follows.
  

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.

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
  

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

Thanks
-Zhigang
  

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