diff mbox

[dpdk-dev,v4,06/26] eal-common: introduce a way to query cpu support

Message ID 20170225160309.31270-7-aconole@redhat.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Aaron Conole Feb. 25, 2017, 4:02 p.m. UTC 
  This adds a new API to check for the eal cpu versions.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_eal/common/eal_common_cpuflags.c          | 13 +++++++++++--
 lib/librte_eal/common/include/generic/rte_cpuflags.h |  9 +++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

Comments

Bruce Richardson Feb. 27, 2017, 1:48 p.m. UTC | #1 
On Sat, Feb 25, 2017 at 11:02:49AM -0500, Aaron Conole wrote:
> This adds a new API to check for the eal cpu versions.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  lib/librte_eal/common/eal_common_cpuflags.c          | 13 +++++++++++--
>  lib/librte_eal/common/include/generic/rte_cpuflags.h |  9 +++++++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
> index b5f76f7..2c2127b 100644
> --- a/lib/librte_eal/common/eal_common_cpuflags.c
> +++ b/lib/librte_eal/common/eal_common_cpuflags.c
> @@ -43,6 +43,13 @@
>  void
>  rte_cpu_check_supported(void)
>  {
> +	if (!rte_cpu_is_supported())
> +		exit(1);
> +}
> +
> +bool
> +rte_cpu_is_supported(void)
> +{
>  	/* This is generated at compile-time by the build system */
>  	static const enum rte_cpu_flag_t compile_time_flags[] = {
>  			RTE_COMPILE_TIME_CPUFLAGS
> @@ -57,14 +64,16 @@ rte_cpu_check_supported(void)
>  			fprintf(stderr,
>  				"ERROR: CPU feature flag lookup failed with error %d\n",
>  				ret);
> -			exit(1);
> +			return false;
>  		}
>  		if (!ret) {
>  			fprintf(stderr,
>  			        "ERROR: This system does not support \"%s\".\n"
>  			        "Please check that RTE_MACHINE is set correctly.\n",
>  			        rte_cpu_get_flag_name(compile_time_flags[i]));
> -			exit(1);
> +			return false;
>  		}
>  	}
> +
> +	return true;
>  }
> diff --git a/lib/librte_eal/common/include/generic/rte_cpuflags.h b/lib/librte_eal/common/include/generic/rte_cpuflags.h
> index 71321f3..e4342ad 100644
> --- a/lib/librte_eal/common/include/generic/rte_cpuflags.h
> +++ b/lib/librte_eal/common/include/generic/rte_cpuflags.h
> @@ -40,6 +40,7 @@
>   */
>  
>  #include <errno.h>
> +#include <stdbool.h>
>  

