[dpdk-dev] Hello Ferruh, Neil,

[Neil Horman]

On Tue, Feb 21, 2017 at 07:12:58PM +0530, Shreyansh Jain wrote:
> Thanks for the suggestions about rte_* renaming in DPAA2 PMD.
> I create a draft patch for a single symbol change. (applies over v7
> of DPAA2 PMD)
> 
> Can you tell me if this is the direction you were suggesting?
> 
> I see two issues in this approach which are somewhat problematic for
> me to change all the symbols:
> 1) We saw a drop of over 5% when I replaced only 3 symbols (one
>    of the most used ones, just for sampling). This also means that
>    when more of such symbols are replaced, it would bring further
>    drop. This was case when I used the Shared library approach.
>    (*) I am not well versed with gcc symbol aliasing to comment for
>        why such a drop would happen. But multiple test cycles confirm
>        this.
> 2) I have to include a new header in almost all the source files for PMD/
>    Pool/Bus etc. This is besides the STATIC_SYMBOL macros across the
>    code. Essentially, any internal repo patch cannot be directly
>    transposed to DPDK repo. Increased effort for each internal->
>    external release
> 
> Overall, I would like you to consider if this effort for changing names
> for exposed symbols is really useful or not.
> 
> There is another approach - that of not using a drivers/common library.
> This again is problematic for us - NXP DPAA2 being a hardware, the lib
> and state for Crypto and Net hardware is tied together - so, having
> multiple instances of library breaks either of Crypto or Net PMD.
> 
> Any other suggestions?
> 
> -
> Shreyansh
> 
> --->8---
> 
> From b9928ed44e69d7f9f9def0f06647a8d3bc25f284 Mon Sep 17 00:00:00 2001
> From: Shreyansh Jain <shreyansh.jain at nxp.com>
> Date: Fri, 10 Feb 2017 19:18:55 +0530
> Subject: [PATCH] dpaa2: fix for rte_* symbol naming
> 
> This sample code:
> * creates nxp_compat.h containing custom version of MAP_STATIC_* and
>   BIND_DEFAULT*
>   Can't use the existing version because they are based on appending
>   a string (_v[0-9]), not prepending as required in this case
> * creates nxp_shared_symbol.h for appending rte_* to symbols
> * declares functions
> * changes map file
> 
If thats what you are trying to do, I would suggest not using MAP_STATIC_SYMBOL,
as that will get confusing,  maybe create a new macro called ALIAS or some such
in the compat header that already exists that just expands to an alias
attribute.  Theres no point in redefining an existing macro if it doesn't
already do what you want.

