Re: [dpdk-dev] [PATCH v5 1/3] vhost: Add callback and private data for vhost PMD

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/12/18 19:03, Xie, Huawei wrote:
> On 12/18/2015 12:15 PM, Yuanhan Liu wrote:
>> On Fri, Dec 18, 2015 at 12:15:42PM +0900, Tetsuya Mukawa wrote:
>>> On 2015/12/17 20:42, Yuanhan Liu wrote:
>>>> On Tue, Nov 24, 2015 at 06:00:01PM +0900, Tetsuya Mukawa wrote:
>>>>> The vhost PMD will be a wrapper of vhost library, but some of vhost
>>>>> library APIs cannot be mapped to ethdev library APIs.
>>>>> Becasue of this, in some cases, we still need to use vhost library APIs
>>>>> for a port created by the vhost PMD.
>>>>>
>>>>> Currently, when virtio device is created and destroyed, vhost library
>>>>> will call one of callback handlers. The vhost PMD need to use this
>>>>> pair of callback handlers to know which virtio devices are connected
>>>>> actually.
>>>>> Because we can register only one pair of callbacks to vhost library, if
>>>>> the PMD use it, DPDK applications cannot have a way to know the events.
>>>>>
>>>>> This may break legacy DPDK applications that uses vhost library. To prevent
>>>>> it, this patch adds one more pair of callbacks to vhost library especially
>>>>> for the vhost PMD.
>>>>> With the patch, legacy applications can use the vhost PMD even if they need
>>>>> additional specific handling for virtio device creation and destruction.
>>>>>
>>>>> For example, legacy application can call
>>>>> rte_vhost_enable_guest_notification() in callbacks to change setting.
>>>> TBH, I never liked it since the beginning. Introducing two callbacks
>>>> for one event is a bit messy, and therefore error prone.
>>> I agree with you.
>>>
>>>> I have been thinking this occasionally last few weeks, and have came
>>>> up something that we may introduce another layer callback based on
>>>> the vhost pmd itself, by a new API:
>>>>
>>>> 	rte_eth_vhost_register_callback().
>>>>
>>>> And we then call those new callback inside the vhost pmd new_device()
>>>> and vhost pmd destroy_device() implementations.
>>>>
>>>> And we could have same callbacks like vhost have, but I'm thinking
>>>> that new_device() and destroy_device() doesn't sound like a good name
>>>> to a PMD driver. Maybe a name like "link_state_changed" is better?
>>>>
>>>> What do you think of that?
>>> Yes,  "link_state_changed" will be good.
>>>
>>> BTW, I thought it was ok that an DPDK app that used vhost PMD called
>>> vhost library APIs directly.
>>> But probably you may feel strangeness about it. Is this correct?
>> Unluckily, that's true :)
>>
>>> If so, how about implementing legacy status interrupt mechanism to vhost
>>> PMD?
>>> For example, an DPDK app can register callback handler like
>>> "examples/link_status_interrupt".
>>>
>>> Also, if the app doesn't call vhost library APIs directly,
>>> rte_eth_vhost_portid2vdev() will be needless, because the app doesn't
>>> need to handle virtio device structure anymore.
>>>
>>>> On the other hand, I'm still thinking is that really necessary to let
>>>> the application be able to call vhost functions like rte_vhost_enable_guest_notification()
>>>> with the vhost PMD driver?
>>> Basic concept of my patch is that vhost PMD will provides the features
>>> that vhost library provides.
>> I don't think that's necessary. Let's just treat it as a normal pmd
>> driver, having nothing to do with vhost library.
>>
>>> How about removing rte_vhost_enable_guest_notification() from "vhost
>>> library"?
>>> (I also not sure what are use cases)
>>> If we can do this, vhost PMD also doesn't need to take care of it.
>>> Or if rte_vhost_enable_guest_notification() will be removed in the
>>> future, vhost PMD is able to ignore it.
>> You could either call it in vhost-pmd (which you already have done that),
>> or ignore it in vhost-pmd, but dont' remove it from vhost library.
>>
>>> Please let me correct up my thinking about your questions.
>>>  - Change concept of patch not to call vhost library APIs directly.
>>> These should be wrapped by ethdev APIs.
>>>  - Remove rte_eth_vhost_portid2vdev(), because of above concept changing.
>>>  - Implement legacy status changed interrupt to vhost PMD instead of
>>> using own callback mechanism.
>>>  - Check if we can remove rte_vhost_enable_guest_notification() from
>>> vhost library.
>> So, how about making it __fare__ simple as the first step, to get merged
>> easily, that we don't assume the applications will call any vhost library
>> functions any more, so that we don't need the callback, and we don't need
>> the rte_eth_vhost_portid2vdev(), either. Again, just let it be a fare
>> normal (nothing special) pmd driver.  (UNLESS, there is a real must, which
>> I don't see so far).
>>
>> Tetsuya, what do you think of that then?
>>
>>> Hi Xie,
>>>
>>> Do you know the use cases of rte_vhost_enable_guest_notification()?
> If vhost runs in loop mode, it doesn't need to be notified. You have
> wrapped vhost as the PMD, which is nice for OVS integration. If we
> require that all PMDs could be polled by select/poll, then we could use
> this API for vhost PMD, and wait on the kickfd. For physical nics, we
> could wait on the fd for user space interrupt.

Thanks for clarification.
I will ignore the function for first release of vhost PMD.

Thanks,
Tetsuya


>> Setting the arg to 0 avoids the guest kicking the virtqueue, which
>> is good for performance, and we should keep it.
>>
>> 	--yliu
>>