Re: [dpdk-dev] [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 08:47:28PM -0800, Rich Lane wrote:
> On Mon, Dec 21, 2015 at 7:41 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com>
> wrote:
> 
>     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.
> 
> 
> Turns out I'd misread the code and it's already using the vring_state_changed
> callback for the
> first queue. Not sure if this is intentional but vring_state_changed is called
> for the first queue
> before new_device.

Yeah, you were right, we can't count on this vring_state_changed(), for
it's sent earlier than vring has been initialized on vhost side. Maybe
we should invoke vring_state_changed() callback at new_device() as well.

>  
> 
>     > 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.
> 
> 
> That would be good.
>  
> 
>     > 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.
> 
>      
> 
>     > 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.
> 
> 
> Your suggestions are exactly how my application is already working. I was
> commenting on the
> proposed changes to the vhost PMD API. I would prefer to
> use RTE_ETH_EVENT_INTR_LSC
> and rte_eth_dev_socket_id for consistency with other NIC drivers, instead of
> these vhost-specific
> hacks.

That's a good suggestion.

> The queue state change callback is the one new API that needs to be
> added because
> normal NICs don't have this behavior.

Again I'd ask, will vring_state_changed() be enough, when above issues
are resolved: vring_state_changed() will be invoked at new_device()/
destroy_device(), and of course, ethtool change?

	--yliu

> 
> You could add another rte_eth_event_type for the queue state change callback,
> and pass the
> queue ID, RX/TX direction, and enable bit through cb_arg. The application would
> never need
> to touch struct virtio_net.