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

[Yuanhan Liu]

On Fri, Dec 18, 2015 at 10:01:25AM -0800, Rich Lane wrote:
> I'm using the vhost callbacks and struct virtio_net with the vhost PMD in a few
> ways:

Rich, thanks for the info!

> 
> 1. new_device/destroy_device: Link state change (will be covered by the link
> status interrupt).
> 2. new_device: Add first queue to datapath.

I'm wondering why vring_state_changed() is not used, as it will also be
triggered at the beginning, when the default queue (the first queue) is
enabled.

> 3. vring_state_changed: Add/remove queue to datapath.
> 4. destroy_device: Remove all queues (vring_state_changed is not called when
> qemu is killed).

I had a plan to invoke vring_state_changed() to disable all vrings
when destroy_device() is called.

> 5. new_device and struct virtio_net: Determine NUMA node of the VM.

You can get the 'struct virtio_net' dev from all above callbacks.
 
> 
> The vring_state_changed callback is necessary because the VM might not be using
> the maximum number of RX queues. If I boot Linux in the VM it will start out
> using one RX queue, which can be changed with ethtool. The DPDK app in the host
> needs to be notified that it can start sending traffic to the new queue.
> 
> The vring_state_changed callback is also useful for guest TX queues to avoid
> reading from an inactive queue.
> 
> API I'd like to have:
> 
> 1. Link status interrupt.

To vhost pmd, new_device()/destroy_device() equals to the link status
interrupt, where new_device() is a link up, and destroy_device() is link
down().


> 2. New queue_state_changed callback. Unlike vring_state_changed this should
> cover the first queue at new_device and removal of all queues at
> destroy_device.

As stated above, vring_state_changed() should be able to do that, except
the one on destroy_device(), which is not done yet.

> 3. Per-queue or per-device NUMA node info.

You can query the NUMA node info implicitly by get_mempolicy(); check
numa_realloc() at lib/librte_vhost/virtio-net.c for reference.

	--yliu
> 
> On Thu, Dec 17, 2015 at 8:28 PM, Tetsuya Mukawa <mukawa at igel.co.jp> wrote:
> 
>     On 2015/12/18 13:15, 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?
> 
>     I agree with you. But will wait a few days.
>     Because if someone wants to use it from vhost PMD, they probably will
>     provides use cases.
>     And if there are no use cases, let's do like above.
> 
>     Thanks,
>     Tetsuya
> 
>