Re: [dpdk-dev] [PATCH 3/5] common/dpaax: add library for PA VA translation table

[Shreyansh Jain <shreyansh.jain@nxp.com>]

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 <shreyansh.jain@nxp.com>
>> ---
> 
> 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 +
> 
> <snip>
> 
>> +    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 */
> 
> <snip>
> 
>> +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.

>