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

["Xie, Huawei" <huawei.xie@intel.com>]

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