diff mbox series

[v3] build: use generic march on arm64 when using 'default' machine

Message ID 20190107141130.24360-1-bluca@debian.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v3] build: use generic march on arm64 when using 'default' machine |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Luca Boccassi Jan. 7, 2019, 2:11 p.m. UTC 
  When building for generic distribution we need a stable baseline
architecture, or depending on the build worker the result will vary.

Force the default flags if the user explicitly sets machine=default
at configuration time.

Fixes: b1d48c41189a ("build: support ARM with meson")
Cc: stable@dpdk.org

Signed-off-by: Luca Boccassi <bluca@debian.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
v2: fix typo in commit message, remove variable used only once
v3: put back temporary variable, as "machine" gets overwritten
    by the function and loses the original value before we need
    it.

 config/arm/meson.build | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Luca Boccassi Jan. 11, 2019, 10:45 a.m. UTC | #1 
On Mon, 2019-01-07 at 14:11 +0000, Luca Boccassi wrote:
> When building for generic distribution we need a stable baseline
> architecture, or depending on the build worker the result will vary.
> 
> Force the default flags if the user explicitly sets machine=default
> at configuration time.
> 
> Fixes: b1d48c41189a ("build: support ARM with meson")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> v2: fix typo in commit message, remove variable used only once
> v3: put back temporary variable, as "machine" gets overwritten
>     by the function and loses the original value before we need
>     it.
> 
>  config/arm/meson.build | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/config/arm/meson.build b/config/arm/meson.build
> index dae55d6b2..614139534 100644
> --- a/config/arm/meson.build
> +++ b/config/arm/meson.build
> @@ -6,6 +6,7 @@
>  march_opt = '-march=@0@'.format(machine)
>  
>  arm_force_native_march = false
> +arm_force_default_march = (machine == 'default')
>  
>  machine_args_generic = [
>  	['default', ['-march=armv8-a+crc+crypto']],
> @@ -105,7 +106,11 @@ else
>  	cmd_generic = ['generic', '', '', 'default', '']
>  	cmd_output = cmd_generic # Set generic by default
>  	machine_args = [] # Clear previous machine args
> -	if not meson.is_cross_build()
> +	if arm_force_default_march and not meson.is_cross_build()
> +		machine = impl_generic
> +		cmd_output = cmd_generic
> +		impl_pn = 'default'
> +	elif not meson.is_cross_build()
>  		# The script returns ['Implementer', 'Variant',
> 'Architecture',
>  		# 'Primary Part number', 'Revision']
>  		detect_vendor = find_program(join_paths(

Any chance for a quick review from the arm maintainers/devs? Thanks!
Jerin Jacob Kollanukkaran Jan. 14, 2019, 8:09 a.m. UTC | #2 
On Fri, 2019-01-11 at 10:45 +0000, Luca Boccassi wrote:
> External Email
> 
> -------------------------------------------------------------------
> ---
> On Mon, 2019-01-07 at 14:11 +0000, Luca Boccassi wrote:
> > When building for generic distribution we need a stable baseline
> > architecture, or depending on the build worker the result will
> > vary.
> > 
> > Force the default flags if the user explicitly sets machine=default
> > at configuration time.
> > 
> > Fixes: b1d48c41189a ("build: support ARM with meson")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> > v2: fix typo in commit message, remove variable used only once
> > v3: put back temporary variable, as "machine" gets overwritten
> >     by the function and loses the original value before we need
> >     it.
> > 
> >  config/arm/meson.build | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/config/arm/meson.build b/config/arm/meson.build
> > index dae55d6b2..614139534 100644
> > --- a/config/arm/meson.build
> > +++ b/config/arm/meson.build
> > @@ -6,6 +6,7 @@
> >  march_opt = '-march=@0@'.format(machine)
> >  
> >  arm_force_native_march = false
> > +arm_force_default_march = (machine == 'default')
> >  
> >  machine_args_generic = [
> >  	['default', ['-march=armv8-a+crc+crypto']],
> > @@ -105,7 +106,11 @@ else
> >  	cmd_generic = ['generic', '', '', 'default', '']
> >  	cmd_output = cmd_generic # Set generic by default
> >  	machine_args = [] # Clear previous machine args
> > -	if not meson.is_cross_build()
> > +	if arm_force_default_march and not meson.is_cross_build()
> > +		machine = impl_generic
> > +		cmd_output = cmd_generic
> > +		impl_pn = 'default'
> > +	elif not meson.is_cross_build()
> >  		# The script returns ['Implementer', 'Variant',
> > 'Architecture',
> >  		# 'Primary Part number', 'Revision']
> >  		detect_vendor = find_program(join_paths(
> 
> Any chance for a quick review from the arm maintainers/devs? Thanks!

Looks good.

Could you please document the procedure to build generic image some
where in the DPDK documentation.

I guess the procedure will be same for x86 and arm64. Right?
Luca Boccassi Jan. 14, 2019, 9:58 a.m. UTC | #3 
On Mon, 2019-01-14 at 08:09 +0000, Jerin Jacob Kollanukkaran wrote:
> On Fri, 2019-01-11 at 10:45 +0000, Luca Boccassi wrote:
> > External Email
> > 
> > -------------------------------------------------------------------
> > ---
> > On Mon, 2019-01-07 at 14:11 +0000, Luca Boccassi wrote:
> > > When building for generic distribution we need a stable baseline
> > > architecture, or depending on the build worker the result will
> > > vary.
> > > 
> > > Force the default flags if the user explicitly sets
> > > machine=default
> > > at configuration time.
> > > 
> > > Fixes: b1d48c41189a ("build: support ARM with meson")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > > v2: fix typo in commit message, remove variable used only once
> > > v3: put back temporary variable, as "machine" gets overwritten
> > >     by the function and loses the original value before we need
> > >     it.
> > > 
> > >  config/arm/meson.build | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/config/arm/meson.build b/config/arm/meson.build
> > > index dae55d6b2..614139534 100644
> > > --- a/config/arm/meson.build
> > > +++ b/config/arm/meson.build
> > > @@ -6,6 +6,7 @@
> > >  march_opt = '-march=@0@'.format(machine)
> > >  
> > >  arm_force_native_march = false
> > > +arm_force_default_march = (machine == 'default')
> > >  
> > >  machine_args_generic = [
> > >  	['default', ['-march=armv8-a+crc+crypto']],
> > > @@ -105,7 +106,11 @@ else
> > >  	cmd_generic = ['generic', '', '', 'default', '']
> > >  	cmd_output = cmd_generic # Set generic by default
> > >  	machine_args = [] # Clear previous machine args
> > > -	if not meson.is_cross_build()
> > > +	if arm_force_default_march and not
> > > meson.is_cross_build()
> > > +		machine = impl_generic
> > > +		cmd_output = cmd_generic
> > > +		impl_pn = 'default'
> > > +	elif not meson.is_cross_build()
> > >  		# The script returns ['Implementer', 'Variant',
> > > 'Architecture',
> > >  		# 'Primary Part number', 'Revision']
> > >  		detect_vendor = find_program(join_paths(
> > 
> > Any chance for a quick review from the arm maintainers/devs?
> > Thanks!
> 
> Looks good.
> 
> Could you please document the procedure to build generic image some
> where in the DPDK documentation.
> 
> I guess the procedure will be same for x86 and arm64. Right?

The "machine" meson option is already described in meson_options.txt,
is that enough?
Jerin Jacob Kollanukkaran Jan. 14, 2019, 10:35 a.m. UTC | #4 
On Mon, 2019-01-14 at 09:58 +0000, Luca Boccassi wrote:
> On Mon, 2019-01-14 at 08:09 +0000, Jerin Jacob Kollanukkaran wrote:
> > On Fri, 2019-01-11 at 10:45 +0000, Luca Boccassi wrote:
> > > External Email
> > > 
> > > ---------------------------------------------------------------
> > > ----
> > > ---
> > > On Mon, 2019-01-07 at 14:11 +0000, Luca Boccassi wrote:
> > > > When building for generic distribution we need a stable
> > > > baseline
> > > > architecture, or depending on the build worker the result will
> > > > vary.
> > > > 
> > > > Force the default flags if the user explicitly sets
> > > > machine=default
> > > > at configuration time.
> > > > 
> > > > Fixes: b1d48c41189a ("build: support ARM with meson")
> > > > Cc: stable@dpdk.org
> > > > 
> > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > ---
> > > > v2: fix typo in commit message, remove variable used only once
> > > > v3: put back temporary variable, as "machine" gets overwritten
> > > >     by the function and loses the original value before we need
> > > >     it.
> > > > 
> > > >  config/arm/meson.build | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/config/arm/meson.build b/config/arm/meson.build
> > > > index dae55d6b2..614139534 100644
> > > > --- a/config/arm/meson.build
> > > > +++ b/config/arm/meson.build
> > > > @@ -6,6 +6,7 @@
> > > >  march_opt = '-march=@0@'.format(machine)
> > > >  
> > > >  arm_force_native_march = false
> > > > +arm_force_default_march = (machine == 'default')
> > > >  
> > > >  machine_args_generic = [
> > > >  	['default', ['-march=armv8-a+crc+crypto']],
> > > > @@ -105,7 +106,11 @@ else
> > > >  	cmd_generic = ['generic', '', '', 'default', '']
> > > >  	cmd_output = cmd_generic # Set generic by default
> > > >  	machine_args = [] # Clear previous machine args
> > > > -	if not meson.is_cross_build()
> > > > +	if arm_force_default_march and not
> > > > meson.is_cross_build()
> > > > +		machine = impl_generic
> > > > +		cmd_output = cmd_generic
> > > > +		impl_pn = 'default'
> > > > +	elif not meson.is_cross_build()
> > > >  		# The script returns ['Implementer', 'Variant',
> > > > 'Architecture',
> > > >  		# 'Primary Part number', 'Revision']
> > > >  		detect_vendor = find_program(join_paths(
> > > 
> > > Any chance for a quick review from the arm maintainers/devs?
> > > Thanks!
> > 
> > Looks good.
> > 
> > Could you please document the procedure to build generic image some
> > where in the DPDK documentation.
> > 
> > I guess the procedure will be same for x86 and arm64. Right?
> 
> The "machine" meson option is already described in meson_options.txt,
> is that enough?

I meant, Documenting the steps to create the generic image i.e -
Dmachine=default scheme to use generic build for meson based
distribution build.

Is it same for x86 too?

meson configure -Dmachine=default
meson build
cd build
ninja
ninja install


>
Luca Boccassi Jan. 14, 2019, 11:07 a.m. UTC | #5 
On Mon, 2019-01-14 at 10:35 +0000, Jerin Jacob Kollanukkaran wrote:
> On Mon, 2019-01-14 at 09:58 +0000, Luca Boccassi wrote:
> > On Mon, 2019-01-14 at 08:09 +0000, Jerin Jacob Kollanukkaran wrote:
> > > On Fri, 2019-01-11 at 10:45 +0000, Luca Boccassi wrote:
> > > > External Email
> > > > 
> > > > ---------------------------------------------------------------
> > > > ----
> > > > ---
> > > > On Mon, 2019-01-07 at 14:11 +0000, Luca Boccassi wrote:
> > > > > When building for generic distribution we need a stable
> > > > > baseline
> > > > > architecture, or depending on the build worker the result
> > > > > will
> > > > > vary.
> > > > > 
> > > > > Force the default flags if the user explicitly sets
> > > > > machine=default
> > > > > at configuration time.
> > > > > 
> > > > > Fixes: b1d48c41189a ("build: support ARM with meson")
> > > > > Cc: stable@dpdk.org
> > > > > 
> > > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > > ---
> > > > > v2: fix typo in commit message, remove variable used only
> > > > > once
> > > > > v3: put back temporary variable, as "machine" gets
> > > > > overwritten
> > > > >     by the function and loses the original value before we
> > > > > need
> > > > >     it.
> > > > > 
> > > > >  config/arm/meson.build | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/config/arm/meson.build b/config/arm/meson.build
> > > > > index dae55d6b2..614139534 100644
> > > > > --- a/config/arm/meson.build
> > > > > +++ b/config/arm/meson.build
> > > > > @@ -6,6 +6,7 @@
> > > > >  march_opt = '-march=@0@'.format(machine)
> > > > >  
> > > > >  arm_force_native_march = false
> > > > > +arm_force_default_march = (machine == 'default')
> > > > >  
> > > > >  machine_args_generic = [
> > > > >  	['default', ['-march=armv8-a+crc+crypto']],
> > > > > @@ -105,7 +106,11 @@ else
> > > > >  	cmd_generic = ['generic', '', '', 'default', '']
> > > > >  	cmd_output = cmd_generic # Set generic by default
> > > > >  	machine_args = [] # Clear previous machine args
> > > > > -	if not meson.is_cross_build()
> > > > > +	if arm_force_default_march and not
> > > > > meson.is_cross_build()
> > > > > +		machine = impl_generic
> > > > > +		cmd_output = cmd_generic
> > > > > +		impl_pn = 'default'
> > > > > +	elif not meson.is_cross_build()
> > > > >  		# The script returns ['Implementer',
> > > > > 'Variant',
> > > > > 'Architecture',
> > > > >  		# 'Primary Part number', 'Revision']
> > > > >  		detect_vendor = find_program(join_paths(
> > > > 
> > > > Any chance for a quick review from the arm maintainers/devs?
> > > > Thanks!
> > > 
> > > Looks good.
> > > 
> > > Could you please document the procedure to build generic image
> > > some
> > > where in the DPDK documentation.
> > > 
> > > I guess the procedure will be same for x86 and arm64. Right?
> > 
> > The "machine" meson option is already described in
> > meson_options.txt,
> > is that enough?
> 
> I meant, Documenting the steps to create the generic image i.e -
> Dmachine=default scheme to use generic build for meson based
> distribution build.
> 
> Is it same for x86 too?
> 
> meson configure -Dmachine=default
> meson build
> cd build
> ninja
> ninja install

Yes that's correct, for any architecture "-Dmachine=default" will pick
a stable baseline.
I can send a separate patch to update the doc with that command.
Luca Boccassi Feb. 27, 2019, 12:15 p.m. UTC | #6 
On Mon, 2019-01-07 at 14:11 +0000, Luca Boccassi wrote:
> When building for generic distribution we need a stable baseline
> architecture, or depending on the build worker the result will vary.
> 
> Force the default flags if the user explicitly sets machine=default
> at configuration time.
> 
> Fixes: b1d48c41189a ("build: support ARM with meson")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> v2: fix typo in commit message, remove variable used only once
> v3: put back temporary variable, as "machine" gets overwritten
>     by the function and loses the original value before we need
>     it.
> 
>  config/arm/meson.build | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/config/arm/meson.build b/config/arm/meson.build
> index dae55d6b2..614139534 100644
> --- a/config/arm/meson.build
> +++ b/config/arm/meson.build
> @@ -6,6 +6,7 @@
>  march_opt = '-march=@0@'.format(machine)
>  
>  arm_force_native_march = false
> +arm_force_default_march = (machine == 'default')
>  
>  machine_args_generic = [
>  	['default', ['-march=armv8-a+crc+crypto']],
> @@ -105,7 +106,11 @@ else
>  	cmd_generic = ['generic', '', '', 'default', '']
>  	cmd_output = cmd_generic # Set generic by default
>  	machine_args = [] # Clear previous machine args
> -	if not meson.is_cross_build()
> +	if arm_force_default_march and not meson.is_cross_build()
> +		machine = impl_generic
> +		cmd_output = cmd_generic
> +		impl_pn = 'default'
> +	elif not meson.is_cross_build()
>  		# The script returns ['Implementer', 'Variant',
> 'Architecture',
>  		# 'Primary Part number', 'Revision']
>  		detect_vendor = find_program(join_paths(

Ping - anything else I can do for this patch? It's necessary for distro
native builds, at least in Debian/Ubuntu. Thanks!
Pavan Nikhilesh Bhagavatula March 20, 2019, 5:01 a.m. UTC | #7 
On Mon, 2019-01-07 at 14:11 +0000, Luca Boccassi wrote:
> When building for generic distribution we need a stable baseline
> architecture, or depending on the build worker the result will vary.
> 
> Force the default flags if the user explicitly sets machine=default
> at configuration time.
> 
> Fixes: b1d48c41189a ("build: support ARM with meson")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> v2: fix typo in commit message, remove variable used only once
> v3: put back temporary variable, as "machine" gets overwritten
>     by the function and loses the original value before we need
>     it.
> 
>  config/arm/meson.build | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/config/arm/meson.build b/config/arm/meson.build
> index dae55d6b2..614139534 100644
> --- a/config/arm/meson.build
> +++ b/config/arm/meson.build
> @@ -6,6 +6,7 @@
>  march_opt = '-march=@0@'.format(machine)
>  
>  arm_force_native_march = false
> +arm_force_default_march = (machine == 'default')
>  
>  machine_args_generic = [
>  	['default', ['-march=armv8-a+crc+crypto']],
> @@ -105,7 +106,11 @@ else
>  	cmd_generic = ['generic', '', '', 'default', '']
>  	cmd_output = cmd_generic # Set generic by default
^
>  	machine_args = [] # Clear previous machine args
> -	if not meson.is_cross_build()
> +	if arm_force_default_march and not meson.is_cross_build()
> +		machine = impl_generic
> +		cmd_output = cmd_generic

Isn't this assignment redundant as it is already done above?

Acked-by: Pavan Nikhilesh <pbhagavatula@marvell.com>

> +		impl_pn = 'default'
> +	elif not meson.is_cross_build()
>  		# The script returns ['Implementer', 'Variant',
> 'Architecture',
>  		# 'Primary Part number', 'Revision']
>  		detect_vendor = find_program(join_paths(
Luca Boccassi March 20, 2019, 1:18 p.m. UTC | #8 
On Wed, 2019-03-20 at 05:01 +0000, Pavan Nikhilesh Bhagavatula wrote:
> On Mon, 2019-01-07 at 14:11 +0000, Luca Boccassi wrote:
> > When building for generic distribution we need a stable baseline
> > architecture, or depending on the build worker the result will
> > vary.
> > 
> > Force the default flags if the user explicitly sets machine=default
> > at configuration time.
> > 
> > Fixes: b1d48c41189a ("build: support ARM with meson")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> > v2: fix typo in commit message, remove variable used only once
> > v3: put back temporary variable, as "machine" gets overwritten
> >     by the function and loses the original value before we need
> >     it.
> > 
> >  config/arm/meson.build | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/config/arm/meson.build b/config/arm/meson.build
> > index dae55d6b2..614139534 100644
> > --- a/config/arm/meson.build
> > +++ b/config/arm/meson.build
> > @@ -6,6 +6,7 @@
> >  march_opt = '-march=@0@'.format(machine)
> >  
> >  arm_force_native_march = false
> > +arm_force_default_march = (machine == 'default')
> >  
> >  machine_args_generic = [
> >  	['default', ['-march=armv8-a+crc+crypto']],
> > @@ -105,7 +106,11 @@ else
> >  	cmd_generic = ['generic', '', '', 'default', '']
> >  	cmd_output = cmd_generic # Set generic by default
> ^
> >  	machine_args = [] # Clear previous machine args
> > -	if not meson.is_cross_build()
> > +	if arm_force_default_march and not meson.is_cross_build()
> > +		machine = impl_generic
> > +		cmd_output = cmd_generic
> 
> Isn't this assignment redundant as it is already done above?
> 
> Acked-by: Pavan Nikhilesh <pbhagavatula@marvell.com>

Removed in v4, thanks.
diff mbox series

Patch

diff --git a/config/arm/meson.build b/config/arm/meson.build
index dae55d6b2..614139534 100644
--- a/config/arm/meson.build
+++ b/config/arm/meson.build
@@ -6,6 +6,7 @@ 
 march_opt = '-march=@0@'.format(machine)
 
 arm_force_native_march = false
+arm_force_default_march = (machine == 'default')
 
 machine_args_generic = [
 	['default', ['-march=armv8-a+crc+crypto']],
@@ -105,7 +106,11 @@  else
 	cmd_generic = ['generic', '', '', 'default', '']
 	cmd_output = cmd_generic # Set generic by default
 	machine_args = [] # Clear previous machine args
-	if not meson.is_cross_build()
+	if arm_force_default_march and not meson.is_cross_build()
+		machine = impl_generic
+		cmd_output = cmd_generic
+		impl_pn = 'default'
+	elif not meson.is_cross_build()
 		# The script returns ['Implementer', 'Variant', 'Architecture',
 		# 'Primary Part number', 'Revision']
 		detect_vendor = find_program(join_paths(