From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH 3/5] common/dpaax: add library for PA VA translation table Date: Tue, 25 Sep 2018 19:09:10 +0530 Message-ID: <894130a9-017c-348d-31f8-c4c23f517f25@nxp.com> References: <20180925125423.7505-1-shreyansh.jain@nxp.com> <20180925125423.7505-4-shreyansh.jain@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: ferruh.yigit@intel.com, dev@dpdk.org To: "Burakov, Anatoly" Return-path: Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-eopbgr30074.outbound.protection.outlook.com [40.107.3.74]) by dpdk.org (Postfix) with ESMTP id 2D2D91B178 for ; Tue, 25 Sep 2018 15:39:40 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Anatoly, On Tuesday 25 September 2018 06:58 PM, Burakov, Anatoly wrote: > On 25-Sep-18 1:54 PM, Shreyansh Jain wrote: >> A common library, valid for dpaaX drivers, which is used to maintain >> a local copy of PA->VA translations. >> >> In case of physical addressing mode (one of the option for FSLMC, and >> only option for DPAA bus), the addresses of descriptors Rx'd are >> physical. These need to be converted into equivalent VA for rte_mbuf >> and other similar calls. >> >> Using the rte_mem_virt2iova or rte_mem_virt2phy is expensive. This >> library is an attempt to reduce the overall cost associated with >> this translation. >> >> A small table is maintained, containing continuous entries >> representing a continguous physical range. Each of these entries >> stores the equivalent VA, which is fed during mempool creation, or >> memory allocation/deallocation callbacks. >> >> Signed-off-by: Shreyansh Jain >> --- > > Hi Shreyansh, > > So, basically, you're reimplementing old DPDK's memory view (storing > VA's in a PA-centric way). Makes sense :) Yes, and frankly, I couldn't come up with any other way. > > I should caution you that right now, external memory allocator > implementation does *not* trigger any callbacks for newly added memory. > So, anything coming from external memory will not be reflected in your > table, unless it happens to be already there before > dpaax_iova_table_populate() gets called. This patchset makes a good > argument for why perhaps it should trigger callbacks. Thoughts? Oh. Then I must be finishing reading through your patches for external memory sooner. I didn't realize this. > > Also, a couple of nitpicks below. > >>   config/common_base                            |   5 + >>   config/common_linuxapp                        |   5 + >>   drivers/common/Makefile                       |   4 + >>   drivers/common/dpaax/Makefile                 |  31 ++ >>   drivers/common/dpaax/dpaax_iova_table.c       | 509 ++++++++++++++++++ >>   drivers/common/dpaax/dpaax_iova_table.h       | 104 ++++ >>   drivers/common/dpaax/dpaax_logs.h             |  39 ++ >>   drivers/common/dpaax/meson.build              |  12 + > > > >> +    DPAAX_DEBUG("Add: Found slot at (%"PRIu64")[(%zu)] for vaddr:(%p)," >> +            " phy(%"PRIu64"), len(%zu)", entry[i].start, e_offset, >> +            vaddr, paddr, length); >> +    return 0; >> +} >> + >> +int >> +dpaax_iova_table_del(phys_addr_t paddr, size_t len __rte_unused) > > len is not unused. I will fix this. Actually, this function itself is useless - more for symmetry reason. Callers would be either simply updating the table, or ignoring it completely. But, yes, this is indeed wrong that I set that unused. > >> +{ >> +    int found = 0; >> +    unsigned int i; >> +    size_t e_offset; >> +    struct dpaax_iovat_element *entry; >> +    phys_addr_t align_paddr; >> + >> +    align_paddr = paddr & DPAAX_MEM_SPLIT_MASK; >> + >> +    /* Check if paddr is available in table */ > > > >> +static inline void * >> +dpaax_iova_table_get_va(phys_addr_t paddr) { >> +    unsigned int i = 0, index; >> +    void *vaddr = 0; >> +    phys_addr_t paddr_align = paddr & DPAAX_MEM_SPLIT_MASK; >> +    size_t offset = paddr & DPAAX_MEM_SPLIT_MASK_OFF; >> +    struct dpaax_iovat_element *entry; >> + >> +    entry = dpaax_iova_table_p->entries; >> + >> +    do { >> +        if (unlikely(i > dpaax_iova_table_p->count)) >> +            break; >> + >> +        if (paddr_align < entry[i].start) { >> +            /* Incorrect paddr; Not in memory range */ >> +            return 0; > > NULL? Yes, NULL. I will fix that as well. >