[dpdk-dev] Hello Ferruh, Neil,

Message ID 1487684578-28656-1-git-send-email-shreyansh.jain@nxp.com (mailing list archive)
State Not Applicable, archived
Headers

Checks

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

Commit Message

Shreyansh Jain Feb. 21, 2017, 1:42 p.m. UTC
  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@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

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

Comments

Shreyansh Jain Feb. 21, 2017, 1:45 p.m. UTC | #1
(Modified subject to: "Re: [PATCHv7 03/47] common/dpaa2: adding qbman 
driver")

On Tuesday 21 February 2017 07:12 PM, 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

Apologies for the modified subject in the previous email.
While sending out the patch, I didn't pay attention to the 'patch head
line'.

-
Shreyansh
  
Ferruh Yigit Feb. 21, 2017, 2:39 p.m. UTC | #2
On 2/21/2017 1:42 PM, 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.

As you showed below, this works for exporting proper APIs, but not sure
if this change worth 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.

Isn't is possible to keep folder structure same, but produce single library.
Because these don't provide any API to the user application, perhaps not
need to be library at all.

Assuming that bus and pool won't be required without a driver existing,
is it possible have a single driver library that contains others?

For net driver, dependency is as following:
  bus_fslmc  --> common_dpaa2_qbman
  pool_dpaa2 --> bus_fslmc, common_dpaa2_qbman
  pmd_dpaa2  --> pool_dpaa2, bus_fslmc, common_dpaa2_qbman

So generating only "librte_pmd_dpaa2" which include above dependent ones.

For cryptodev pmd, I assume it has dependency to same modules:
  pmd_crypto --> pool_dpaa2, bus_fslmc, common_dpaa2_qbman

And this will generate only crypto pmd library, including all again.


This will create duplication in binaries, but I think easier to manage
library dependencies.


And for above case, as far as I know, both net and crypto libraries can
be linked against a binary even there are duplicate symbols. Are you
getting error here?

> 
> Any other suggestions?
> 
> -
> Shreyansh
> 
> --->8---
> 
> From b9928ed44e69d7f9f9def0f06647a8d3bc25f284 Mon Sep 17 00:00:00 2001
> From: Shreyansh Jain <shreyansh.jain@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
> 
> 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@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@DPDK_1 and the latter is mapped to foo@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;
>
  
Shreyansh Jain Feb. 22, 2017, 8:23 a.m. UTC | #3
(Modified the subject to: 'Re: [PATCHv7 03/47] common/dpaa2: adding 
qbman driver' from 'Re: Hello Ferruh, Neil,')

Hello Ferruh,

On Tuesday 21 February 2017 08:09 PM, Ferruh Yigit wrote:
> On 2/21/2017 1:42 PM, 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.
>
> As you showed below, this works for exporting proper APIs, but not sure
> if this change worth or not.

Given such symbol aliasing is an impact on performance, probably there
is a need to discuss the strictness of rte_* appending for driver
symbols.
As for cost of maintaining such code base, it can be rationalized over
a period of time, but not performance.

>
>>
>> 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.
>
> Isn't is possible to keep folder structure same, but produce single library.
> Because these don't provide any API to the user application, perhaps not
> need to be library at all.
>
> Assuming that bus and pool won't be required without a driver existing,
> is it possible have a single driver library that contains others?
>
> For net driver, dependency is as following:
>   bus_fslmc  --> common_dpaa2_qbman
>   pool_dpaa2 --> bus_fslmc, common_dpaa2_qbman
>   pmd_dpaa2  --> pool_dpaa2, bus_fslmc, common_dpaa2_qbman
>
> So generating only "librte_pmd_dpaa2" which include above dependent ones.
>
> For cryptodev pmd, I assume it has dependency to same modules:
>   pmd_crypto --> pool_dpaa2, bus_fslmc, common_dpaa2_qbman
>
> And this will generate only crypto pmd library, including all again.
>
>
> This will create duplication in binaries, but I think easier to manage
> library dependencies.
>
>
> And for above case, as far as I know, both net and crypto libraries can
> be linked against a binary even there are duplicate symbols. Are you
> getting error here?
>
<snip>

Thanks for your comments.

The key issue here is that driver/common is not actually a 'library' in
traditional sense. It is a driver support system. It provides
interfaces to interact with the hardware - and that includes the Net
and Crypto hardware.

Being a 'driver', this also has its own state. For example, a mem area
to interact with hardware queues, whether net or crypto - there is a
single instance of it.

This restricts its duplication as a library.
In fact, as of now the statefulness is quite limited, but once more
devices (like for eventdev) come into picture, this would become more
prominent.

Now, we have these possibility:
1. Have a shared library with non rte_* symbols
2. We have shared library with rte_* symbols
3. We have non-net devices (crypto, eventdev, ..) depend on net for 
these hardware interfaces

(2) is hitting performance significantly.
(3) it not a clean solution, having driver/crypto depend on driver/net. 
When new devices are there, more dependencies will occur.

In crux, probably we need to have a discussion on (1) and how strongly 
we feel about that (specially in context of drivers).

-
Shreyansh
  
Neil Horman Feb. 22, 2017, 12:41 p.m. UTC | #4
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@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@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@DPDK_1 and the latter is mapped to foo@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
> 
>
  
Ferruh Yigit Feb. 24, 2017, 9:58 a.m. UTC | #5
On 2/22/2017 8:23 AM, Shreyansh Jain wrote:
> (Modified the subject to: 'Re: [PATCHv7 03/47] common/dpaa2: adding 
> qbman driver' from 'Re: Hello Ferruh, Neil,')
> 
> Hello Ferruh,
> 
> On Tuesday 21 February 2017 08:09 PM, Ferruh Yigit wrote:
>> On 2/21/2017 1:42 PM, 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.
>>
>> As you showed below, this works for exporting proper APIs, but not sure
>> if this change worth or not.
> 
> Given such symbol aliasing is an impact on performance, probably there
> is a need to discuss the strictness of rte_* appending for driver
> symbols.
> As for cost of maintaining such code base, it can be rationalized over
> a period of time, but not performance.
> 
>>
>>>
>>> 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.
>>
>> Isn't is possible to keep folder structure same, but produce single library.
>> Because these don't provide any API to the user application, perhaps not
>> need to be library at all.
>>
>> Assuming that bus and pool won't be required without a driver existing,
>> is it possible have a single driver library that contains others?
>>
>> For net driver, dependency is as following:
>>   bus_fslmc  --> common_dpaa2_qbman
>>   pool_dpaa2 --> bus_fslmc, common_dpaa2_qbman
>>   pmd_dpaa2  --> pool_dpaa2, bus_fslmc, common_dpaa2_qbman
>>
>> So generating only "librte_pmd_dpaa2" which include above dependent ones.
>>
>> For cryptodev pmd, I assume it has dependency to same modules:
>>   pmd_crypto --> pool_dpaa2, bus_fslmc, common_dpaa2_qbman
>>
>> And this will generate only crypto pmd library, including all again.
>>
>>
>> This will create duplication in binaries, but I think easier to manage
>> library dependencies.
>>
>>
>> And for above case, as far as I know, both net and crypto libraries can
>> be linked against a binary even there are duplicate symbols. Are you
>> getting error here?
>>
> <snip>
> 
> Thanks for your comments.
> 
> The key issue here is that driver/common is not actually a 'library' in
> traditional sense. It is a driver support system. It provides
> interfaces to interact with the hardware - and that includes the Net
> and Crypto hardware.
> 
> Being a 'driver', this also has its own state. For example, a mem area
> to interact with hardware queues, whether net or crypto - there is a
> single instance of it.
> 
> This restricts its duplication as a library.
> In fact, as of now the statefulness is quite limited, but once more
> devices (like for eventdev) come into picture, this would become more
> prominent.
> 
> Now, we have these possibility:
> 1. Have a shared library with non rte_* symbols
> 2. We have shared library with rte_* symbols
> 3. We have non-net devices (crypto, eventdev, ..) depend on net for 
> these hardware interfaces
> 
> (2) is hitting performance significantly.
> (3) it not a clean solution, having driver/crypto depend on driver/net. 
> When new devices are there, more dependencies will occur.
> 
> In crux, probably we need to have a discussion on (1) and how strongly 
> we feel about that (specially in context of drivers).

Insight of above information, I would be OK with (1).

We can go with option (1) now, since these are not real APIs to user
application, it can be possible to change them if better solution found.

Do you think is it good idea to have different naming syntax for those
libraries to clarify they are for PMD internal usage?

> 
> -
> Shreyansh
>
  
Shreyansh Jain Feb. 27, 2017, 10:01 a.m. UTC | #6
Hello Ferruh,

On Friday 24 February 2017 03:28 PM, Ferruh Yigit wrote:

[snip]

>>
>> Now, we have these possibility:
>> 1. Have a shared library with non rte_* symbols
>> 2. We have shared library with rte_* symbols
>> 3. We have non-net devices (crypto, eventdev, ..) depend on net for
>> these hardware interfaces
>>
>> (2) is hitting performance significantly.
>> (3) it not a clean solution, having driver/crypto depend on driver/net.
>> When new devices are there, more dependencies will occur.
>>
>> In crux, probably we need to have a discussion on (1) and how strongly
>> we feel about that (specially in context of drivers).
>
> Insight of above information, I would be OK with (1).

Great. Thank you for understanding.

>
> We can go with option (1) now, since these are not real APIs to user
> application, it can be possible to change them if better solution found.
>
> Do you think is it good idea to have different naming syntax for those
> libraries to clarify they are for PMD internal usage?
>

Indeed. Current name is librte_common_dpaa2_*.
Do you think librte_drvlib_dpaa2 or librte_drvlib_dpaa2_pmd is better?
  
Ferruh Yigit Feb. 27, 2017, 3:35 p.m. UTC | #7
On 2/27/2017 10:01 AM, Shreyansh Jain wrote:
> Hello Ferruh,
> 
> On Friday 24 February 2017 03:28 PM, Ferruh Yigit wrote:
> 
> [snip]
> 
>>>
>>> Now, we have these possibility:
>>> 1. Have a shared library with non rte_* symbols
>>> 2. We have shared library with rte_* symbols
>>> 3. We have non-net devices (crypto, eventdev, ..) depend on net for
>>> these hardware interfaces
>>>
>>> (2) is hitting performance significantly.
>>> (3) it not a clean solution, having driver/crypto depend on driver/net.
>>> When new devices are there, more dependencies will occur.
>>>
>>> In crux, probably we need to have a discussion on (1) and how strongly
>>> we feel about that (specially in context of drivers).
>>
>> Insight of above information, I would be OK with (1).
> 
> Great. Thank you for understanding.
> 
>>
>> We can go with option (1) now, since these are not real APIs to user
>> application, it can be possible to change them if better solution found.
>>
>> Do you think is it good idea to have different naming syntax for those
>> libraries to clarify they are for PMD internal usage?
>>
> 
> Indeed. Current name is librte_common_dpaa2_*.
> Do you think librte_drvlib_dpaa2 or librte_drvlib_dpaa2_pmd is better?

common vs drvlib may not be different for who don't know about these
libraries, what about using "internal" or "private" kind of keyword?
  
Shreyansh Jain Feb. 28, 2017, 5:27 a.m. UTC | #8
> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Monday, February 27, 2017 9:06 PM
> To: Shreyansh Jain <shreyansh.jain@nxp.com>
> Cc: dev@dpdk.org; nhorman@tuxdriver.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>
> Subject: Re: [PATCHv7 03/47] common/dpaa2: adding qbman driver
> 
> On 2/27/2017 10:01 AM, Shreyansh Jain wrote:
> > Hello Ferruh,
> >
> > On Friday 24 February 2017 03:28 PM, Ferruh Yigit wrote:
> >
> > [snip]
> >
> >>>
> >>> Now, we have these possibility:
> >>> 1. Have a shared library with non rte_* symbols
> >>> 2. We have shared library with rte_* symbols
> >>> 3. We have non-net devices (crypto, eventdev, ..) depend on net for
> >>> these hardware interfaces
> >>>
> >>> (2) is hitting performance significantly.
> >>> (3) it not a clean solution, having driver/crypto depend on driver/net.
> >>> When new devices are there, more dependencies will occur.
> >>>
> >>> In crux, probably we need to have a discussion on (1) and how strongly
> >>> we feel about that (specially in context of drivers).
> >>
> >> Insight of above information, I would be OK with (1).
> >
> > Great. Thank you for understanding.
> >
> >>
> >> We can go with option (1) now, since these are not real APIs to user
> >> application, it can be possible to change them if better solution found.
> >>
> >> Do you think is it good idea to have different naming syntax for those
> >> libraries to clarify they are for PMD internal usage?
> >>
> >
> > Indeed. Current name is librte_common_dpaa2_*.
> > Do you think librte_drvlib_dpaa2 or librte_drvlib_dpaa2_pmd is better?
> 
> common vs drvlib may not be different for who don't know about these
> libraries, what about using "internal" or "private" kind of keyword?

I am ok with librte_pvtlib_dpaa2_pmd or librte_pvtlib_dpaa2. Sounds fine?
('internal' is too long and its abbreviation 'int' doesn't make it easier
to read. :D )

-
Shreyansh
  
Thomas Monjalon March 1, 2017, 11 a.m. UTC | #9
2017-02-28 05:27, Shreyansh Jain:
> From: Ferruh Yigit
> > On 2/27/2017 10:01 AM, Shreyansh Jain wrote:
> > > On Friday 24 February 2017 03:28 PM, Ferruh Yigit wrote:
> > >> We can go with option (1) now, since these are not real APIs to user
> > >> application, it can be possible to change them if better solution found.
> > >>
> > >> Do you think is it good idea to have different naming syntax for those
> > >> libraries to clarify they are for PMD internal usage?
> > >>
> > >
> > > Indeed. Current name is librte_common_dpaa2_*.
> > > Do you think librte_drvlib_dpaa2 or librte_drvlib_dpaa2_pmd is better?
> > 
> > common vs drvlib may not be different for who don't know about these
> > libraries, what about using "internal" or "private" kind of keyword?
> 
> I am ok with librte_pvtlib_dpaa2_pmd or librte_pvtlib_dpaa2. Sounds fine?
> ('internal' is too long and its abbreviation 'int' doesn't make it easier
> to read. :D )

There is already "lib" in "librte". What about librte_internal_dpaa2_ prefix?

Internal is really the best word as you are requesting DPDK to host
libraries used only for your drivers.
I thought you agreed to host them outside of the DPDK repository.
What is your reason to keep pushing in DPDK?
  
Hemant Agrawal March 1, 2017, 12:26 p.m. UTC | #10
On 3/1/2017 4:30 PM, Thomas Monjalon wrote:
> 2017-02-28 05:27, Shreyansh Jain:
>> From: Ferruh Yigit
>>> On 2/27/2017 10:01 AM, Shreyansh Jain wrote:
>>>> On Friday 24 February 2017 03:28 PM, Ferruh Yigit wrote:
>>>>> We can go with option (1) now, since these are not real APIs to user
>>>>> application, it can be possible to change them if better solution found.
>>>>>
>>>>> Do you think is it good idea to have different naming syntax for those
>>>>> libraries to clarify they are for PMD internal usage?
>>>>>
>>>>
>>>> Indeed. Current name is librte_common_dpaa2_*.
>>>> Do you think librte_drvlib_dpaa2 or librte_drvlib_dpaa2_pmd is better?
>>>
>>> common vs drvlib may not be different for who don't know about these
>>> libraries, what about using "internal" or "private" kind of keyword?
>>
>> I am ok with librte_pvtlib_dpaa2_pmd or librte_pvtlib_dpaa2. Sounds fine?
>> ('internal' is too long and its abbreviation 'int' doesn't make it easier
>> to read. :D )
>
> There is already "lib" in "librte". What about librte_internal_dpaa2_ prefix?
>
Ok. We were just trying to avoid that long name. :)

> Internal is really the best word as you are requesting DPDK to host
> libraries used only for your drivers.
> I thought you agreed to host them outside of the DPDK repository.
> What is your reason to keep pushing in DPDK?
>

I don't think the last discussion closed like this. You suggested [1] 
this as an option during handing of issue with rte.lib.mk causing the 
parallel build dependency of shared lib to fail for drivers/common. 
Ferruh has already fixed that[2].

The drivers/common is an integral part of PMD - very similar to a *base* 
directory of any existing DPDK PMDs. The only difference is that *NXP's 
base* is common across multiple drivers (bus, pool, net, crypto, 
eventdev etc) - all of them interface with SoC using this 'driver 
interface'.  We named it as "driver common lib" - we could not find a 
better way to name it. Probably, a hw driver interface or dpaa2_base is 
a better explanation.

Imagine a PMD which resides within DPDK (dpaa2 pmd) which only has high 
level interfaces with DPDK API and cannot talk to a real hardware (SoC) 
because it lacks the basic driver interface. That would really not make 
that code a 'PMD'.

Probably it would be better if we close what is the scope of a PMD 
before this discussion is to go ahead. And, why would there be a need to 
store this 'lib' externally.

Following is what Neil replied to your suggestion [3]. And we fully 
agree with it.
"Please do not go with suggestion two, the more libraries get hosted 
outside of the project, the less likely any sort of test/build/ongoing 
maintenance from the community can be expected.  If you're going to go 
with solution (2), then you may as well host the entire PMD outside of 
the DPDK project, and that more undesirable."


[1] http://dpdk.org/ml/archives/dev/2017-January/056243.html
[2] http://dpdk.org/ml/archives/dev/2017-January/056321.html
[3] http://dpdk.org/ml/archives/dev/2017-January/056292.html
  

Patch

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@DPDK_1 and the latter is mapped to foo@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;