> Hereafter:
> * for every newly introduced symbol, create NXP_*_SYMBOL mapping
> * put symbol in nxp_shared_symbols.h
> * add entry in map with rte_* prepended
> 
> Signed-off-by: Shreyansh Jain <shreyansh.jain at nxp.com>
> 
> ---
>  drivers/bus/fslmc/portal/dpaa2_hw_dpio.c           |  3 +
>  .../common/dpaa2/qbman/include/fsl_qbman_portal.h  |  1 +
>  drivers/common/dpaa2/qbman/include/nxp_compat.h    | 70 ++++++++++++++++++++++
>  .../dpaa2/qbman/include/nxp_shared_symbols.h       | 41 +++++++++++++
>  drivers/common/dpaa2/qbman/qbman_portal.c          |  2 +
>  .../dpaa2/qbman/rte_common_dpaa2_qbman_version.map |  2 +-
>  6 files changed, 118 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/common/dpaa2/qbman/include/nxp_compat.h
>  create mode 100644 drivers/common/dpaa2/qbman/include/nxp_shared_symbols.h
> 
> diff --git a/drivers/bus/fslmc/portal/dpaa2_hw_dpio.c b/drivers/bus/fslmc/portal/dpaa2_hw_dpio.c
> index c80d6c5..1c157b9 100644
> --- a/drivers/bus/fslmc/portal/dpaa2_hw_dpio.c
> +++ b/drivers/bus/fslmc/portal/dpaa2_hw_dpio.c
> @@ -59,9 +59,12 @@
>  
>  #include <fslmc_logs.h>
>  #include <fslmc_vfio.h>
> +#include <fsl_qbman_portal.h>
>  #include "dpaa2_hw_pvt.h"
>  #include "dpaa2_hw_dpio.h"
>  
> +#include <nxp_shared_symbols.h>
> +
>  #define NUM_HOST_CPUS RTE_MAX_LCORE
>  
>  struct dpaa2_io_portal_t dpaa2_io_portal[RTE_MAX_LCORE];
> diff --git a/drivers/common/dpaa2/qbman/include/fsl_qbman_portal.h b/drivers/common/dpaa2/qbman/include/fsl_qbman_portal.h
> index 7731772..63ffdd1 100644
> --- a/drivers/common/dpaa2/qbman/include/fsl_qbman_portal.h
> +++ b/drivers/common/dpaa2/qbman/include/fsl_qbman_portal.h
> @@ -29,6 +29,7 @@
>  #define _FSL_QBMAN_PORTAL_H
>  
>  #include <fsl_qbman_base.h>
> +#include "nxp_compat.h"
>  
>  /**
>   * DOC - QBMan portal APIs to implement the following functions:
> diff --git a/drivers/common/dpaa2/qbman/include/nxp_compat.h b/drivers/common/dpaa2/qbman/include/nxp_compat.h
> new file mode 100644
> index 0000000..edbbfde
> --- /dev/null
> +++ b/drivers/common/dpaa2/qbman/include/nxp_compat.h
> @@ -0,0 +1,70 @@
> +/*-
> + *   BSD LICENSE
> + *
> + * Copyright (C) 2014-2016 Freescale Semiconductor, Inc.
> + * Copyright (C) 2016 NXP
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in the
> + *       documentation and/or other materials provided with the distribution.
> + *     * Neither the name of Freescale Semiconductor nor the
> + *       names of its contributors may be used to endorse or promote products
> + *       derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _NXP_COMPAT_H_
> +#define _NXP_COMPAT_H_
> +#include <rte_common.h>
> +
> +#ifdef RTE_BUILD_SHARED_LIB
> +
> +/*
> + * BIND_DEFAULT_SYMBOL
> + * Creates a symbol version entry instructing the linker to bind references to
> + * symbol <b> to exported symbol <a>; it also appends the symbol version as
> + * per the map file.
> + */
> +#define NXP_SHARED_SYMBOL(b, a, n) __asm__(".symver " RTE_STR(b) ", " RTE_STR(a) "@@DPDK_" RTE_STR(n))
> +
> +/*
> + * MAP_STATIC_SYMBOL
> + * If a function has been bifurcated into multiple versions, none of which
> + * are defined as the exported symbol name in the map file, this macro can be
> + * used to alias a specific version of the symbol to its exported name.  For
> + * example, if you have 2 versions of a function foo_v1 and foo_v2, where the
> + * former is mapped to foo at DPDK_1 and the latter is mapped to foo at DPDK_2 when
> + * building a shared library, this macro can be used to map either foo_v1 or
> + * foo_v2 to the symbol foo when building a static library, e.g.:
> + * MAP_STATIC_SYMBOL(void foo(), foo_v2);
> + */
> +#define NXP_STATIC_SYMBOL(f, p)
> +
> +#else
> +/*
> + * No symbol versioning in use
> + */
> +#define NXP_SHARED_SYMBOL(b, e, n)
> +#define NXP_STATIC_SYMBOL(f, p) f __attribute__((alias(RTE_STR(p))))
> +/*
> + * RTE_BUILD_SHARED_LIB=n
> + */
> +#endif
> +
> +struct qbman_swp *rte_qbman_swp_init(const struct qbman_swp_desc *d);
> +
> +#endif /* _NXP_COMPAT_H_ */
> diff --git a/drivers/common/dpaa2/qbman/include/nxp_shared_symbols.h b/drivers/common/dpaa2/qbman/include/nxp_shared_symbols.h
> new file mode 100644
> index 0000000..6faba36
> --- /dev/null
> +++ b/drivers/common/dpaa2/qbman/include/nxp_shared_symbols.h
> @@ -0,0 +1,41 @@
> +/*-
> + *   BSD LICENSE
> + *
> + * Copyright (C) 2014-2016 Freescale Semiconductor, Inc.
> + * Copyright (C) 2016 NXP
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in the
> + *       documentation and/or other materials provided with the distribution.
> + *     * Neither the name of Freescale Semiconductor nor the
> + *       names of its contributors may be used to endorse or promote products
> + *       derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _NXP_SHARED_SYMBOLS_H_
> +#define _NXP_SHARED_SYMBOLS_H_
> +
> +#ifdef RTE_BUILD_SHARED_LIB
> +
> +#define qbman_swp_init			rte_qbman_swp_init
> +
> +#endif
> +
> +#endif /* NXP_STATIC_SYMBOL */
> +
> +
> diff --git a/drivers/common/dpaa2/qbman/qbman_portal.c b/drivers/common/dpaa2/qbman/qbman_portal.c
> index 11116bd..f9bad4f 100644
> --- a/drivers/common/dpaa2/qbman/qbman_portal.c
> +++ b/drivers/common/dpaa2/qbman/qbman_portal.c
> @@ -178,6 +178,8 @@ struct qbman_swp *qbman_swp_init(const struct qbman_swp_desc *d)
>  	portal_idx_map[p->desc.idx] = p;
>  	return p;
>  }
> +NXP_SHARED_SYMBOL(qbman_swp_init, rte_qbman_swp_init, 17.02);
> +NXP_STATIC_SYMBOL(struct qbman_swp *rte_qbman_swp_init(const struct qbman_swp_desc *d), qbman_swp_init);
>  
>  void qbman_swp_finish(struct qbman_swp *p)
>  {
> diff --git a/drivers/common/dpaa2/qbman/rte_common_dpaa2_qbman_version.map b/drivers/common/dpaa2/qbman/rte_common_dpaa2_qbman_version.map
> index f653421..1dda776 100644
> --- a/drivers/common/dpaa2/qbman/rte_common_dpaa2_qbman_version.map
> +++ b/drivers/common/dpaa2/qbman/rte_common_dpaa2_qbman_version.map
> @@ -18,7 +18,7 @@ DPDK_17.02 {
>  	qbman_result_DQ_flags;
>  	qbman_result_has_new_result;
>  	qbman_swp_acquire;
> -	qbman_swp_init;
> +	rte_qbman_swp_init;
>  	qbman_swp_pull;
>  	qbman_swp_release;
>  	qbman_swp_send_multiple;
> -- 
> 2.7.4
> 
>