DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: Andrzej Ostruszka <amo@semihalf.com>
To: Bruce Richardson <bruce.richardson@intel.com>, dev@dpdk.org
Cc: Thomas Monjalon <thomas@monjalon.net>,
	Ray Kinsella <mdr@ashroe.eu>, Neil Horman <nhorman@tuxdriver.com>,
	bluca@debian.org
Subject: Re: [dpdk-dev] [PATCH v2 2/2] build: support building ABI versioned files twice
Date: Tue, 1 Oct 2019 15:23:47 +0200
Message-ID: <e0e68877-27e3-28a1-21f1-4fa1b413b6b8@semihalf.com> (raw)
In-Reply-To: <20190927205931.23022-3-bruce.richardson@intel.com>

Thanks Bruce for the patch.  I like the idea of splitting versioning out
of rte_compat.h, but I have some comments.

On 9/27/19 10:59 PM, Bruce Richardson wrote:
[...]
> --- a/config/common_base
> +++ b/config/common_base
> @@ -111,6 +111,7 @@ CONFIG_RTE_MAX_VFIO_CONTAINERS=64
>  CONFIG_RTE_MALLOC_DEBUG=n
>  CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n
>  CONFIG_RTE_USE_LIBBSD=n
> +CONFIG_RTE_USE_FUNCTION_VERSIONING=y

I'm not fond of this config option - it is not really an option to be
changed by the user.  I would prefer to just add flag to CFLAGS in
mk/target/generic/rte.vars.mk.

>  #
>  # Recognize/ignore the AVX/AVX512 CPU flags for performance/power testing.
> diff --git a/config/rte_config.h b/config/rte_config.h
> index 0bbbe274f..b63a2fdea 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -31,9 +31,6 @@
>  
>  /****** library defines ********/
>  
> -/* compat defines */
> -#define RTE_BUILD_SHARED_LIB
> -

So now everything builds "as static lib" (but with "-fPIC") apart from
those libraries that use symbol versioning.  I'm OK with that however
I'd like to note that code might be using RTE_BUILD_SHARED_LIB and do
different things e.g. app/test-bbdev/test_bbdev_perf.c.  I know that was
already the case - just wanted to say that aloud to make sure we are all
aware of this :).

> diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
> index 449b33494..e95a1a2be 100644
> --- a/doc/guides/contributing/coding_style.rst
> +++ b/doc/guides/contributing/coding_style.rst
> @@ -948,6 +948,13 @@ reason
>  	built. For missing dependencies this should be of the form
>  	``'missing dependency, "libname"'``.
>  
> +use_function_versioning
> +	**Default Value = false**.
> +	Specifies if the library in question has ABI versioned functions. If it
> +	has, this value should be set to ensure that the C files are compiled
> +	twice with suitable parameters for each of shared or static library
> +	builds.
> +

Maybe a different name for this option?  In general an "ideal
theoretical" solution would be for build system to figure out on its own
that separate build is necessary automatically - but that might incur
some performance penalty (additional grep'ing of sources or so).  So I'm
fine with this option however I'd like to rename it to actually indicate
what it's effect is.  Like 'separate_build' or 'split_build' or
'rebuild_objects' or ...

The intention of using of versioned symbols is already indicated by
inclusion of the relevant header.

> diff --git a/lib/librte_eal/common/include/rte_function_versioning.h b/lib/librte_eal/common/include/rte_function_versioning.h
> index ce963d4b1..55e88ffae 100644
> --- a/lib/librte_eal/common/include/rte_function_versioning.h
> +++ b/lib/librte_eal/common/include/rte_function_versioning.h
> @@ -7,6 +7,10 @@
>  #define _RTE_FUNCTION_VERSIONING_H_
>  #include <rte_common.h>
>  
> +#ifndef RTE_USE_FUNCTION_VERSIONING
> +#error Use of function versioning disabled, is "use_function_versioning=true" in meson.build?
> +#endif

If you accept above suggestion then change this message to something
like: "Function versioning requires 'separate_build=true' in meson.build"

BTW it turned out that this need for separate build for versioned
symbols is a result of long standing gcc bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48200

I'll test this with clang and if this will work then maybe we could
guard this #if with another check for 'gcc'.

Best regards
Andrzej

Tested-by: Andrzej Ostruszka <amo@semihalf.com>

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-27 19:49 [dpdk-dev] [PATCH 0/2] Improve function versioning meson support Bruce Richardson
2019-09-27 19:49 ` [dpdk-dev] [PATCH 1/2] eal: split compat header file Bruce Richardson
2019-09-27 20:48   ` Bruce Richardson
2019-09-27 19:49 ` [dpdk-dev] [PATCH 2/2] build: support building ABI versioned files twice Bruce Richardson
2019-09-27 20:59 ` [dpdk-dev] [PATCH v2 0/2] Improve function versioning meson support Bruce Richardson
2019-09-27 20:59   ` [dpdk-dev] [PATCH v2 1/2] eal: split compat header file Bruce Richardson
2019-09-27 20:59   ` [dpdk-dev] [PATCH v2 2/2] build: support building ABI versioned files twice Bruce Richardson
2019-10-01 13:23     ` Andrzej Ostruszka [this message]
2019-10-01 16:53       ` Bruce Richardson
2019-10-07 15:57         ` Bruce Richardson
2019-10-07 15:45 ` [dpdk-dev] [PATCH v3 0/2] Improve function versioning meson support Bruce Richardson
2019-10-07 15:45   ` [dpdk-dev] [PATCH v3 1/2] eal: split compat header file Bruce Richardson
2019-10-27  9:49     ` Thomas Monjalon
2019-10-07 15:45   ` [dpdk-dev] [PATCH v3 2/2] build: support building ABI versioned files twice Bruce Richardson
2019-10-23 10:19   ` [dpdk-dev] [PATCH v3 0/2] Improve function versioning meson support Andrzej Ostruszka
2019-10-27 10:26     ` Thomas Monjalon
2019-10-09 22:59 ` [dpdk-dev] [PATCH " Stephen Hemminger

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e0e68877-27e3-28a1-21f1-4fa1b413b6b8@semihalf.com \
    --to=amo@semihalf.com \
    --cc=bluca@debian.org \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=mdr@ashroe.eu \
    --cc=nhorman@tuxdriver.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git