From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCHv7 03/47] common/dpaa2: adding qbman driver Date: Wed, 22 Feb 2017 13:53:00 +0530 Message-ID: References: <1487684578-28656-1-git-send-email-shreyansh.jain@nxp.com> <8958b9ca-0a7d-3df0-3b62-4b9c610d301c@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: , , "hemant.agrawal@nxp.com" To: Ferruh Yigit Return-path: Received: from NAM03-CO1-obe.outbound.protection.outlook.com (mail-co1nam03on0077.outbound.protection.outlook.com [104.47.40.77]) by dpdk.org (Postfix) with ESMTP id 3A8F82BB4 for ; Wed, 22 Feb 2017 09:18:18 +0100 (CET) In-Reply-To: <8958b9ca-0a7d-3df0-3b62-4b9c610d301c@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" (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? > 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