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

Shreyansh Jain shreyansh.jain at nxp.com
Mon Apr 9 14:09:55 CEST 2018


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.


More information about the dev mailing list