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

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Mon, Dec 21, 2015 at 11:10:10AM +0900, Tetsuya Mukawa wrote:
> nes: 168
> 
> On 2015/12/19 3:01, Rich Lane wrote:
> > I'm using the vhost callbacks and struct virtio_net with the vhost PMD in a
> > few ways:
> >
> > 1. new_device/destroy_device: Link state change (will be covered by the
> > link status interrupt).
> > 2. new_device: Add first queue to datapath.
> > 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).
> > 5. new_device and struct virtio_net: Determine NUMA node of the VM.
> >
> > 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.
> > 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.
> > 3. Per-queue or per-device NUMA node info.
> 
> Hi Rich and Yuanhan,
> 
> As Rich described, some users needs more information when the interrupts
> comes.
> And the virtio_net structure contains the information.
> 
> I guess it's very similar to interrupt handling of normal hardware.
> First, a interrupt comes, then an interrupt handler checks status
> register of the device to know actually what was happened.
> In vhost PMD case, reading status register equals reading virtio_net
> structure.
> 
> So how about below specification?
> 
> 1. The link status interrupt of vhost PMD will occurs when new_device,
> destroy_device and vring_state_changed events are happened.
> 2. Vhost PMD provides a function to let the users know virtio_net
> structure of the interrupted port.
>    (Probably almost same as "rte_eth_vhost_portid2vdev" that I described
> in "[PATCH v5 3/3] vhost: Add helper function to convert port id to
> virtio device pointer")

That is one option, to wrapper everything into the link status interrupt
handler, and let it to query the virtio_net structure to know what exactly
happened, and then take the proper action.

With that, we could totally get rid of the "two sets of vhost callbacks".
The interface is a also clean. However, there is a drawback: it's not
that extensible: what if vhost introduces a new vhost callback, and it
does not literally belong to a link status interrupt event?

The another options is to introduce a new set of callbacks (not based on
vhost, but based on vhost-pmd as I suggested before). Here we could
rename the callback to make it looks more reasonable to a pmd driver,
say, remove the new_device()/destroy_device() and combine them as one
callback: link_status_changed.

The good thing about that is it's extensible; we could easily add a
new callback when there is a new one at vhost. However, it's not that
clean as the first one. Besides that, two sets of callback for vhost
is always weird to me.

Thoughts, comments?

	--yliu