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 >>
