(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
