[dpdk-dev] [PATCH v4 66/70] bus/fslmc: enable support for mem event callbacks for vfio

[Burakov, Anatoly]

On 09-Apr-18 1:09 PM, Shreyansh Jain wrote:
> On Monday 09 April 2018 04:25 PM, Burakov, Anatoly wrote:
>> On 09-Apr-18 11:01 AM, Shreyansh Jain wrote:
>>> Hi Anatoly,
>>>
>>> On Monday 09 April 2018 01:48 AM, Anatoly Burakov wrote:
>>>> VFIO needs to map and unmap segments for DMA whenever they
>>>> become available or unavailable, so register a callback for
>>>> memory events, and provide map/unmap functions.
>>>>
>>>> Signed-off-by: Shreyansh Jain <shreyansh.jain at nxp.com>
>>>> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
>>>> ---
>>
>> <...>
>>
>>>> +        DPAA2_BUS_DEBUG("Calling with type=%d, va=%p, 
>>>> virt_addr=0x%" PRIx64 ", iova=0x%" PRIx64 ", map_len=%zu\n",
>>>
>>> I would like to correct this message (80char + rewording) - What do 
>>> you suggest? Should I send a new patch to you or just convey what 
>>> should be changed?
>>>
>>
>> As far as i know, leaving strings on single line is good for grepping. 
>> However, perhaps having PRIx64 etc in there breaks it anyway.
> 
> Yes, that and the debug message was not helpful.
> This is what I had in mind. (DPAA2_BUS_DEBUG doesn't require an extra \n)
> 
> DPAA2_BUS_DEBUG("Request for %s, va=%p, virt_addr=0x%" PRIx64 ","
>          "iova=0x%" PRIx64 ", map_len=%zu",
>          type == RTE_MEM_EVENT_ALLOC? "alloc" : "dealloc",
>          va, virt_addr, iova_addr, map_len);
> 
>>
>>>> +                type, va, virt_addr, iova_addr, map_len);
>>>> +
>>>> +        if (type == RTE_MEM_EVENT_ALLOC)
>>>> +            ret = fslmc_map_dma(virt_addr, iova_addr, map_len);
>>>> +        else
>>>> +            ret = fslmc_unmap_dma(virt_addr, iova_addr, map_len);
>>>> +
>>>> +        if (ret != 0) {
>>>> +            DPAA2_BUS_ERR("DMA Mapping/Unmapping failed. Map=%d, 
>>>> addr=%p, len=%zu, err:(%d)",
>>>> +                    type, va, map_len, ret);
>>>
>>> Same as above. 80 Char issue.
>>
>> Same reasoning - leaving strings unbroken allows for easier grepping 
>> the codebase, but i'm not sure what's our policy on having formatted 
>> strings unbroken.
> 
> My policy is not different, but the various variables being dumped 
> cannot anyway help in grepping - So, keeping the variables on separate 
> lines for 80chars is ok. "DMA Mapping/Unmapping failed." is enough for 
> greps.
> 
>>
>>>
>>>> +            return;
>>>> +        }
>>>> +
>>>> +        cur_len += map_len;
>>>> +    }
>>>> +
>>>> +    if (type == RTE_MEM_EVENT_ALLOC)
>>>> +        DPAA2_BUS_DEBUG("Total Mapped: addr=%p, len=%zu\n",
>>>> +                addr, len);
>>>> +    else
>>
>> <...>
>>
>>>> +    ret = rte_mem_event_callback_register("fslmc_memevent_clb",
>>>> +                          fslmc_memevent_cb);
>>>> +    if (ret && rte_errno == ENOTSUP)
>>>> +        DPAA2_BUS_DEBUG("Memory event callbacks not supported");
>>>> +    else if (ret)
>>>> +        DPAA2_BUS_DEBUG("Unable to install memory handler");
>>>> +    else
>>>> +        DPAA2_BUS_DEBUG("Installed memory callback handler");
>>>>       /* Verifying that at least single segment is available */
>>>>       if (i <= 0) {
>>>> +        /* TODO: Is this verification required any more? What would
>>>> +         * happen to non-legacy case where nothing was preallocated
>>>> +         * thus causing i==0?
>>>> +         */
>>>
>>> And this as well - if call backs are not going to appear in case of 
>>> legacy, this needs to be removed.
>>
>> Callbacks aren't only not going to appear in legacy mode - they will 
>> also not appear on FreeBSD. We check this above, with checking 
>> rte_errno value (if callbacks are not supported, it's set to ENOTSUP, 
>> and having callbacks unsupported is not an error).
>>
>>> let me know how do you want to take these changes.
>>>
>>
>> Now that i think of it, this error condition is wrong. This is called 
>> in both legacy and non-legacy mode. This is bus probe, no? For 
>> non-legacy mode, it is entirely possible to start without any memory 
>> whatsoever. It just so happens that rte_service API allocates some on 
>> init, and hence you always have at least one segment - that may not be 
>> the case forever. So, non-legacy mode, not having memsegs is not an 
>> error, it is expected behavior, so maybe we should remove this error 
>> check altogether.
> 
> Agree - that count was only required in the earlier case. It can be 
> removed.
> 
>>
>>>>           DPAA2_BUS_ERR("No Segments found for VFIO Mapping");
>>>> +        rte_rwlock_read_unlock(mem_lock);
>>>>           return -1;
>>>>       }
>>>>       DPAA2_BUS_DEBUG("Total %d segments found.", i);
>>>> @@ -250,6 +367,11 @@ int rte_fslmc_vfio_dmamap(void)
>>>>        */
>>>>       vfio_map_irq_region(&vfio_group);
>>>> +    /* Existing segments have been mapped and memory callback for 
>>>> hotplug
>>>> +     * has been installed.
>>>> +     */
>>>> +    rte_rwlock_read_unlock(mem_lock);
>>>> +
>>>>       return 0;
>>>>   }
>>>>
>>>
>>>
>>
>>
> 
> I think there are enough changes, even if trivial. Maybe I can rework 
> this patch and send you. If that is inconvenient, just extract from that 
> whatever you want.
> 

There aren't a lot of changes so i'll respin it myself, addressing the 
comments above. Thanks!

-- 
Thanks,
Anatoly