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

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/12/24 12:51, Yuanhan Liu wrote:
> On Wed, Dec 23, 2015 at 11:00:15PM +0100, Thomas Monjalon wrote:
>> 2015-12-23 10:44, Yuanhan Liu:
>>> On Tue, Dec 22, 2015 at 01:38:29AM -0800, Rich Lane wrote:
>>>> On Mon, Dec 21, 2015 at 9:47 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>> wrote:
>>>>
>>>>     On Mon, Dec 21, 2015 at 08:47:28PM -0800, Rich Lane wrote:
>>>>     > 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?
>>>>
>>>>
>>>> It would be sufficient. It is not a great API though, because it requires the
>>>> application to do the conversion from struct virtio_net to a DPDK port number,
>>>> and from a virtqueue index to a DPDK queue id and direction. Also, the current
>>>> implementation often makes this callback when the vring state has not actually
>>>> changed (enabled -> enabled and disabled -> disabled).
>>>>
>>>> If you're asking about using vring_state_changed() _instead_ of the link status
>>>> event and rte_eth_dev_socket_id(),
>>> No, I like the idea of link status event and rte_eth_dev_socket_id();
>>> I was just wondering why a new API is needed. Both Tetsuya and I
>>> were thinking to leverage the link status event to represent the
>>> queue stats change (triggered by vring_state_changed()) as well,
>>> so that we don't need to introduce another eth event. However, I'd
>>> agree that it's better if we could have a new dedicate event.
>>>
>>> Thomas, here is some background for you. For vhost pmd and linux
>>> virtio-net combo, the queue can be dynamically changed by ethtool,
>>> therefore, the application wishes to have another eth event, say
>>> RTE_ETH_EVENT_QUEUE_STATE_CHANGE, so that the application can
>>> add/remove corresponding queue to the datapath when that happens.
>>> What do you think of that?
>> Yes it is an event. So I don't understand the question.
>> What may be better than a specific rte_eth_event_type?
> The alternative is a new set of callbacks, but judging that we already
> have a set of callback for vhost libraray, and adding a new set to vhost
> pmd doesn't seem elegant to me.
>
> Therefore, I'd prefer a new eth event.
>
> 	--yliu

I am ok to have one more event type.

BTW, I have questions about numa_node.
I guess "rte_eth_dev_socket_id()" can only return numa_node of specified
port.
If multiple queues are used in one device(port), can we say all queues
are always in same numa_node?

If the answer is no, I am still not sure we can remove "struct
virtio_net" from DPDK application callback handling.
I agree we can add RTE_ETH_EVENT_QUEUE_STATE_CHANGE for interrupt notice.
But current ethdev APIs may not be able to hide vhost specific
properties, then the callback hander needs to handle "struct virtio_net"
directly.

Thanks,
Tetsuya