The addition of this include is causing all sorts of compilation errors
inside the PMDs, as many of them seem to be defining their own bools
types. :-(

For safety sake, probably best to have the function return int rather
than bool.

/Bruce
Aaron Conole Feb. 27, 2017, 2:33 p.m. UTC | #2 
Bruce Richardson <bruce.richardson@intel.com> writes:

> On Sat, Feb 25, 2017 at 11:02:49AM -0500, Aaron Conole wrote:
>> This adds a new API to check for the eal cpu versions.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  lib/librte_eal/common/eal_common_cpuflags.c          | 13 +++++++++++--
>>  lib/librte_eal/common/include/generic/rte_cpuflags.h |  9 +++++++++
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>> 
>> diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
>> index b5f76f7..2c2127b 100644
>> --- a/lib/librte_eal/common/eal_common_cpuflags.c
>> +++ b/lib/librte_eal/common/eal_common_cpuflags.c
>> @@ -43,6 +43,13 @@
>>  void
>>  rte_cpu_check_supported(void)
>>  {
>> +	if (!rte_cpu_is_supported())
>> +		exit(1);
>> +}
>> +
>> +bool
>> +rte_cpu_is_supported(void)
>> +{
>>  	/* This is generated at compile-time by the build system */
>>  	static const enum rte_cpu_flag_t compile_time_flags[] = {
>>  			RTE_COMPILE_TIME_CPUFLAGS
>> @@ -57,14 +64,16 @@ rte_cpu_check_supported(void)
>>  			fprintf(stderr,
>>  				"ERROR: CPU feature flag lookup failed with error %d\n",
>>  				ret);
>> -			exit(1);
>> +			return false;
>>  		}
>>  		if (!ret) {
>>  			fprintf(stderr,
>>  			        "ERROR: This system does not support \"%s\".\n"
>>  			        "Please check that RTE_MACHINE is set correctly.\n",
>>  			        rte_cpu_get_flag_name(compile_time_flags[i]));
>> -			exit(1);
>> +			return false;
>>  		}
>>  	}
>> +
>> +	return true;
>>  }
>> diff --git a/lib/librte_eal/common/include/generic/rte_cpuflags.h b/lib/librte_eal/common/include/generic/rte_cpuflags.h
>> index 71321f3..e4342ad 100644
>> --- a/lib/librte_eal/common/include/generic/rte_cpuflags.h
>> +++ b/lib/librte_eal/common/include/generic/rte_cpuflags.h
>> @@ -40,6 +40,7 @@
>>   */
>>  
>>  #include <errno.h>
>> +#include <stdbool.h>
>>  
>
> The addition of this include is causing all sorts of compilation errors
> inside the PMDs, as many of them seem to be defining their own bools
> types. :-(
>
> For safety sake, probably best to have the function return int rather
> than bool.

Will do - I never saw the issue, but perhaps I was excluding the PMDs
in question.

Thanks for the review, Bruce!
Bruce Richardson Feb. 27, 2017, 3:11 p.m. UTC | #3 
On Mon, Feb 27, 2017 at 09:33:19AM -0500, Aaron Conole wrote:
> Bruce Richardson <bruce.richardson@intel.com> writes:
> 
> > On Sat, Feb 25, 2017 at 11:02:49AM -0500, Aaron Conole wrote:
> >> This adds a new API to check for the eal cpu versions.
> >> 
> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> ---
> >>  lib/librte_eal/common/eal_common_cpuflags.c          | 13 +++++++++++--
> >>  lib/librte_eal/common/include/generic/rte_cpuflags.h |  9 +++++++++
> >>  2 files changed, 20 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
> >> index b5f76f7..2c2127b 100644
> >> --- a/lib/librte_eal/common/eal_common_cpuflags.c
> >> +++ b/lib/librte_eal/common/eal_common_cpuflags.c
> >> @@ -43,6 +43,13 @@
> >>  void
> >>  rte_cpu_check_supported(void)
> >>  {
> >> +	if (!rte_cpu_is_supported())
> >> +		exit(1);
> >> +}
> >> +
> >> +bool
> >> +rte_cpu_is_supported(void)
> >> +{
> >>  	/* This is generated at compile-time by the build system */
> >>  	static const enum rte_cpu_flag_t compile_time_flags[] = {
> >>  			RTE_COMPILE_TIME_CPUFLAGS
> >> @@ -57,14 +64,16 @@ rte_cpu_check_supported(void)
> >>  			fprintf(stderr,
> >>  				"ERROR: CPU feature flag lookup failed with error %d\n",
> >>  				ret);
> >> -			exit(1);
> >> +			return false;
> >>  		}
> >>  		if (!ret) {
> >>  			fprintf(stderr,
> >>  			        "ERROR: This system does not support \"%s\".\n"
> >>  			        "Please check that RTE_MACHINE is set correctly.\n",
> >>  			        rte_cpu_get_flag_name(compile_time_flags[i]));
> >> -			exit(1);
> >> +			return false;
> >>  		}
> >>  	}
> >> +
> >> +	return true;
> >>  }
> >> diff --git a/lib/librte_eal/common/include/generic/rte_cpuflags.h b/lib/librte_eal/common/include/generic/rte_cpuflags.h
> >> index 71321f3..e4342ad 100644
> >> --- a/lib/librte_eal/common/include/generic/rte_cpuflags.h
> >> +++ b/lib/librte_eal/common/include/generic/rte_cpuflags.h
> >> @@ -40,6 +40,7 @@
> >>   */
> >>  
> >>  #include <errno.h>
> >> +#include <stdbool.h>
> >>  
> >
> > The addition of this include is causing all sorts of compilation errors
> > inside the PMDs, as many of them seem to be defining their own bools
> > types. :-(
> >
> > For safety sake, probably best to have the function return int rather
> > than bool.
> 
> Will do - I never saw the issue, but perhaps I was excluding the PMDs
> in question.
> 
> Thanks for the review, Bruce!

No problem.
FYI, the drivers I saw the errors in with this patch are:
* qede
* i40e
* ixgbe
* cxgbe

Regards,
/Bruce
diff mbox

Patch

diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
index b5f76f7..2c2127b 100644
--- a/lib/librte_eal/common/eal_common_cpuflags.c
+++ b/lib/librte_eal/common/eal_common_cpuflags.c
@@ -43,6 +43,13 @@ 
 void
 rte_cpu_check_supported(void)
 {
+	if (!rte_cpu_is_supported())
+		exit(1);
+}
+
+bool
+rte_cpu_is_supported(void)
+{
 	/* This is generated at compile-time by the build system */
 	static const enum rte_cpu_flag_t compile_time_flags[] = {
 			RTE_COMPILE_TIME_CPUFLAGS
@@ -57,14 +64,16 @@  rte_cpu_check_supported(void)
 			fprintf(stderr,
 				"ERROR: CPU feature flag lookup failed with error %d\n",
 				ret);
-			exit(1);
+			return false;
 		}
 		if (!ret) {
 			fprintf(stderr,
 			        "ERROR: This system does not support \"%s\".\n"
 			        "Please check that RTE_MACHINE is set correctly.\n",
 			        rte_cpu_get_flag_name(compile_time_flags[i]));
-			exit(1);
+			return false;
 		}
 	}
+
+	return true;
 }
diff --git a/lib/librte_eal/common/include/generic/rte_cpuflags.h b/lib/librte_eal/common/include/generic/rte_cpuflags.h
index 71321f3..e4342ad 100644
--- a/lib/librte_eal/common/include/generic/rte_cpuflags.h
+++ b/lib/librte_eal/common/include/generic/rte_cpuflags.h
@@ -40,6 +40,7 @@ 
  */
 
 #include <errno.h>
+#include <stdbool.h>
 
 /**
  * Enumeration of all CPU features supported
@@ -82,4 +83,12 @@  rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature);
 void
 rte_cpu_check_supported(void);
 
+/**
+ * This function checks that the currently used CPU supports the CPU features
+ * that were specified at compile time. It is called automatically within the
+ * EAL, so does not need to be used by applications.  This version returns a
+ * result so that decisions may be made (for instance, graceful shutdowns).
+ */
+bool
+rte_cpu_is_supported(void);
 #endif /* _RTE_CPUFLAGS_H_ */